Make WordPress Core

Opened 9 months ago

Closed 9 months ago

Last modified 7 months ago

#58460 closed enhancement (fixed)

Add `wp_get_remote_theme_patterns` function

Reported by: oandregal's profile oandregal Owned by: oandregal's profile oandregal
Milestone: 6.3 Priority: normal
Severity: normal Version: 6.3
Component: Themes Keywords: has-patch commit add-to-field-guide
Focuses: Cc:

Description

Part of https://github.com/WordPress/gutenberg/issues/45171

What

Add a new public function, wp_get_remote_theme_patterns to query the patterns datum from theme.json and substitutes current usage of private APIs.

Why?

We need to offer public APIs for consumers to add the data they need, so they don't resort to using private APIs.

Change History (20)

This ticket was mentioned in PR #4543 on WordPress/wordpress-develop by @oandregal.


9 months ago
#1

Trac ticket https://core.trac.wordpress.org/ticket/58460
Part of https://github.com/WordPress/gutenberg/issues/45171
Backports https://github.com/WordPress/gutenberg/pull/49307

## What?

Adds a new public function, wp_get_remote_theme_patterns to query the patterns datum from theme.json and substitutes current usage of private APIs.

## Why?

We need to offer public APIs for consumers to add the data they need, so they don't resort to using private APIs.

## How?

  • Creates a new function called wp_get_remote_theme_patterns.
  • Uses the new function in place of the call to the private class WP_Theme_JSON_Resolver.

## Testing Instructions

  • Use TwentyTwentyThree theme and add the following to its theme.json:

{{{json
{

"patterns": [ "partner-logos" ]

}
}}}

  • Go to the post editor and open the inserter.
  • Search for "logos".
  • The expected result is that the pattern shows up in the list.

Also test that by removing the patterns from theme.json the pattern is not present.

## Commit message

{{{msg

Props ntsekouras.
}}}

#2 @poena
9 months ago

  • Keywords commit added

Works well in my test, thank you.

This ticket was mentioned in Slack in #core-committers by nosolosw. View the logs.


9 months ago

#4 @dd32
9 months ago

The name wp_get_remote_theme_patterns() feels a bit close to the wp_remote_*() and wp_safe_remote_*() methods to me. The inclusion of remote when it's not actually performing a remote-request seems weird too (I get that it's to say "This returns something that needs to be fetched remotely")

I would suggest that something along the lines of wp_theme_required_patterns() or ...pattern_slugs() is maybe a little more understandable as to what it means? ie. wp_ (it's a WP function) theme_ (It's related to the themes component) required_ (It might not be required, and rather suggested?) pattern_slugs() what's being queried for.

upstreamed this here: https://github.com/WordPress/gutenberg/issues/45171#issuecomment-1596426839

Last edited 8 months ago by dd32 (previous) (diff)

#5 @oandregal
9 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 55926:

Themes: add wp_get_remote_theme_patterns function.

Adds a new public function, wp_get_remote_theme_patterns to query the patterns datum from theme.json and substitutes current usage of private APIs.

Props ntsekouras, poena, audrasjb.
Fixes #58460

#7 follow-up: @oandregal
9 months ago

@dd32 I just saw your feedback after committing, I'm so sorry about that. I presume I can do a follow-up given this is only been committed and nobody would use this until 6.3 is released.

For the record, I'm not a big fan of changing names that were already vetted in Gutenberg, given this was a backport PR. However, I'm open to do the follow-up if others feel the same.

#8 in reply to: ↑ 7 @dd32
8 months ago

Replying to oandregal:

dd32 I just saw your feedback after committing, I'm so sorry about that.

All good!

I presume I can do a follow-up given this is only been committed and nobody would use this until 6.3 is released.

Yep, totally.

For the record, I'm not a big fan of changing names that were already vetted in Gutenberg

For the record, I'm not a fan of vetting being done within Gutenberg repo's where the majority of WordPress contributors don't follow actively. However, there's a big difference between what seems "okay" in gutenberg_get_remote_.. and wp_get_remote_...

Given the function is being renamed as part of the import from Gutenberg, renaming as part of the backport hardly seems to be a concern..

I've commented upstream though, thanks!

Last edited 8 months ago by dd32 (previous) (diff)

This ticket was mentioned in PR #4640 on WordPress/wordpress-develop by @oandregal.


8 months ago
#9

Trac ticket https://core.trac.wordpress.org/ticket/58460
Follow-up to https://github.com/WordPress/wordpress-develop/pull/4543

This PR renames the recently landed wp_get_remote_patterns function to wp_get_theme_directory_pattern_slugs.

The rationale is to better provide context on what it does by avoiding the use of remote affix in the function name. Using remote may suggest the function actually does an external request, while it only provides metadata for the editor, which will take care of the request itself.

### Commit

Rename `wp_get_remote_patterns` to `wp_get_theme_directory_pattern_slugs`.

Props dd32.

Fixes #58460.

#10 @oandregal
8 months ago

@dd32 I've prepared https://github.com/WordPress/wordpress-develop/pull/4640 to rename wp_get_remote_patterns to wp_get_theme_directory_pattern_slugs.

The new name avoids using remote and is more explicit about what it does.

Probably, I won't be available until mid/end of week. If the name suggestion addresses your concerns, please, be welcome to commit yourself. Otherwise, I will when I come back.

@oandregal commented on PR #4640:


8 months ago
#12

When/If this goes through, we also need to update Gutenberg to use the same naming.

#13 @oandregal
8 months ago

In 55983:

Rename wp_get_remote_patterns to wp_get_theme_directory_pattern_slugs.

Props dd32, ntsekouras.

Fixes #58460.

@oandregal commented on PR #4640:


8 months ago
#15

Porting these changes back to Gutenberg in https://github.com/WordPress/gutenberg/pull/51784

#16 @milana_cap
8 months ago

  • Keywords add-to-field-guide added

@colorful tones commented on PR #4543:


7 months ago
#17

@oandregal I'm super curious as to what this PR is addressing. The testing notes and description leave me confused. I tried drilling through related Issues and Backport tickets, and I'm still lost.

Also, I had to Google "datum." 🤷‍♂️

I'm grateful for the work. Please explain in layperson's terms what use case or issue this PR solves.

@oandregal commented on PR #4543:


7 months ago
#18

One of the fields of theme.json is patterns. So far, there wasn't any public API to get the value of that field. This PR introduces it.

@colorful tones commented on PR #4543:


7 months ago
#19

For an everyday WordPress user - this would enhance their experience by allowing the "patterns" entries in the theme.json to be surfaced within the Inserter. Whereas this was not possible before? (Sorry, I don't have time to test it out and wanted to know if this is what the PR is directly solving.) Or is there another use case this might enhance or alleviate? 🤔

@colorful tones commented on PR #4543:


7 months ago
#20

For an everyday WordPress user - this would enhance their experience by allowing the "patterns" entries in the theme.json to be surfaced within the Inserter. Whereas this was not possible before? (Sorry, I don't have time to test it out and wanted to know if this is what the PR is directly solving.) Or is there another use case this might enhance or alleviate? 🤔

Note: See TracTickets for help on using tickets.