Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#47003 closed defect (bug) (fixed)

i18n: Merge similar translation strings in new user registration screen

Reported by: ramiy's profile ramiy Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.5 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-screenshots has-patch needs-testing
Focuses: administration, multisite Cc:

Description

See the attached patch.

Attachments (8)

47003.png (181.4 KB) - added by ramiy 6 years ago.
47003.patch (664 bytes) - added by ramiy 6 years ago.
47003.2.patch (2.3 KB) - added by ramiy 6 years ago.
another 3 similar strings merged into 1 string
47003.2.png (33.1 KB) - added by ramiy 6 years ago.
3 more…
47003.3.png (42.5 KB) - added by ramiy 6 years ago.
47252.4.png (53.2 KB) - added by ramiy 6 years ago.
47003.diff (2.3 KB) - added by chetan200891 5 years ago.
Refreshed patch.
47003.4.diff (2.1 KB) - added by garrett-eclipse 5 years ago.
Refreshed patch to account for removal of Error prefixes in [48059]

Download all attachments as: .zip

Change History (29)

@ramiy
6 years ago

@ramiy
6 years ago

#1 @ramiy
6 years ago

  • Keywords has-screenshots has-patch added

@SergeyBiryukov

#2 @ramiy
6 years ago

Actually, this is a bigger issue. We have many similar error messages in several locations. We should use the same error messages in the following functions:

  • register_new_user() (wp-includes/user.php)
  • wpmu_validate_user_signup() (wp-includes/ms-functions.php)
  • check_username() (wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php)

@ramiy
6 years ago

another 3 similar strings merged into 1 string

@ramiy
6 years ago

3 more...

#3 @ramiy
6 years ago

Sorry, the last screenshot is not for this ticket.

#4 @SergeyBiryukov
6 years ago

  • Milestone changed from Awaiting Review to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#5 @ramiy
6 years ago

@SergeyBiryukov

This ticket merges the following 3 strings:

  • Usernames can only contain lowercase letters (a-z) and numbers.
  • Username contains invalid characters.
  • <strong>ERROR</strong>: This username is invalid because it uses illegal characters. Please enter a valid username.

to only one string:

  • Usernames can only contain lowercase letters (a-z) and numbers (0-9).

#6 follow-up: @garrett-eclipse
6 years ago

The only string I'd be concerned about here is the one in wp-includes/user.php as all the other error messages there have the Error: prefix, examples;

'<strong>ERROR</strong>: Please enter a username.'
'<strong>ERROR</strong>: This username is already registered. Please choose another one.'

#7 in reply to: ↑ 6 @ramiy
6 years ago

Replying to garrett-eclipse:

The only string I'd be concerned about here is the one in wp-includes/user.php as all the other error messages there have the Error: prefix, examples;

<strong>ERROR</strong>: Please enter a username.
<strong>ERROR</strong>: This username is already registered. Please choose another one.

I know, this is why I mentioned above that this is a bigger issue. I think we should remove the Error: prefix from wp-includes/user.php.

@ramiy
6 years ago

#8 follow-up: @ramiy
6 years ago

As you can see from the screenshot, update nag has no Warning prefix, it only has a yellow color. Same should be in with errors. The prefix should be removed, it has a red color.

Note that many error in WordPress dashboard don't have the error prefix. I'll a few attach screenshots.

@ramiy
6 years ago

#9 in reply to: ↑ 8 @garrett-eclipse
6 years ago

Replying to ramiy:

As you can see from the screenshot, update nag has no Warning prefix, it only has a yellow color. Same should be in with errors. The prefix should be removed, it has a red color.

Note that many error in WordPress dashboard don't have the error prefix. I'll a few attach screenshots.

Thanks @ramiy seeing it there in context I agree there's no need for the Error: prefix as it's redundant since it's already in an error notice. Not sure if that should be handled in a separate ticket or could be done here. @SergeyBiryukov thoughts?

#10 follow-up: @ramiy
6 years ago

I think that this ticket should handled only the merge of similar translation strings. A separate ticket should remove the Error: prefix from the error messages in the dashboard.

#11 in reply to: ↑ 10 @garrett-eclipse
6 years ago

Replying to ramiy:

I think that this ticket should handled only the merge of similar translation strings. A separate ticket should remove the Error: prefix from the error messages in the dashboard.

I agree with you there @ramiy and created #47656 to accommodate it. The two will conflict so one will need a refresh in the end.

#12 @garrett-eclipse
5 years ago

  • Keywords needs-refresh added
  • Milestone changed from 5.3 to 5.4

@SergeyBiryukov made some good points and a proposal in #47656;

Previous discussion: #15887

Specifically, I think comment:5:ticket:15887 sums up the purpose of the ERROR: prefix nicely:

All CAPITAL letters in a word typically denotes Emphasis. It's not meant to be

"Good day gentle, but incorrect, user - there seems to be an error. Be a sport and type in your e-mail address"

But rather

"HEY, USER! (yelling) THERE'S A PROBLEM! ATTENTION! ERROR. Type in your email address please"

We could, however, move the ERROR: prefix out of the strings and add it separately, see the error message in wp-admin/themes.php for example. Related: #38860

I concur and believe isolating the prefix is our patch forward here. @ramiy would you like to handle the refresh?

As it will need a refresh and would be a string change I would feel more comfortable tackling this in 5.4 as we're already in beta2 for 5.3, as well that allows us to coincide #47656 as I'm marking it for 5.4 as well.

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


5 years ago

@chetan200891
5 years ago

Refreshed patch.

#14 @chetan200891
5 years ago

  • Keywords needs-refresh removed

@SergeyBiryukov I have refreshed patch.

#15 follow-up: @SergeyBiryukov
5 years ago

  • Milestone changed from 5.4 to 5.5

Looks like this merges a Multisite-specific username limitation string with single site strings, which might not be accurate. See #17904 for more details.

This needs a closer review, moving to 5.5 for now.

#16 @afercia
5 years ago

In 48059:

I18N: Remove the "Error:" prefix from error messages.

For a number of years, most of the WordPress error messages have been prefixed with "Error:". However, these messages appear in a context where it's already clear an error occurred. Whether it's an error, a warning, or any other classification, that's not so relevant for users. The content of the message is the relevant part. The "Error:" prefix doesn't add great value while it does add unnecessary complexity for the message readability.

Also, revises some of these messages to improve clarity and removes HTML from translatable strings.

Props garrett-eclipse, ramiy, SergeyBiryukov, afercia, sabernhardt, quadthemes, audrasjb.
See #47003, #43037, #42945, #15887.
Fixes #47656.

@garrett-eclipse
5 years ago

Refreshed patch to account for removal of Error prefixes in [48059]

#17 in reply to: ↑ 15 ; follow-ups: @garrett-eclipse
5 years ago

  • Focuses multisite added
  • Keywords needs-testing added

I've refreshed the patch to account for [48059] committed recently.

Replying to SergeyBiryukov:

Looks like this merges a Multisite-specific username limitation string with single site strings, which might not be accurate. See #17904 for more details.

This needs a closer review, moving to 5.5 for now.

Reviewing the ticket it's currently still open and what's in core seems to be the regex for /[^a-z0-9]/. @SergeyBiryukov should we move forward with these consistent strings for now and flag on the multisite ticket that once those changes are in place the error message should be updated to reflect that?

#18 in reply to: ↑ 17 @ramiy
5 years ago

Replying to garrett-eclipse:

I've refreshed the patch.

Thank you Garrett!

@SergeyBiryukov can you commit this patch?

#19 in reply to: ↑ 17 @SergeyBiryukov
5 years ago

Replying to garrett-eclipse:

Replying to SergeyBiryukov:

Looks like this merges a Multisite-specific username limitation string with single site strings, which might not be accurate. See #17904 for more details.

Reviewing the ticket it's currently still open and what's in core seems to be the regex for /[^a-z0-9]/. Should we move forward with these consistent strings for now and flag on the multisite ticket that once those changes are in place the error message should be updated to reflect that?

I meant that this particular string only applies to Multisite (as is):

Usernames can only contain lowercase letters (a-z) and numbers (0-9).

Usernames on single site don't have the same restriction. As seen in sanitize_user():

// If strict, reduce to ASCII for max portability.
if ( $strict ) {
	$username = preg_replace( '|[^a-z0-9 _.\-@]|i', '', $username );
}

This allows for uppercase characters, spaces, underscores, dots, hyphens, and the @ sign.

So the patch is inaccurate as is, we cannot merge all three strings. We should be able to merge at least two, though.

#20 @SergeyBiryukov
5 years ago

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

In 48195:

Users: Merge two similar error messages about usernames with invalid characters.

Props ramiy, garrett-eclipse, chetan200891.
Fixes #47003.

#21 @SergeyBiryukov
5 years ago

In 48196:

Tests: Adjust the test for invalid username in WP_Test_REST_Users_Controller to match the new string.

Follow-up to [48195].

See #47003.

Note: See TracTickets for help on using tickets.