Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

luisherranz
Copy link
Member

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

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

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:

Copy link
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
Member Author

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
55 checks passed
@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
@mikachan
Copy link
Member

👋 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
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Interactivity API API to add frontend interactivity to blocks. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants