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
[Pattern Directory]: Allow pattern registration from directory with theme.json #38323
[Pattern Directory]: Allow pattern registration from directory with theme.json #38323
Changes from all commits
3d68a4f
5d795ef
6741a08
2f1473c
0852d94
c435c6a
73c0d96
9cca636
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.
Instead of copying the entire file to lib/compat/wordpress-6.0, should we instead keep the original file under
lib/compact/wordpress-5.9
but rename the class to match core classname and only declare it if we're on 5.8 (the class doesn't exist) and inlib/compat/wordpress-6.0
extend that class (like you did above for the rest controller and I believe I did that on another controller).The advantage of this is that we know exactly what's on 5.9 and what was added on 6.0
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 in many cases including this one, this is quite hard due to private functions and properties. In this case for making it work we need to override the
VALID_TOP_LEVEL_KEYS
const in child class, but this const is referenced inside private functions in the parent class, so it uses the existing old value from the parent.I then tried to move more functions to the new child class but we have so many private functions/properties there used, that we would need to move so much code there, changing the names(prefix
gutenberg_
) in order to be called by the child class without being private, but the content would be the same.I think we cannot avoid this easily - at least from what I could see, and maybe we should copy the whole class as is here in
6.0
and in followups check what we should change toprotected
instead ofprivate
. That would help as from that point on, to handle this more gracefully.What do you think? Am I missing something php related?
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.
Seems like the problem here is the change in
VALID_TOP_LEVEL_KEYS
right? Would be good if we could switch at some point to "schema" based validation and avoid the adhoc code. That said, it's subject for another time.For now, I guess we don't have any other option than copying everything 🤷
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 this case
VALID_TOP_LEVEL_KEYS
is affected, but in the future we will probably need to make more changes in other private functions. That goes to other classes as well, so that's why I'm suggesting making some thingsprotected
orstatic
for6.0
and then we could make adjustments easier for future releases.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, I think each PR should add its own needed changes, if we can land this without copying everything, I'd do that. If we need to make another change later, we'll need to figure how to do it at that 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.
What if we add a couple of filters to the JSON parser in WP-Core for 5.9.1?
That would unblock this PR - as well as some others that currently have no way to hook in there
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's the thing though, the way I see it it as I explained above, we cannot avoid copying the
class
. But if we do not change someprivate
things we will end up copying the class forever(when we have changes of course). Do you find something bad with making some thingsprotected
or are there strong reasons for keeping themprivate
?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 @ntsekouras I haven't yet dug into an alternative way to extend this. I'll do it later.
I wanted you to be aware that in https://github.com/WordPress/gutenberg/pull/37140/files#r801650187 we'll need to change from
private
toprotected
some things in the resolver. I've prepared #38625 that could be part of 5.9.1.Perhaps we can do similar modifications to
WP_Theme_JSON
so this doesn't require copying the entire class.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 required changes in core for this to work with a child class that changes
VALID_TOP_LEVEL_KEYS
and just adds the newget_patterns
function would be:protected
, because we need to access this from the child classVALID_TOP_LEVEL_KEYS
fromself::VALID_TOP_LEVEL_KEYS
tostatic::VALID_TOP_LEVEL_KEYS
, asstatic::
is inheritance-aware.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.
Incorporated in #38625 the changes needed for this and pushed at f6a6dd7 and 52ca23f
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.
Thanks for doing this!
I thought the reference doc for theme.json was updated upon the schema changes. Apparently, it doesn't. I've now realized that it only contains data coming from the settings & styles keys, but not from
customTemplates
,templateParts
, or this new key.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.
#38620