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
Add the Block Bindings API #5888
Add the Block Bindings API #5888
Changes from 20 commits
8720e12
8891169
eac5b22
39e52ed
82e70ff
6486471
048330e
4a4b825
9a662e4
c92b6ac
abb56fb
e8b1195
cc6c2c2
206b6b7
dc8377a
751a459
423bb66
d1b0d2e
9f2274d
02f0ea3
9303e02
30f4ed7
940fb82
dbda027
34d9dd7
74bfae7
013535e
89e9833
0c931e0
7b1ebe2
b0c300f
e0b3883
2900bf5
26da23e
29213d8
4c3d892
06a3930
f53121d
5b8eb4a
45a96f8
08ddb0a
3a16941
1729f65
9ea2c9a
fd090ab
6ea69cd
15dea88
5893867
d10b165
c324e65
a0d550a
3f242d3
d202e5d
2a20dee
2b4cf33
8604e82
65a7c09
1ceef1b
3d97457
5f397aa
f2a611b
a1e1bac
26571a7
d189f50
d7201ce
69ed754
951ca16
f10a4fe
0bec833
3134faa
9491be3
c95b92f
3619e18
c8232ac
871cedc
604f7a2
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.
Do we have use-cases where the post id is an attribute and not something in the 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.
@SantosGuillamot might know better. I would assume we rather read it from the context or other block attributes through
$block_instance
param.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.
The idea was to allow people to select a specific ID to connect to. So, instead of connecting to the post title of the context, you connect to a specific post title.
I added it more as an example of other attributes that could make sense at some point.
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 there might be a bug here.
Instead of completely omitting the usage of the $block_instance->context['postId'] here, we should check if the value isset and if so use it over the ID we get from
get_the_ID
.Something like this:
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 also wonder if we should always use the context here. as if I'm not wrong the default context should be set as
get_the_ID
if there's no parent query block.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.
From what I tested,
$block_instance->context
is an empty array for the heading, button, and image block, so that's why we couldn't get thepostId
from there. Is that not expected?We could definitely use the conditional suggested. However, I'd like to understand if just using
$block_instance->context
should work.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.
Maybe we have an issue somewhere but in the client the block context has some "default context values" like the post ID if I'm not wrong, and in the server you're saying that these are not present. IMO, the block context should be the same for the client and the server.
I think it's fine if we defer the potential fix to a dedicated issue but we might want to track it somewhere.
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.
@SantosGuillamot @gziolo Heads up here --
This is lacking any access rights checks and finding/fixing this now means we avoid having to patch the eventual release. If someone provides any post ID, it could pull meta for any post ID of any post type of any status including password-protected posts.
Something like this could help, maybe only in the conditional above where a custom post ID is set but to be safe we could have it here before the return.
We may also want to check whether the associated post type is public + publicly_queryable to ensure that we follow the same constraints established by the Query Loop block for dynamically embedding a list posts themselves.
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.
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.
In the block editor itself, this logic runs through the REST API which already does this sort of logic.
This only impacts the render on the server-side.
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.
@sc0ttkclark That makes sense to me. I added the permissions check in f53121d and had to check the post status too to make sure it works as intended.
(Pardon the PHPCS errors — those have all been fixed in later commits)
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.
Following up on this on the line where this was implemented: https://github.com/WordPress/wordpress-develop/pull/5888/files#r1471516195
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.
Looking at this here I would love it if the function in core could already take care of looking up the post_id in the current context.
I cannot think of any instance where I wouldn't want to get the ID of the current queried post within a query loop for example. So this code that is shown here will be needed in 100% of the cases where this API is used and feels error prone. If we could make it so that the callback is already getting passed the current post id :)
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.
Could we also pass the result through a filter here, passing through the post ID, and
source_attrs['value']
.This could be useful for ACF to hydrate a raw post meta value into it's formatted value which users would expect.
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 don't think we should pass this through filters, this is its own abstractions if a third-party plugin wants its own kind of values, they can implement their own custom 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.
@fabiankaegy I'm not sure I understand the comment, can you clarify a bit?
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.
@youknowriad What I mean is that I would love it if the
wp_block_bindings_register_source
function already passed in the current post ID as a parameter of the callback function.Having to manually do the check to see if
$block_instance->context['postId']
is available and if so using it vs. accessing the id via theget_the_ID
function is something that every single block binding source will need to do if it should work correctly in all context such as inside query loops.I would like it if the callback could be simplified to:
If I'm not mistaken there even is a bug in this implementation here. Because it doesn't use the context value at all. It should instead check for the availability of the context and if so use that over
get_the_ID
. And because of that very reason I think ideally core would already handle that check for context and pass the correct post id as a parameter to the callback.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.
Personally it doesn't make sense to me. The sources can be anything, from posts to a random database table, to a call to some remote API... I don't think the signature should accommodate a specific source over any other.
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.
But don't sources always need to know which context they are rendered within?
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’m still not sold on the name source, because it implies that the value is sourced from something external. In practice, the value used for replacement could be for example :
Overall, we are opening an API to dynamically compute the value of the attribute on the server so people don't need to default to custom and dynamic blocks.
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.
That context actually is helpful! In my mind, I was really mostly concerned with dynamic attributes on the post object / external API connections that need to be aware of the current context which is why I was so focussed on the Post ID.
In that case an unrelated side note, I would love it if simply all blocks anywhere had the
usesContext: [ 'postId' ]
specified because the whole accessing the correct ID is an error-prone thing that often gets overlooked and the a feature doesn't work correctly in query loops etcCheck failure on line 25 in src/wp-includes/block-bindings/sources/post-meta.php
GitHub Actions / Check PHP compatibility