Skip to content

[Pattern Directory]: Allow pattern registration from directory with theme.json #38323

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.

Sign up for GitHub

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

Merged
ntsekouras merged 8 commits into from
Feb 10, 2022
Merged

[Pattern Directory]: Allow pattern registration from directory with theme.json #38323

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3d68a4f
[Pattern Directory]: Allow pattern registration from directory with t…
ntsekouras Jan 28, 2022
5d795ef
fix php linting issues
ntsekouras Jan 28, 2022
6741a08
identation fix
ntsekouras Feb 3, 2022
2f1473c
review feedback
ntsekouras Feb 8, 2022
0852d94
always return an array
ntsekouras Feb 8, 2022
c435c6a
Revert changes to existing WP_Theme_JSON_Gutenberg class
oandregal Feb 8, 2022
73c0d96
Override the parts that this PR needs
oandregal Feb 8, 2022
9cca636
fix after rebase
ntsekouras Feb 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
always return an array
  • Loading branch information
@ntsekouras
ntsekouras committed Feb 9, 2022
commit 0852d94e18ec6cf4acb1c4f2f2669e49b551a3a2
3 changes: 2 additions & 1 deletion lib/compat/wordpress-6.0/class-wp-theme-json-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,12 +680,13 @@ public function get_custom_templates() {
* Returns the current theme's wanted patterns(slugs) to be
* registered from Pattern Directory.
*
* @return array|null
* @return array
*/
public function get_patterns() {
if ( isset( $this->theme_json['patterns'] ) && is_array( $this->theme_json['patterns'] ) ) {
return $this->theme_json['patterns'];
}
return array();
}
Copy link
Contributor

@youknowriad youknowriad Feb 3, 2022

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 in lib/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

Copy link
Contributor Author

@ntsekouras ntsekouras Feb 3, 2022 •
edited
Loading

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 to protected instead of private. That would help as from that point on, to handle this more gracefully.

What do you think? Am I missing something php related?

Copy link
Contributor

@youknowriad youknowriad Feb 3, 2022

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 🤷

Copy link
Contributor Author

@ntsekouras ntsekouras Feb 3, 2022

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 things protected or static for 6.0 and then we could make adjustments easier for future releases.

Copy link
Contributor

@youknowriad youknowriad Feb 4, 2022

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.

Copy link
Member

@aristath aristath Feb 4, 2022

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

Copy link
Contributor Author

@ntsekouras ntsekouras Feb 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we need to make another change later, we'll need to figure how to do it at that point.

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 some private things we will end up copying the class forever(when we have changes of course). Do you find something bad with making some things protected or are there strong reasons for keeping them private?

Copy link
Member

@oandregal oandregal Feb 8, 2022

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 to protected 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.

Copy link
Contributor Author

@ntsekouras ntsekouras Feb 8, 2022 •
edited
Loading

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 new get_patterns function would be:

  1. Change $theme_json to protected, because we need to access this from the child class
  2. Update the line that uses VALID_TOP_LEVEL_KEYS from self::VALID_TOP_LEVEL_KEYS to static::VALID_TOP_LEVEL_KEYS, as static:: is inheritance-aware.

Copy link
Member

@oandregal oandregal Feb 8, 2022

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


/**
Expand Down
2 changes: 1 addition & 1 deletion test/emptytheme/theme.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@
"wideSize": "1100px"
}
},
"patterns": ["short-text-surrounded-by-round-images", "partner-logos" ]
"patterns": [ "short-text-surrounded-by-round-images", "partner-logos" ]
}