-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Block Bindings: Add block uses_context
defined by block bindings sources
#6079
Block Bindings: Add block uses_context
defined by block bindings sources
#6079
Conversation
Hi @SantosGuillamot! 👋 Thank you for your contribution to WordPress! 💖 It looks like this is your first pull request to No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description. Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making. More information about how GitHub pull requests can be used to contribute to WordPress can be found in this blog post. Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook. If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook. The Developer Hub also documents the various coding standards that are followed:
Thank you, |
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 Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
I guess, according to the tests, that I shouldn't modify the |
That needs to happen in the Gutenberg plugin. It isn't as important, so that can be synced later during the 6.5 Beta phase as code quality improvement. You also need to plan to remove these |
@@ -60,6 +68,9 @@ public function __construct( string $name, array $source_properties ) { | |||
$this->name = $name; | |||
$this->label = $source_properties['label']; | |||
$this->get_value_callback = $source_properties['get_value_callback']; | |||
if ( isset( $source_properties['uses_context'] ) ) { |
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.
I think that we should move this isset()
check next to the other checks
in WP_Block_Bindings_Registry
.
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.
And let's cover it with a unit test as well
🙂. (Should be almost a copy-paste of the
existing ones)
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.
My idea is to make the uses_context
optional because some sources won't need to use it. Do you think it shouldn't be that case? It would make sense to add a check that, if it exists, it is an array though.
I'll work on unit tests as well 🙂
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.
Yes, it should be optional and default to null
.
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.
Right, that makes sense! It should be optional indeed 🙂
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.
If I understood you correctly, I believe I prefer the second option. I made this commit to illustrate how I understood it. We can always revert it.
PD: I included unnecessary code I was using for testing, I just removed it in other commit 😅
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.
Yes, it's exactly how it could work 👍🏻
It would be nice to add a unit test for the registry that asserts that it errors when an unknown property is provided.
At the same time, we are closing the source object for an extension, which I'm a bit unsure about, given we might need to change during beta if we figure out it prevents iterations in the Gutenberg plugin. We also miss the following annotation on the class if we decide to go that path:
#[AllowDynamicProperties] |
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.
I am unsure about restricting it as well. I can expect we might want to increase it in Gutenberg after external developers create their own sources the same way we need now to include uses_context
.
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.
On the other hand, if we allow source to use their own properties, could we face backwards-compatibility issues in the future?
Imagine a source is using a custom property format
. And in the future, we want to include it as part of the core properties and we use it somehow as we are doing with uses_context
. The source that had a custom property with the same name wouldn't be working as expected, right?
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.
I'll keep thinking about it as I don't have an ultimate answer for now as it's hard to tell what will be needed here.
This reverts commit 828dd93.
c4fb33f
to
e41ba30
Compare
I've created a bunch of tests to check |
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.
I left some additional feedback to consider, but it's already in good shape overall. Thank you for adding all the changes and covering the unit tests.
// Add `uses_context` defined by block bindings sources. | ||
add_filter( | ||
'get_block_type_uses_context', | ||
function ( $uses_context, $block_type ) use ( $source ) { |
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.
One more thing here and not a blocker. I believe we should use a class method and unregister the filter when unregistering the source. It would also help with documenting, as we could put all details from inline comments in the PHPDoc.
I'm not entirely sure how that would look in practice, but happy to explore together options. It can be another patch.
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.
This makes sense to me. Although I don't know how it will work 🤔 I was thinking something like this:
/**
* Source that is being processed.
*
* @since 6.5.0
* @var WP_Block_Bindings_Source|false
*/
private $source;
/**
* Callback passed to the `get_block_type_uses_context` filter to add the source's `uses_context` to the block.
*
* @since 6.5.0
*
* @param array $uses_context Array of registered uses context for a block type.
* @param WP_Block_Type $block_type The full block type object.
* @return array Modified array of uses_context with the values provided by the source.
*/
public function _add_source_uses_context( $uses_context, $block_type ) {
$source = $this->source;
if ( ! in_array( $block_type->name, $this->supported_blocks, true ) || empty( $source->uses_context ) ) {
return $uses_context;
}
// Use array_values to reset the array keys.
return array_values( array_unique( array_merge( $uses_context, $source->uses_context ) ) );
}
But we probably need a different callback name per source.
Anyway, I'll add a task in the tracking issue to handle it 🙂
Co-authored-by: Greg Ziółkowski <[email protected]>
Co-authored-by: Greg Ziółkowski <[email protected]>
Co-authored-by: Greg Ziółkowski <[email protected]>
Committed with https://core.trac.wordpress.org/changeset/57641. |
This pull request adds some logic to add the necessary
uses_context
defined by block bindings sources. Each source defines the context it needs and it is added to the blocks that are using the block bindings API.Apart from that, it adapts the post meta and pattern overrides sources for this change.
Finally, it removes the
pattern/overrides
from theblock.json
because it is handled this way now.Testing instructions
Post meta source test
Pattern overrides test
Trac ticket: https://core.trac.wordpress.org/ticket/60525
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.