Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#54518 closed defect (bug) (fixed)

There could be an issue with setting the post_title to "Custom Styles"

Reported by: antonvlasenko's profile antonvlasenko Owned by: audrasjb's profile audrasjb
Milestone: 5.9 Priority: normal
Severity: normal Version: 5.9
Component: Editor Keywords: has-patch commit
Focuses: rest-api Cc:

Description

As reported by @desrosj here:
Another thing I noticed is that the default post_title for a global styles post is set as __( 'Custom Styles' ).

I believe that this will result in the post title being set to Custom Styles translated into the selected locale of the current user, which could be problematic. If user B has selected German, but the site default is English, it's possible that the title could be unreadable for a large number of users on the site.

I'm not really sure what the best approach is for that.

Change History (15)

#1 @antonvlasenko
3 years ago

  • Keywords reporter-feedback added

@desrosj I'm not sure why we need to care about the post_title. As far as I understand this post is not visible to users.
It's only visible in the database.
https://cldup.com/8_4gP63Cu3.png

Last edited 3 years ago by antonvlasenko (previous) (diff)

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


3 years ago
#2

  • Keywords has-patch added; needs-patch removed

#3 @audrasjb
3 years ago

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

Thanks for pointing this out and for the PR @antonvlasenko. Moving for 5.9 consideration.

#4 @audrasjb
3 years ago

  • Keywords needs-refresh added; reporter-feedback removed

I agree the best we can do is to keep the original Custom Styles string hardcoded.
Moreover, the title of this specific post if never shown to the end user, so I think it's fine to keep it in English. Plus, it would be easier to identify when searching in the database.

The only thing that I would change in the proposed PR is to use /* */ instead of // for the multi-line comment, as per WordPress Coding Standards.
I'll update the PR accordingly.

This ticket was mentioned in PR #1965 on WordPress/wordpress-develop by audrasjb.


3 years ago
#5

  • Keywords needs-refresh removed

#6 @audrasjb
3 years ago

In the above PR, I entirely removed the inline comment.
I think it's good to go as it is now :)

#7 @audrasjb
3 years ago

  • Owner changed from antonvlasenko to audrasjb

Self assigning for commit consideration.

#8 @desrosj
3 years ago

I went back and looked at how the Hello World! post is handled. It looks like it does wrap the string in __() (source code).

However, since this is during installation, the string will be translated to the default language selected during installation. The same is true for the post_name for the post (not relevant for global styles), and the first comment.

If global style posts are not visible through a UI by default, I'd argue that using the site's default language is the desired behavior. This would more closely match the established practices. But at a minimum, the __() wrapper should remain, and the default text domain should be removed.

Last edited 3 years ago by desrosj (previous) (diff)

#9 @audrasjb
3 years ago

@desrosj I don't think we can compare the "Hello world" default post with the "Custom Styles" posts, since the first one is public and the second ones are never printed anywhere. You'll need to fetch the database to see them, or to run a query using PHP and searching for wp_global_styles post type.

And that's also the reason why I think it's better to keep it in English: it would be easier to retrieve those posts. As a matter of fact, there can be several "Custom Styles" posts (one per activated block theme) in the database, so it's worth having something to quickly recognize them on any WordPress installation.

Concerning consistency, worth noting that the wp_template_part and wp_template post titles (which are registered by block themes likes Twenty Twenty-Two) are not translatable either.

In any case, the default text domain should be indeed be removed, at the very least :)

#10 @audrasjb
3 years ago

So to sum-up, today is Beta 1 and we have two options for this ticket:

  1. remove the default text domain and punt the rest for later
  2. hardcode the 'Custom Styles' post title

My preferred option is (2), but we need at least to remove the default text-domain today before Beta 1. Let's wait a few hours for other thoughts concerning the second options before committing (1) 😌

#11 @hellofromTonya
3 years ago

  • Keywords commit added

As @audrasjb and @antonvlasenko have pointed out, the post title being hardcoded is (a) consistent with wp_template_part and wp_template and (b) is not publicly visible, but rather exists in the database. I think @audrasjb PR is the right way to go.

Marking PR 1965 for commit.

#12 @audrasjb
3 years ago

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

In 52280:

Editor: Do not translate the title of "Custom Styles" specific posts.

This change removes the internationalization of the "Custom Styles" specific posts as the post title being hardcoded is consistent with wp_template_part and wp_template post types, and is not publicly visible, except in the database. Moreover, using consistent "Custom Styles" post title may make is easier to retrieve the related posts in the database.

Props antonvlasenko, audrasjb, desrosj, hellofromTonya.
Fixes #54518.

#13 @audrasjb
3 years ago

In 52282:

Tests: Update WP_REST_Global_Styles_Controller_Test "Custom Styles" string after [52280].

Follow-up to [52280].

See #54518.

Note: See TracTickets for help on using tickets.