Allow contents of Paragraph to be "connected" to a meta custom field #53247
Conversation
…nto paragraph-block-custom-fields
michalczaplinski
added
[Block] Paragraph
|
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/connection-sources/index.php ❔ lib/experimental/blocks.php |
michalczaplinski
changed the base branch from
experiment/add-experimental-custom-fields-setting
to
experiment/add-custom-connectinons-inspector-control
|
Size Change: +27 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 39cb931. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5739684755
|
Changed the attribute access from `$block_type->attributes` to `$block['attrs']`. Also updated attribute 'source' value from 'meta' to 'meta_fields'. These changes have been applied in both `blocks.php` and block attributes declaration in `block.json`.
…nto paragraph-block-custom-fields
…nto paragraph-block-custom-fields
Rearranged the block type checks. Now, the code first checks if a given block is in the accepted types list (Paragraph and Image) and then it checks if the block type is null. This is a slight revamp of the existing checks for block support and block type acceptance.
- Removed unused test functions. - Update the comment above the tag name query in the `custom-sources` to clarify future improvements for supporting more block types.
- Separate the "connections" into own file. - Move the code that renders the HTML using the meta fields back into blocks.php
| * Filters registered block settings, extending attributes to include `__experimentalConnections`. | ||
| * Filters registered block settings, extending attributes to include `connections`. | ||
| * | ||
| * @param {Object} settings Original block settings. | ||
| * | ||
| * @return {Object} Filtered block settings. | ||
| */ | ||
| function addAttribute( settings ) { | ||
| if ( hasBlockSupport( settings, '__experimentalConnections', true ) ) { | ||
| // Gracefully handle if settings.attributes is undefined. | ||
| // Gracefully handle if settings.attributes.connections is undefined. | ||
| settings.attributes = { | ||
| ...settings.attributes, | ||
| __experimentalConnections: { | ||
| connections: { |
There was a problem hiding this comment.
I'm not sure why these changes show up in GithHub's UI. They are already in the other PR: #53241
There was a problem hiding this comment.
Maybe we need to rebase/merge the other PR branch. If I am not mistaken the last merge was done before the change you mention.
There was a problem hiding this comment.
yep, I had to merge experiment/add-custom-connectinons-inspector-control into this one before pushing again 🙂
michalczaplinski
requested review from
spacedmonkey,
ellatrix and
ajitbohra
as code owners
- "custom sources" -> connection sources - use "block connections" consistently
|
Great, I've just updated the naming to consistently use:
I'll first merge #53241 and then this one. |
…nto paragraph-block-custom-fields
| $blocks_attributes_allowlist = array( | ||
| 'core/paragraph' => array( 'content' ), | ||
| 'core/image' => array( 'url' ), | ||
| ); |
There was a problem hiding this comment.
Can we retrieve this from a block support instead of a hardcoded list.
There was a problem hiding this comment.
Actually it's the user that should define which attribute to connect, so this is coming from the block attribute value normally no?
There was a problem hiding this comment.
See my comment in #53241 (comment) :)
Yes, but for now we want to limit this to core blocks and those two attributes. We'll expand it in the future.
So far it's not well-defined how users are going to opt-in to connecting their custom blocks' attributes to custom fields.
| } | ||
|
|
||
| // Get the content from the connection source. | ||
| $custom_value = $connection_sources[ $attribute_value['source'] ]( |
There was a problem hiding this comment.
This is weird, I'd have preferred a unique name for all connection, like get_value or apply or something.
There was a problem hiding this comment.
Oh, maybe the confusion is coming from this line:
I should have removed the "name" because it serves no purpose at the moment.
So in your PR, there was an assumption that there's one file per connection source (attributes-custom-sources/meta.php, attributes-custom-sources/some-other-source.php) and here they are all in under keys of one array. I say "all" but there is only one type of connection source, anyway 😅 .
There was a problem hiding this comment.
ah ok, I guess depending on the complexity we could split but agree, seems like implementation detail for now but let's aim for the simplest. I do expect us to allow registering sources at some point so I think "one config array per source" is probably better ultimately (as opposed to all configs in a single array).
| @@ -7,6 +7,7 @@ | |||
| "description": "Start with the basic building block of all narrative.", | |||
| "keywords": [ "text" ], | |||
| "textdomain": "default", | |||
| "usesContext": [ "postId" ], | |||
There was a problem hiding this comment.
I wonder whose responsibility is it to do that. In other words, the block is not using the "postId" explicitly, so why add the "useContext" here. The "connection framework" should inject/filter this somehow.
There was a problem hiding this comment.
Do you mean that in the context of Core blocks or custom blocks or both?
There was a problem hiding this comment.
OK! For now, I think that for core blocks this is fine. But you're right that for custom blocks we'll have to come up with a proper mechanism here. Are you all right with that?
There was a problem hiding this comment.
Yeah I think it's part of the definition of a "custom source" to say which change it needs to make to block registration. I think we should use it for both core and third-party blocks but yeah it's fine to start without it.
There was a problem hiding this comment.
For now, you could update usesContext and pass postId in PHP when processing gutenberg_render_block_connections. This way, it won't get unnecessarily wired for all Paragraph block instances through block.json definition.
There was a problem hiding this comment.
Even though this is the first implementation and custom blocks are not supported yet, I believe it'd be better to start creating mechanisms that would work for both if possible. Mainly because we already know we will want to support that in the future.
There was a problem hiding this comment.
Coming in a bit late here.
The current work on partial syncing (#54233) is using the store actions/selectors for attributes to inject/update values, but it might be difficult to pull this off if block context is being used for custom fields, as I believe it uses react context in JS.
While they're still proofs of concepts, the current work for custom fields/partial syncing does need to remain compatible, so it might be good to explore ways to read/write custom field values in the editor soon though so that we have a clear idea of the problems that need to be solved.
| return $block_content; | ||
| }; | ||
| $tag_name = $tags->get_tag(); | ||
| $markup = "<$tag_name>$custom_value</$tag_name>"; |
There was a problem hiding this comment.
This line is scary. Any plans for escaping? It's not trivial, since the goal is to be able to inject rich text and not just simple values.
There was a problem hiding this comment.
thanks for catching this @mcsf. this definitely should be "<$tag_name>" . esc_html( $custom_value ) . "</$tag_name>" for now.
actually @michalczaplinski let's talk about how to sub-class the Tag Processor so this is cleaner all over
There was a problem hiding this comment.
Thanks for raising it! We should definitely do that. Keep in mind that this code was considered experimental and we'll need to refactor it.
There was a problem hiding this comment.
Definitely, but all too often lines of experimental code somehow make their way into production code. :) I'd really patch ASAP, even if just with esc_html.
There was a problem hiding this comment.
Thanks for catching this!
I want to take a holistic look at all the code shipped behind the experimental flag. It's very likely most of it will be replaced altogether. The team is on support rotation this week so it'll have to wait till next week at least 🙂
@dmsnell yes, let's talk!
Following up on work in #51375
This PR is a counterpart to and stacked on top of #53241 because we need some code from there to make it work. So #53241 should be merged first.
What does this do?
We're adding a way to "connect" the contents of the Paragraph block to a meta custom field.
2023-08-01_19-19-24.mp4