-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Interactivity API: Prevent non-object state from being added #59886
Conversation
Size Change: +5 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
The status check is confused here, it says there's a failing label job, but that seems stake or wrong. I do need to add a changelog before this lands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix looks good to me, 😄 although I would move the test to store.spec.ts
and packages/e2e-tests/plugins/interactive-blocks/store/
, as those are the test files we currently have for the store()
function.
Could you update that before merging?
5a5c346
to
922919e
Compare
I've updated the tests as requested and verified they fail if I revert the fix:
|
* Add mode to directive processing * Add #59886 wp-develop fix * Add #6251 fix * Fix linter * Include #6261 wp-develop * Fix spacing Co-authored-by: c4rl0sbr4v0 <[email protected]> Co-authored-by: SantosGuillamot <[email protected]> Co-authored-by: sirreal <[email protected]>
Ensure that `state` is an object when creating a store for a given namespace. `store.state` is expected to be an object. There is code that prevents state from being updated if it is not an object, but it's possible to set the state to a non-object initially, which breaks subsequent updates. Co-authored-by: sirreal <[email protected]> Co-authored-by: DAreRodz <[email protected]>
I just cherry-picked this PR to the update/packages-rc2-6.5 branch to get it included in the next release: 6421202 |
Ensure that `state` is an object when creating a store for a given namespace. `store.state` is expected to be an object. There is code that prevents state from being updated if it is not an object, but it's possible to set the state to a non-object initially, which breaks subsequent updates. Co-authored-by: sirreal <[email protected]> Co-authored-by: DAreRodz <[email protected]>
…ss#59886) Ensure that `state` is an object when creating a store for a given namespace. `store.state` is expected to be an object. There is code that prevents state from being updated if it is not an object, but it's possible to set the state to a non-object initially, which breaks subsequent updates. Co-authored-by: sirreal <[email protected]> Co-authored-by: DAreRodz <[email protected]>
* Add mode to directive processing * Add WordPress#59886 wp-develop fix * Add WordPress#6251 fix * Fix linter * Include WordPress#6261 wp-develop * Fix spacing Co-authored-by: c4rl0sbr4v0 <[email protected]> Co-authored-by: SantosGuillamot <[email protected]> Co-authored-by: sirreal <[email protected]>
What?
Prevent interactivity store state from being set to non-object.
Why?
store.state
is expected to be an object. There is code that prevents state from being updated if it is not an object, but it's possible to set the state to a non-object initially, which breaks subsequent updates.See WordPress/wordpress-develop#6261 (comment) and this Core Trac comment for related details.
How?
isObject
checks are applied when updating state, apply isObject to initial state as well, default to{}
if non-object state was provided.Testing Instructions
This PR includes e2e tests. You can see a failing run before the fix was pushed here.