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

iAPI: Fix the logic path that merges plain objects #68579

Merged
merged 2 commits into from
Jan 9, 2025

Conversation

luisherranz
Copy link
Member

@luisherranz luisherranz commented Jan 9, 2025

What?

This is a bug that David and I have been hunting for a while, and David finally found the problem. This regression was introduced in WP 6.7, so this fix should be released in WP 6.7.2.

Fixes a problem accessing getters on the client during the merge of the client and server states on client-side navigation, when the derived state is defined in the server as a Closure.

We should also stop serializing the derived state defined in the server using Closures as a follow-up. I've added the task to the WP 6.8 iteration.

Why?

Because the getters should never be executed without scope.

How?

Refactoring the deepMergeRecursive so it doesn't execute the getters.

@luisherranz luisherranz added [Type] Bug An existing feature does not function as intended [Feature] Interactivity API API to add frontend interactivity to blocks. [Packages] Interactivity /packages/interactivity labels Jan 9, 2025
@luisherranz luisherranz added this to the Gutenberg 20.0 milestone Jan 9, 2025
@luisherranz luisherranz requested a review from DAreRodz as a code owner January 9, 2025 18:09
Copy link

github-actions bot commented Jan 9, 2025

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: DAreRodz <[email protected]>
Co-authored-by: luisherranz <[email protected]>
Co-authored-by: priethor <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@luisherranz luisherranz added the Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) label Jan 9, 2025
@luisherranz luisherranz enabled auto-merge (squash) January 9, 2025 18:10
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.

LGTM!

@luisherranz luisherranz disabled auto-merge January 9, 2025 18:13
@luisherranz luisherranz enabled auto-merge (squash) January 9, 2025 18:14
@luisherranz luisherranz added Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release and removed Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release labels Jan 9, 2025
@luisherranz luisherranz disabled auto-merge January 9, 2025 18:18
@luisherranz luisherranz marked this pull request as draft January 9, 2025 18:20
@luisherranz luisherranz marked this pull request as ready for review January 9, 2025 18:30
Copy link

github-actions bot commented Jan 9, 2025

Flaky tests detected in 0a24a9a.
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/12696005927
📝 Reported issues:

@priethor priethor removed the Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) label Jan 9, 2025
@priethor
Copy link
Contributor

priethor commented Jan 9, 2025

This regression was introduced in WP 6.7, so this fix should be released in WP 6.7.2.

Since this issue was introduced in 6.7, should we add back the Backport to WP minor release label?

I launched earlier the Gutenberg 20.0 release process when this PR wasn't ready, and therefore, I removed the Backport to Gutenberg RC label; however, it is now ready to merge (that was fast 🙂 ). It doesn't to me that this patch warrants the release of a minor GB version, but I'll defer to you if you think so.

@luisherranz
Copy link
Member Author

should we add back the Backport to WP minor release label?

Ah, I thought I had to add that label in the pull request of the backport to the WordPress 6.7 branch. I was planning on doing that next.

It doesn't to me that this patch warrants the release of a minor GB version, but I'll defer to you if you think so.

No, don't worry, it can go in the next version.

@luisherranz luisherranz removed this from the Gutenberg 20.0 milestone Jan 9, 2025
@luisherranz luisherranz merged commit be95ec3 into trunk Jan 9, 2025
70 checks passed
@luisherranz luisherranz deleted the fix/iapi-deep-merge-recursive-getter-checking branch January 9, 2025 19:33
@luisherranz luisherranz restored the fix/iapi-deep-merge-recursive-getter-checking branch January 9, 2025 19:33
@github-actions github-actions bot added this to the Gutenberg 20.1 milestone Jan 9, 2025
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. [Packages] Interactivity /packages/interactivity [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