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

[ntsekouras]

Resolves: #35364
Depends on #38625

This PR enables themes to register patterns from Pattern Directory through theme.json. It checks for a patterns top level key that is an array of pattern slugs from the directory.

Testing instructions

1. Merge Allow child classes to use the private methods and constants #38625 in this branch. git merge --squash update/extend-theme-json.
2. Use empty theme and go to the patterns tab in the inserter.
3. You can see that the two provided patterns from theme.json are registered(pattern1, pattern2).

Alternatively, you can add in your theme.json some patterns in the patterns property.

Notes

• You can see from the second pattern that is under uncategorized. That is because there is no parity between pattern directory categories and core categories(specifically images category in Pattern Directory.

[carolinan]

Are the slugs already surfaced in the pattern directory? This assumes that you know the slug to place it in theme.json.

[ntsekouras]

Are the slugs already surfaced in the pattern directory? This assumes that you know the slug to place it in theme.json.

They can be extracted by the url in single pattern view at the Pattern Directory: example https://wordpress.org/patterns/pattern/short-text-surrounded-by-round-images where the slug is short-text-surrounded-by-round-images

[carolinan]

I think it needs to be more obvious.

[kjellr]

I think it needs to be more obvious.

I'm not sure it does need to be. After all, this is a technical feature geared towards developers who are generally familiar with what a slug is already. To me, using slugs feels like a pretty reasonable solution, and one that doesn't require any new functionality or UI in the patterns directory. I'm curious to hear other folks' reactions too.

[scruffian]

This feels easy enough for theme developers to use. I'm curious what a more obvious alternative would be.

[mcsf]

I think this could be a good start. As with other features that block themes enable, there are many opportunities for nicer, more integrated experiences; for instance, a UI similar to the Patterns Explorer with which a themer/user can cherry pick patterns from the directory to "attach" to the current theme.

[ryelle]

Neat! This is working well in my testing, and creating Gutenberg_REST_Pattern_Directory_Controller will help with a few other API updates that need to happen before 6.0 💯

Should these patterns be highlighted as being featured by the theme? Right now they're dropped in with the rest of the patterns, so there's no indication to the user that the theme author specifically picked out "Short text surrounded by round images," for example.

That is because there is no parity between pattern directory categories and core categories

We could change that - initially there was an assumption that categories would be loaded from the directory, we even have an endpoint stubbed out for it: https://api.wordpress.org/patterns/1.0/?categories

Are the slugs already surfaced in the pattern directory?

To me, using slugs feels like a pretty reasonable solution, and one that doesn't require any new functionality or UI in the patterns directory.

I think for now this is fine, but it would be pretty neat to have a UI like the google fonts selection while browsing wp.org/patterns, that would let you collect a few patterns & then output the patterns JSON property 🤔

[ntsekouras]

Should these patterns be highlighted as being featured by the theme?

I'm not really sure, but we can explore this in follow ups for sure. Do you think this adds some value in the form of some badge or something similar in the UI? Maybe it would be more prominent if the requested patterns were on a different category from the rest? 🤔 Would that work well though since we already have the featured patterns?

We could change that - initially there was an assumption that categories would be loaded from the directory, we even have an endpoint stubbed out for it: https://api.wordpress.org/patterns/1.0/?categories

That could work then. I can get the categories and register what's needed, but I think we should first try to find the best path forward with current and future pattern categories. Should there be parity between the directory and core? Are there are plans to add more categories in the directory?

[kjellr]

IDo you think this adds some value in the form of some badge or something similar in the UI?

I think for the first version of this, we should just ensure that these patterns show up at the top of the list, and that's fine.

[ryelle]

Do you think this adds some value in the form of some badge or something similar in the UI? Maybe it would be more prominent if the requested patterns were on a different category from the rest?

There was a little conversation about that on the original issue #35364 (comment), and I still think having a "Theme Name" category would be useful, but don't feel strongly about it.

I think we should first try to find the best path forward with current and future pattern categories. Should there be parity between the directory and core? Are there are plans to add more categories in the directory?

Yep, WordPress/pattern-directory#190 — we need to decide what they'll be, but there will be more. Eventually the source of truth for categories should be the Pattern Directory.

we can explore this in follow ups for sure.

Absolutely, none of what I mentioned was a blocker — let me know if you want me to make issues for anything 🙂

[ntsekouras]

I think for the first version of this, we should just ensure that these patterns show up at the top of the list, and that's fine.

Unfortunately there is no specific way to make this happen, because the patterns order depends on pattern registration time, which can happen in different times(actions, etc...) and we do not have any indication of that with the current API. In addition an API like that for ordering wouldn't work well in practice as it would give a way for everyone to register patterns and try to make them always appear on top.

If we need to differentiate them from the rest, I guess we should create a new pattern category.

Having said that, I think we can land this first iteration as is not something user facing and we can follow up with enhancements on category handling and explore sorting options. What do you think?

[youknowriad]

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

[ntsekouras]

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?

[youknowriad]

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 🤷

[ntsekouras]

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.

[youknowriad]

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.

[aristath]

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

[ntsekouras]

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?

[oandregal]

@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.

[ntsekouras]

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.

[oandregal]

Incorporated in #38625 the changes needed for this and pushed at f6a6dd7 and 52ca23f

[oandregal]

I've pushed some changes to the theme.json classes. This PR depends on #38625

To test this, you need to have #38625 locally and do git merge --squash update/extend-theme-json. Updated the testing instructions in the issue description as well.

[oandregal]

I've just merged #38625 so this needs a rebase to get those changes in.

[ntsekouras]

I've just merged #38625 so this needs a rebase to get those changes in.

Will do in a jiffy 😄

[oandregal]

It works as expected, the approach with slugs sounds easy enough, and the code looks fine. 🚢

[ntsekouras]

Thanks all for the feedback/reviews! I'll land and let's iterate if needed about the prominence of the added patterns (specific new category, etc...).

[youknowriad]

Thanks for your work here, should we add some documentation?

[ntsekouras]

Thanks for your work here, should we add some documentation?

Definitely. Sorry for missing this - I'll create a PR right away!

[oandregal]

Addressing #38620 would create docs automatically for this new key as well as other ones that are undocumented (customTemplates, templateParts, and any future one).

[ntsekouras]

I started adding some documentation here: https://github.com/WordPress/gutenberg/blob/trunk/docs/how-to-guides/themes/theme-json.md. That's a different thing @oandregal , no?

[oandregal]

That's more of a "how to" guide, it's aimed at presenting examples (although it needs more work to point to the reference doc, remove things, and update others).