Skip to content

Fix server processing of wp-context getting out of sync#55436

Merged
luisherranz merged 2 commits intotrunkfrom
fix/server-wp-context-processing-rewind-out-of-sync
Oct 18, 2023
Merged

Fix server processing of wp-context getting out of sync#55436
luisherranz merged 2 commits intotrunkfrom
fix/server-wp-context-processing-rewind-out-of-sync

Conversation

@luisherranz
Copy link
Copy Markdown
Member

@luisherranz luisherranz commented Oct 18, 2023

What?

Fix a problem where the internal stack of contexts could get out of sync when the JSON is not valid during the server processing of wp-context.

Why?

As spotted by @DAreRodz here, the internal stack of contexts can get out of sync when a call to set_context fails.

For context, the "Fatal error" that can be seen in #54816 (comment) is caused by a problem insidegutenberg_interactivity_process_wp_context implementation, which is calling $context->rewind_context() even when no context where added on a potential failure. That could empty the context stack and make subsequent $context->set_context( $new_context ) calls fail.

How?

Make sure that we're always adding a new array to the stack, even if the JSON is not valid.

Testing Instructions

This is covered by a unit test, but if you want to test it out:

  • Add an interactive block.
  • Add a div with valid JSON in a wp-context directive.
  • Add nested divs inside with wp-context and invalid JSONs.
  • Check that it doesn't do a fatal error and that the initial context is still valid.

@luisherranz luisherranz added [Type] Bug An existing feature does not function as intended [Feature] Interactivity API API to add frontend interactivity to blocks. labels Oct 18, 2023
@luisherranz luisherranz self-assigned this Oct 18, 2023
@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 18, 2023

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/experimental/interactivity-api/class-wp-directive-context.php
❔ lib/experimental/interactivity-api/directives/wp-context.php
❔ phpunit/experimental/interactivity-api/directives/wp-context-test.php

@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 18, 2023

Flaky tests detected in f341fc1.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6558724095
📝 Reported issues:

DAreRodz
DAreRodz approved these changes Oct 18, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@DAreRodz DAreRodz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me. Approved!

PS: There's a thing that worries me: this is supposed to fail only when the content of wp-context is not valid. Are we generating an invalid context somewhere, or could it be a different issue this time? 🤔

@luisherranz
Copy link
Copy Markdown
Member Author

luisherranz commented Oct 18, 2023

For now, let's cover invalid contexts. If more problems arise, we'll check them out 🙂

Thanks for the quick review, David! 🎉

@luisherranz luisherranz merged commit 9905301 into trunk Oct 18, 2023
@luisherranz luisherranz deleted the fix/server-wp-context-processing-rewind-out-of-sync branch October 18, 2023 10:10
@github-actions github-actions bot added this to the Gutenberg 16.9 milestone Oct 18, 2023
This was referenced Oct 18, 2023
Merged
Closed
Merged
@mikachan
Copy link
Copy Markdown
Member

mikachan commented Oct 23, 2023

👋 Hey! Checking if this PR should be included in 6.4 - if so, we should add the Backport to WP Beta/RC label. Thanks!

@luisherranz
Copy link
Copy Markdown
Member Author

luisherranz commented Oct 23, 2023

Hey @mikachan. No, this part is still experimental and it's not included in the 6.4 release.

@getdave getdave mentioned this pull request Jan 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DAreRodz DAreRodz DAreRodz approved these changes

@noahtallen noahtallen Awaiting requested review from noahtallen

@spacedmonkey spacedmonkey Awaiting requested review from spacedmonkey spacedmonkey is a code owner

Assignees

@luisherranz luisherranz

Labels

[Feature] Interactivity API API to add frontend interactivity to blocks. [Type] Bug An existing feature does not function as intended

Projects

None yet

Milestone

Gutenberg 16.9

Development

Successfully merging this pull request may close these issues.

3 participants

@luisherranz @mikachan @DAreRodz