#47003 closed defect (bug) (fixed)
i18n: Merge similar translation strings in new user registration screen
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (29)
#2
@
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)
#4
@
6 years ago
- Milestone changed from Awaiting Review to 5.3
- Owner set to SergeyBiryukov
- Status changed from new to reviewing
#5
@
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:
↓ 7
@
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
@
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 theError:
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
.
#8
follow-up:
↓ 9
@
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.
#9
in reply to:
↑ 8
@
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:
↓ 11
@
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
@
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
@
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
#15
follow-up:
↓ 17
@
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.
#17
in reply to:
↑ 15
;
follow-ups:
↓ 18
↓ 19
@
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
@
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
@
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.
@SergeyBiryukov