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.
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 #6079Block Bindings: Add block
uses_context
defined by block bindings sources #6079Changes from 29 commits
63d381f
65e259a
68ce2a8
ade4119
5011e16
929afb6
44125a4
5a931f4
dccef2e
016ec29
e305f7c
49a8b7a
42575c4
e41ba30
4ca8176
46df776
59746c0
7bcc71e
5810180
a578d5c
9f01568
ba7bf25
0c4a795
7e80f5e
d839f33
e82879e
102c940
f4af4f7
f2b3b3a
4f8668d
07a7317
f14817f
edaaffa
03e66ee
0b49cc4
9838fec
59fde29
5f2ddc3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
But we probably need a different callback name per source.
Anyway, I'll add a task in the tracking issue to handle it 🙂
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 checksin
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:
wordpress-develop/src/wp-includes/class-wp-block-type.php
Line 17 in 7b24083
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 withuses_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.