Block Bindings API: Add block bindings PHP registration mechanisms and "Post meta" source under the experimental flag #57249
Conversation
SantosGuillamot
added
[Feature] Block API
SantosGuillamot
requested review from
spacedmonkey,
ellatrix and
ajitbohra
as code owners
|
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/block-bindings/html-processing.php ❔ lib/experimental/block-bindings/index.php ❔ lib/experimental/block-bindings/sources/index.php ❔ lib/experimental/block-bindings/sources/pattern.php ❔ lib/experimental/block-bindings/sources/post-meta.php ❔ lib/block-supports/pattern.php ❔ lib/experimental/blocks.php ❔ lib/experimental/editor-settings.php ❔ lib/experiments-page.php |
|
Size Change: +35 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
michalczaplinski
changed the title
SantosGuillamot
added
the
[Feature] Custom Fields
There was a problem hiding this comment.
Fantastic job with this and the related PRs 🙌 I’ve followed the testing instructions and everything works as expected. While I don’t have enough experience with Gutenberg to advise on best practices there, I have a few general thoughts.
At a high level, as per our conversation @SantosGuillamot, I think a minimal version like this without any visible UI could be good to merge into core for 6.5 (though perhaps a bit early for the partially synced patterns). Regardless, I think we can just focus on merging the prototype to Gutenberg and collecting feedback rather than worrying too much about core for the moment.
A heads up that both @c4rl0sbr4v0 and I found the customizable attributes in the partially synced patterns to have a confusing label, since these attributes are in fact the ones that will not be synchronized. Would we be able to rename this heading from Synced Attributes to Customizable Attributes or Decoupled Attributes or another more apt name?
Test that multiple attributes values can be connected
Repeat the same process but using a button (or an image) to bind the content and the URL.
To help folks who are testing and may not have a chance to look in-depth at the code, we should note that currently 1.) for the button, only the url and text attributes work, and 2.) for the image, only the url and title attributes work. On that note, how about we also expose the alt attribute for the image? I know we’re mostly in a phase of getting feedback at the moment, but that small change would make this more useable in the wild.
Lastly, I saw in the previous PR's description that you were still undecided on the format of the bindings property. Has that discussion now been settled? Note that I don’t have a strong opinion in any direction — I’m just curious to hear if it's been resolved or requires further input.
Will continue looking at the code and offer additional thoughts as they arise. Thanks 🙏
|
Thanks for sharing your thoughts, Artemio 🙂
Would you mind sharing this feedback in the Synced Patterns issue: link? This PR just focuses on the block bindings API, that is used by synced patterns. But their UI is independent of this.
I discussed it with some folks, and we agreed on an initial syntax, trying to leave as few remaining decisions as possible. But if you, or anyone else, believes another syntax is better, we can still modify it. |
I just checked out 
Ok great. I think the initial syntax we have is good 👍 |
| @@ -18,51 +18,45 @@ import { PARTIAL_SYNCING_SUPPORTED_BLOCKS } from '../constants'; | |||
| function PartialSyncingControls( { name, attributes, setAttributes } ) { | |||
| const syncedAttributes = PARTIAL_SYNCING_SUPPORTED_BLOCKS[ name ]; | |||
There was a problem hiding this comment.
Not sure if this PR would be the best place to do this, but I think we should rename this to editableAttributes or something similar for clarity, as these are the attributes that users will be able to override.
|
I renamed some variables for clarity and cleaned up some code around the button markup in 176da0a, though feel free to change it back or continue modifying. As far as I can tell, everything in the PR still seems to work. |
We recently modified the control to be a single checkbox in #57009. We just need a rebase here 🙂. I'm happy to help too! |
@kevin940726 Great, thanks! I've opened a throwaway PR in my fork to illustrate how I'm thinking to approach the rebase. I believe it works as expected, but would you be able to take a look? Note that I added a commit to attempt to actually use the multiple overridden attributes in the pattern, but it did not work for the image — only one attribute was read and it was erroneously used for both the I see here however that support for multiple attributes does not yet seem to be added in the editor code for the partially synced pattern. Am I following this correctly? I just want to check in before I get too deep. Any guidance appreciated! |
Yes, that's correct! We don't currently support them at the time, but we're planning to add support soon. The rebase PR looks good! It's mostly the same as what I did 😆. I think the next step for us would be adding support for |
|
Noting here that I've come across a bug — when enabling pattern overrides, the original content disappears when the pattern is used in an instance. I won't have time to look at this until next week, but I'm documenting it here in case anyone is able to look. pattern-override-bug.mp4 |
There was a problem hiding this comment.
I'm approving with the caveat that there's still a lot to improve in terms of the API as well as removing the old block supports code and other improvements.
There's already a lot happening here though so I think it makes sense to begin addressing these in follow up PRs, as the partially synced patterns cleanup especially seems to be moving beyond the scope of what's being proposed here.
Outstanding Items
Finish removing block supportsReformulate registration mechanisms- Begin implementing singleton pattern via a Block Bindings class
I noticed onEdit: It turns out this is now expected behavior.trunktoday that the block UI has gone missing from pattern instances when the partially synced patterns experiment is enabled, and while not caused by this PR, it prevents the functionality here from working and also needs to be fixed.
| @@ -13,7 +13,7 @@ | |||
| * @param WP_Block_Type $block_type Block Type. | |||
| */ | |||
| function gutenberg_register_pattern_support( $block_type ) { | |||
| $pattern_support = property_exists( $block_type, 'supports' ) ? _wp_array_get( $block_type->supports, array( '__experimentalConnections' ), false ) : false; | |||
| $pattern_support = property_exists( $block_type, 'supports' ) ? _wp_array_get( $block_type->supports, array( '__experimentalBlockBindings' ), false ) : false; | |||
|
|
|||
| if ( $pattern_support ) { | |||
| if ( ! $block_type->uses_context ) { | |||
There was a problem hiding this comment.
If we're removing block supports. we should probably remove this entire file, which exists inside the EDIT: Since we may be adding block supports in the future, perhaps removing this file isn't necessary.block-supports/ directory, and put its logic elsewhere.
There was a problem hiding this comment.
Agree with Artemio's review above.
I would only add more context to this:
Reformulate registration mechanisms
I think that there are 2 aspects to this:
- Reconsidering what parameters should be passed to the
applyas already mentioned by Dan in Block Bindings API: Add block bindings PHP registration mechanisms and "Post meta" source under the experimental flag #57249 (comment) - Possibly simplifying the signature of
register_block_bindings_source.
I believe that instead ofregister_block_bindings_source( $source_name, $source_args )where `$source_args is an associative array, the function could have 3 parameters:
function register_block_bindings_source( $source_name, $label, $apply) {
global $block_bindings_sources;
$block_bindings_sources[ $source_name ] = array( $label, $apply );
}This way, the PHPDoc is much simpler and the signature is simplified as well.
I think that this and removing the block supports are the only 2 items we should address before merging.
|
Ok @michalczaplinski I've removed all of the block supports and hardcoded support for just the paragraph block, as that's all that's currently enabled for pattern overrides. I've also updated the signature of the register function. Let me know what you think! |
|
Flaky tests detected in 23629de. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7489673430
|
michalczaplinski
deleted the
add/block-bindings-php-registration-mechanisms
branch
|
@kevin940726, does it mean that Pattern Overrides work only with the Paragraph block at the moment? Who is working on the follow-up PR? |
Yes. Because both |
| } | ||
| $source_value = $source_callback( $source_args, $block_instance, $binding_attribute ); | ||
| // If the value is null, process next attribute. | ||
| if ( is_null( $source_value ) ) { |
There was a problem hiding this comment.
I was just reminded that if the attribute accepts null or undefined as valid values for different meanings, then we should still be able to apply overrides here. Do you think we need a special value here to represent none? 🤔
There was a problem hiding this comment.
@kevin940726 can you elaborate?
What do you think @SantosGuillamot @michalczaplinski?
I wonder how we would handle a null or undefined value as valid here or here. It seems to me that what makes sense intuitively is for null or undefined to mean that there is no valid value to apply, but I also don't know enough about how developers would use this in the wild to have a strong opinion either way.
There was a problem hiding this comment.
For context, I was trying to add support for the linkTarget attribute of the button block. It accepts a string (like _blank) or undefined for no target. Let's say we have a source pattern with a button block and its linkTarget is _blank. Now we want to override the source pattern in an instance to remove the linkTarget. We have two problems here:
- On the editor side, we need a way to represent
undefinedbecause it's not JSON serializable. - On the backend, we need a way to distinguish
NULLfrom "skipping the attribute" because sometimesNULLis a valid value for some attributes.
I opened two draft PRs exploring different solutions for this problem and I'd love all your feedback!
- Support
linkTargetofcore/buttonfor Pattern Overrides #57947: Treats non-existent attributes the same asnulland usesnullthroughout to represent the attribute. - Support removing an attribute for Pattern Overrides #57993: Assumes
undefinedandnullcould be handled differently, and uses a patch similar to JSON patch to represent overrides. This also opens up future possibilities to represent "inserting" a partial override.
Note that the latter changes the overrides attribute format so it'd be better if we decide the format sooner than later to save us from back-compat pain 😅.
What?
Related issues:
#54536
#53300
#56867
I'm splitting the block bindings API prototype into smaller PRs so they are more manageable and discussions can be kept separately. You can find more information in the original PR.
This pull request covers:
register_block_bindings_sourcethat allow users to create new sources, passing a name, a label and a callback.set_inner_htmlfunction from the HTML API once that is ready.post_metasource, that binds block attributes and custom fields.pattern_attributessource, that is needed for partially synced patterns.This pull request doesn't add a UI to create the bindings, so the attribute has to be added manually.
Why?
This pull request is the basis for the block bindings API which will enable the connection between block attributes and other sources like custom fields.
As mentioned, I am splitting the original prototype into smaller PRs.
Testing Instructions
For all the testing we have to go to Gutenberg-Experiments and enable the Test Block Bindings option.
Test that adding the bindings attribute works
Test that multiple attributes values can be connected
Repeat the same process but using a button (or an image) to bind the content and the URL.
Check that the old UI is not there
In the existing experiment, the UI used doesn't make sense anymore.
Test that new sources can be registered
register_block_bindings_sourcefunction. In this case, I'm adding a "Shortcode" source and create a custom one to test it:Test that partially synced patterns keep working as expected
Partially synced patterns experiment should keep working as it does before this PR.