Block Bindings: Only pass context included in usesContext from rich text component
#65618
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SantosGuillamot
added
[Type] Bug
SantosGuillamot
requested review from
gziolo,
artemiomorales and
cbravobernal
|
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. |
|
Size Change: +30 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
It's better to start this way. |
|
I was considering three options for the rich text:
|
gziolo
reviewed
Sep 25, 2024
| @@ -179,11 +179,17 @@ export function RichTextWrapper( | |||
| const blockBindingsSource = getBlockBindingsSource( | |||
| relatedBinding.source | |||
| ); | |||
| const blockBindingsContext = {}; | |||
There was a problem hiding this comment.
I wanted to highlight the differences I discovered with how the code feeds other API methods with the context.
gutenberg/packages/block-editor/src/hooks/use-bindings-attributes.js
Lines 139 to 142 in b681b50
gutenberg/packages/block-editor/src/hooks/use-bindings-attributes.js
Lines 162 to 167 in b681b50
gutenberg/packages/block-editor/src/hooks/use-bindings-attributes.js
Lines 238 to 248 in b681b50
While updatedContext is correct in that code as it eventually gets passed to the <Edit /> component, I don't think we should use updatedContext when passing context to setValues and getValues. Instead, we should replicate the handling proposed in this PR. In effect, useSelect defined for the source on the server and/or client would be the ultimate contract of what these API methods can get as params.
I would proceed with this PR as planned. If we agree the same changes should get applied in other places seperately, then there should be two subsets of the context:
blockBindingsContextlike in this PR that gets passed to the methods registered for the sourceupdatedContextwould be refactored to:const updatedContext = { ...context, ... blockBindingsContext };
There was a problem hiding this comment.
Unifying and using only the context defined in usesContext for setValues and getValues makes sense to me. I can work on that in a follow-up.
There was a problem hiding this comment.
I'm exploring changing this in this pull request. I believe just changing how updatedContext is initialized should be enough.
gziolo
approved these changes
Sep 25, 2024
There was a problem hiding this comment.
Filter the BlockContext to pass only the properties included in source.usesContext: This is what this PR is doing.
I arrived at the same conclusion. The API support usesContext on both the server and the client so we should treat that as a source of truth and align with the same behavior for block types. In effect, only the context defined during registration gets exposed. More about that in https://github.com/WordPress/gutenberg/pull/65618/files#r1775077099.
SantosGuillamot
deleted the
fix/only-pass-bindings-context-from-rich-text
branch
gutenbergplugin
pushed a commit
that referenced
this pull request
Sep 25, 2024
- Only pass context included in `usesContext` Co-authored-by: SantosGuillamot <[email protected]> Co-authored-by: gziolo <[email protected]>
github-actions
bot
added
Backported to WP Core
|
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 31de263 |
SantosGuillamot
removed
the
Backport to Gutenberg RC
64 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
It passes only the context properties included in
usesContextblock bindings definition instead of the wholeblockContext.Why?
It seems safer to limit the context provided to the block bindings editor APIs to only the values defined through
usesContextproperty.How?
I just added a similar check to what we have in other places like this.
I tried getting the BlockEdit context from the rich text, but I don't think that's possible.
Testing Instructions
Everything should keep working as expected. e2e tests should pass.