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
Prototype: Server-side block attributes sourcing #18414
Prototype: Server-side block attributes sourcing #18414
Changes from all commits
71ea659
c1e7d7e
c7cded6
aafc34d
1e359a8
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.
I don't think we have
block.json
for all core blocks, those which are dynamic don't have this metadata file provided because we still didn't resolve the following issues:supports
is implemented only on the client-sideThose aren't blockers for this proposal if we were to use only
attributes
though. So maybe it would be a good idea to move attributes to theblock.json
file to better promote this format.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.
You'll have to forgive me since it's been a while that I've revisited some of those specific details of the JSON manifest. Working through this prototype forced me to consider how we would implement at least some of those supports on the server (
className
,align
,anchor
are implemented in this pull request).For translations, I seem to recall something about how we considered to wrap the translateable fields via
__
et. al., automatically? I'm not sure exactly how we determine the domain in that case.As a prototype, I'm also fine to start splitting those off into their own individual tasks. It was at least interesting to explore the feasibility of pulling them in and highlighting some of these shortcomings (notably
supports
).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.
Nice, I missed that, sorry about it :(
There needs to be the
textDomain
field declared in theblock.json
file. You can check my prototype for JS side as a reference: #16088.I don't think it is a concern though in the context of
attributes
. I just wanted to raise awareness of that. The general agreement was thatattributes
shouldn't be translatable.Yes,
supports
seems like the only place which can cause issues for the proposed code.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.
Should some level of validation be happening here?
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 you mean that it has necessary properties to consider it a valid block manifest?
There are some simple checks below, both to account that the file could be parsed as JSON, and that it has a
name
. We could expand on this.gutenberg/lib/blocks.php
Line 41 in 1e359a8
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.
Validation of types, like it string, int, array etc?
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 understand you correctly, we probably should want something like what exists in
WP_Block_Type#prepare_attributes_for_render
, though I expect this would be applied at the time that$attributes
are being sourced.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.
Unit tests?
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 call is going to be expensive from a compute level. Is there anyway we can cache the result?
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'd thought a bit about what might make sense to cache. Since the HTML of each block will likely be unique, I don't know that we would want to cache either the loaded HTML or the queried results, as the cache hit ratio would be very low.
What might make sense, depending on whether it makes a measurable difference:
$document
itself.DOMDocument
is expensive (I don't know that it is)p
selector).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.
How about cache the attributes, in say post meta?
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.
Hm, I'd have to think about it more, but that does seem like a good idea. In fact, it might then make sense to run this sourcing logic when a post is saved, rather than at parse-time (the parse would just read the cached result).
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.
It seems like this class has some requirements. Can we confirm that the libxml package is currently a required one for WP Core?
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 the page you link, it says "libxml is enabled by default".
We could still have some graceful fallback here for environments where it's explicitly disabled, although it would be unable to populate
$attributes
, yes.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.
We may need to change the requires for WP core.
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.
when I have worked with
DOMDocument
I found it extremely helpful to extract the setup code into a separate function that abstracts away the noisy quirks.it's a small thing to abstract but I find in my experience it worth it especially as we learn about the settings we need to activate with whitespace and with parse-handling.
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 seems like a reasonable revision, for sure! I expect it would also make writing the tests a little nicer, since I'd not need to lump all the error cases for this function otherwise intended specifically at sourcing values.