WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 6 weeks ago

Last modified 6 weeks ago

#54517 closed defect (bug) (fixed)

The naming of some of the WP_Theme_JSON_Resolver's methods is not optimal

Reported by: antonvlasenko Owned by: antonvlasenko
Milestone: 5.9 Priority: normal
Severity: normal Version: trunk
Component: Themes Keywords: has-patch commit
Focuses: rest-api Cc:

Description

As reported by @desrosj:
I wanted to raise a few concerns I have about some of the method names and phrasing within WP_Theme_JSON_Resolver.

The get_user_*() functions are a bit misleading. In reading their names, it's reasonable to assume that they are retrieving user specific data. But that's not true. A few ocblocks also reference the "user's origin configuration", but it does not have anything to do with users in the WordPress sense.
A handful of functions refer to a custom post type. Anything built into WordPress Core is not really custom. Could these be changed to "site global styles" or "get current global styles"?

Change History (19)

This ticket was mentioned in PR #1957 on WordPress/wordpress-develop by anton-vlasenko.


2 months ago

  • Keywords has-patch added; needs-patch removed

#2 @SergeyBiryukov
2 months ago

  • Component changed from General to Themes
  • Milestone changed from Awaiting Review to 5.9

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


8 weeks ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


7 weeks ago

#5 @prbot
7 weeks ago

hellofromtonya commented on PR #1957:

@anton-vlasenko can you resolve the merge conflicts please?

#6 @prbot
7 weeks ago

hellofromtonya commented on PR #1957:

@noisysocks what do you think of the new naming?

#7 follow-up: @hellofromTonya
7 weeks ago

  • Keywords needs-refresh added
  • Type changed from enhancement to defect (bug)

Marking for refresh as there are merge conflicts in the PR.

Also, 5.9 is in feature freeze meaning no enhancements can be merged.

In this case, it's discussing the best name for a new public method in an existing API, new method introduced in 5.9. Reclassified this ticket to defect though that's not a great classification for it either.

It's okay to discuss it now as once 5.9 final is released, the name has be maintained for backwards-compatibility.

#8 @prbot
7 weeks ago

noisysocks commented on PR #1957:

@noisysocks what do you think of the new naming?

We do typically avoid referencing CPTs in other parts of Core as it's usually more of an implementation detail.

I defer to @ockham who ported most of this 😀

One consideration is that we'd have to update Gutenberg with these changes too.

#9 in reply to: ↑ 7 @SergeyBiryukov
7 weeks ago

Replying to hellofromTonya:

It's okay to discuss it now as once 5.9 final is released, the name has be maintained for backwards-compatibility.

Just noting that it looks like the block editor might have a different approach to backward compatibility than the rest of core. For example, in [52049]:

  • The WP_Theme_JSON::get_merged_preset_by_slug() and ::has_properties() methods have been completely removed.
  • The WP_Theme_JSON::get_block_styles() method was renamed.
  • The parameters of WP_Theme_JSON::get_merged_data() and ::get_stylesheet() methods have been completely changed, though _deprecated_argument() calls were added to handle the older format.
  • The WP_Theme_JSON::ALLOWED_TOP_LEVEL_KEYS, ::ALLOWED_SETTINGS, and ::ALLOWED_STYLES constants were renamed.

So it looks like it could still be an option to rename some methods at a later stage, though I agree it would be preferable to do that now if there is enough consensus.

#10 @prbot
7 weeks ago

hellofromtonya commented on PR #1957:

I defer to @ockham who ported most of this 😀

Hey @ockham, what do you think of renaming?

One consideration is that we'd have to update Gutenberg with these changes too.

@anton-vlasenko is there a matching PR in Gutenberg? Also can you fix the merge conflicts please?

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


7 weeks ago

#12 @prbot
7 weeks ago

ockham commented on PR #1957:

I defer to @ockham who ported most of this 😀

Hey @ockham, what do you think of renaming?

Hey, sorry for the late reply 😅 TBH I'm less familiar with the theme.json resolver -- that would be more @oandregal's or @jorgefilipecosta's area of expertise, so maybe they can weigh in.

(FWIW, personally I think that something like get_something_from_post makes sense, when e.g. compared to get_user_something_from_custom_post_type. The _from_post suffix is also something we do in template resolution, at least in one private function, _build_block_template_result_from_post (which has a counterpart, _build_block_template_result_from_file).)

This ticket was mentioned in Slack in #core by bernhardreiter. View the logs.


7 weeks ago

#14 @prbot
6 weeks ago

hellofromtonya commented on PR #1957:

@anton-vlasenko These changes will need to be backported to Gutenberg. Can you take care of that?

#15 @hellofromTonya
6 weeks ago

  • Keywords commit added; needs-refresh removed

PR 1957 is ready for commit.

#16 @prbot
6 weeks ago

hellofromtonya commented on PR #1957:

@anton-vlasenko are you available to resolve the merge conflicts?

#17 @hellofromTonya
6 weeks ago

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

In 52372:

Themes: Rename public static functions in WP_Theme_JSON_Resolver to remove custom_post_type references.

WordPress Core is not really custom and does not reference "custom post type" in its function naming. This commit renames 2 public static methods:

  • WP_Theme_JSON_Resolver::get_user_custom_post_type_id() renamed to WP_Theme_JSON_Resolver::get_user_global_styles_post_id().
  • WP_Theme_JSON_Resolver::get_user_data_from_custom_post_type() renamed to WP_Theme_JSON_Resolver:: get_user_data_from_wp_global_styles().

Follow-up to [52049], [52051], [52069], [52232], [52275], [52364].

Props antonvlasenko, bernhard-reiter, costdev, desrosj, hellofromTonya, noisysocks, oandregal, SergeyBiryukov.
Fixes #54517.

#19 @hellofromTonya
6 weeks ago

I opened an issue on Gutenberg https://github.com/WordPress/gutenberg/issues/37379 to also rename the methods there, i.e. to resync Gutenberg with Core.

Note: See TracTickets for help on using tickets.