#28502 closed defect (bug) (fixed)
'User deleted' message needs context
Reported by: | extendwings | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 4.2 | Priority: | normal |
Severity: | normal | Version: | 3.9.1 |
Component: | I18N | Keywords: | |
Focuses: | administration, multisite | Cc: |
Description
I found that _n( 'User deleted.', '%s users deleted.', $delete_count )
(wp-admin/users.php#L372) and _e( 'User deleted.' )
(wp-admin/network/users.php#L251) is conflicted. This causes strange message box in some languages:
To fix this _e( 'User deleted.' )
should be changed to _ex( 'User deleted.', 'network' )
Attachments (5)
Change History (53)
#3
@
10 years ago
- Keywords reporter-feedback added
I could not reproduce this on a clean install of Japanese or Russian package.
- Are there any specific steps to reproduce?
- Are you using a multilanguage plugin? Does it still happen with all plugins disabled and a default theme (Twenty Fourteen or Twenty Thirteen) activated?
#4
@
10 years ago
- Keywords reporter-feedback removed
OK, I was able to reproduce in network admin with Japanese language files. Investigating.
#5
follow-up:
↓ 9
@
10 years ago
- Keywords has-patch needs-testing removed
I don't think adding a context is a correct solution here.
According to GlotPress' locales.php and gettext documentation, Japanese doesn't have a distinction between a singular and plural form.
We have quite a few instances in core where singular and plural forms are different:
$message = sprintf( _n( 'User deleted.', '%s users deleted.', $delete_count ), number_format_i18n( $delete_count ) );
This doesn't seem like a valid use of _n()
. 'User deleted.'
should be a separate string instead:
if ( $delete_count > 1 ) { $message = sprintf( _n( '%s user deleted.', '%s users deleted.', $delete_count ), number_format_i18n( $delete_count ) ); } else { $message = __( 'User deleted.' ); }
This causes problems in Russian too. We have 3 plural forms. The first form is used not just for 1, but also for 21, 31, 41, etc. 'User deleted' only makes sense for 1. For 21, 31, etc., the number should be included in the string, but it's still the first form.
I tried to address this previously in comment:12:ticket:15920 and on Polyglots, and ended up introducing a fourth plural form for Russian as a workaround. I managed to make it backwards compatible with the plural forms expression in GlotPress to import translations there (only the first three forms were imported), but still had to use Poedit to edit files and SVN to build packages.
I think the correct solution here would be to review all these instances and separate single item strings from strings with numbers.
#6
@
10 years ago
Yes, adding context is just workaround.
I think this (and other singular and plural problems) should be resolved in the process of this internationalization improvement posted by @nacin.
This ticket was mentioned in IRC in #wordpress-dev by DrewAPicture. View the logs.
10 years ago
#8
@
10 years ago
- Keywords dev-feedback added
- Milestone changed from 4.0 to Future Release
- Summary changed from Context needed to distinguish to 'User deleted' message needs context
Sounds like this is a larger issue than just this one instance. Punting.
#9
in reply to:
↑ 5
@
10 years ago
Replying to SergeyBiryukov:
We have quite a few instances in core where singular and plural forms are different:
$message = sprintf( _n( 'User deleted.', '%s users deleted.', $delete_count ), number_format_i18n( $delete_count ) );This doesn't seem like a valid use of
_n()
.
See also #meta764 for a similar issue in the plugin repository (already resolved).
#10
@
10 years ago
- Milestone changed from Future Release to 4.2
- Owner set to SergeyBiryukov
- Status changed from new to accepted
This ticket was mentioned in Slack in #core by drew. View the logs.
10 years ago
This ticket was mentioned in Slack in #core by sergeybiryukov. View the logs.
10 years ago
#14
@
10 years ago
@SergeyBiryukov: How would you like to proceed with this ticket? Should we commit the one workaround and handle the remaining issues in separate tickets, or would you rather punt and try to cover all of the cases in a future release?
28502.diff still applies.
#15
follow-up:
↓ 16
@
10 years ago
- Keywords has-patch added
28502.2.diff fixes all the invalid instances of _n()
I've found in core, including the one from ticket description.
There's also an issue with 'One thought on “%2$s”'
in bundled themes (similar to the one in theme-compat/comments.php
here), I'll create a separate ticket for that once this patch get in.
Note that the removal of context in _nx( 'Theme deleted.', ..., 'network' )
is intentional. It was added in [17233] as a workaround for a similar conflicting strings issue and should no longer be necessary after this patch.
#16
in reply to:
↑ 15
;
follow-up:
↓ 17
@
10 years ago
Replying to SergeyBiryukov:
28502.2.diff fixes all the invalid instances of
_n()
I've found in core, including the one from ticket description.
Can you please explain the change to wp-admin/edit.php? It doesn't make sense to me. :(
#17
in reply to:
↑ 16
;
follow-up:
↓ 18
@
10 years ago
Replying to ocean90:
Can you please explain the change to wp-admin/edit.php? It doesn't make sense to me. :(
"it" vs. "them". "it" doesn't make sense for 21 :)
#18
in reply to:
↑ 17
;
follow-up:
↓ 19
@
10 years ago
Replying to SergeyBiryukov:
"it" vs. "them". "it" doesn't make sense for 21 :)
OK, but why are singular and plural the same? Seems not to be the case for the others.
#19
in reply to:
↑ 18
@
10 years ago
Replying to ocean90:
OK, but why are singular and plural the same? Seems not to be the case for the others.
It shouldn't make a difference, since this form won't be displayed in English, and other languages would translate it according to their plural form rules, but I agree that consistency with other strings would be clearer. See 28502.3.diff.
This ticket was mentioned in Slack in #core by drew. View the logs.
10 years ago
#21
@
10 years ago
- Keywords commit added
28502.3.diff still applies. Moving for commit consideration.
#22
follow-up:
↓ 23
@
10 years ago
I don't like 28502.3.diff at it current state.
Example: sprintf( _n( 'Theme enabled.', '%s themes enabled.', $_GET['enabled'] ), number_format_i18n( $_GET['enabled'] ) )
This looks valid since translators can/should translate Theme enabled.
as %s theme enabled.
, if the singular string is used for other numbers.
The same is done in an example on https://www.gnu.org/software/gettext/manual/html_node/Translating-plural-forms.html#Translating-plural-forms
msgid "One file removed" msgid_plural "%d files removed" msgstr[0] "%d slika je uklonjena" msgstr[1] "%d datoteke uklonjenih" msgstr[2] "%d slika uklonjenih"
The only issue is, that GlotPress/translate.wordpress.org will give you a dismissable warning about wrongly used placeholders since the original (Theme enabled.)
doesn't include a placeholder. But this is not really a core issue, rather GlotPress.
I agree that the gnu.org example doesn't fit in all our cases, like 1 post not updated, somebody is editing it.
. Maybe it makes sense to introduce a new wrapper function for _n()
which supports $singular_one, $singular_one_or_more, $plural
.
I also want to propose a change to strings like _n( 'Yes, Delete this theme', 'Yes, Delete these themes', $themes_to_delete )
.
When ever we use _n()
we should pass the number too, even it's not used in the English string:
sprintf( __( 'Yes, Delete this theme', 'Yes, Delete these themes', $themes_to_delete ), number_format_i18n( $themes_to_delete )
. This will allow translators to change the strings based on there plural rules.
#23
in reply to:
↑ 22
;
follow-up:
↓ 24
@
10 years ago
Basically, _n()
should only be used for the same string with different numbers, not for different strings.
_n( '%d item', '%d items', $number )
is correct.
_n( 'One item', '%d items', $number )
is *kinda* correct, but there are some issues:
'One item'
is not guaranteed to be used for exactly one item, but we treat it like it is.- We could translate it to
'%d item'
, but in some cases it's just not possible:'%s post not updated, somebody is editing it'
requires a different translation in Slavic languages when used for 1 vs. 21, it can't be the same string.- It also doesn't work for
'You have specified this user for deletion:'
,'Delete Theme'
, etc.
- It's more user-friendly to see
'Theme deleted'
for non-bulk actions than'1 theme deleted'
. - It causes string conflicts, see #16130 and this ticket.
- As noted above, GlotPress gives you a warning about placeholders mismatch.
So the fix is to make sure 'One item'
is only used for exactly one item, not for 21 or 31, etc. You don't often delete exactly 21 users or update 21 posts, but when you do, you should see the correct string :)
The patch introduces this fix as a consistent pattern. I went through all the strings and patched those where the first form is different from the additional fourth form I added with Poedit years ago. This should allow me to ditch that fourth form and use GlotPress.
I just looked through all the changes again, and the only string that I wouldn't mind to remove is the one in wp-admin/network.php
(authentication keys), since we know we're dealing with values less than 10 there. I've just included it for the sake of consistency.
Overall it's not that many changed strings, ~20 or so. Each new release generally has more than a hundred.
Replying to ocean90:
Example:
sprintf( _n( 'Theme enabled.', '%s themes enabled.', $_GET['enabled'] ), number_format_i18n( $_GET['enabled'] ) )
This looks valid since translators can/should translateTheme enabled.
as%s theme enabled.
, if the singular string is used for other numbers.
I don't agree :) If you're not performing a bulk action, it's more user-friendly to see 'Theme enabled'
than '1 theme enabled'
. I'd like to keep the strings as close to the originals as possible, without redundant numbers.
I also want to propose a change to strings like
_n( 'Yes, Delete this theme', 'Yes, Delete these themes', $themes_to_delete )
.
When ever we use_n()
we should pass the number too, even it's not used in the English string:
sprintf( __( 'Yes, Delete this theme', 'Yes, Delete these themes', $themes_to_delete ), number_format_i18n( $themes_to_delete )
. This will allow translators to change the strings based on there plural rules.
Initially I thought that might help, but actually it wouldn't. It means that in our translation we would have to include a number in all the strings on that screen:
'Delete Theme'
=>'Delete %d Theme'
'This theme may be active on other sites in the network.'
=>'This %d theme may be active on other sites in the network.'
'You are about to remove the following theme:'
=>'You are about to remove the following %d theme:'
'Are you sure you wish to delete this theme?'
=>'Are you sure you wish to delete %d theme?'
'Yes, Delete this theme'
=>'Yes, Delete %d theme'
As noted above, including additional numbers like that is redundant and not user-friendly, and I'd like to avoid that. 28502.3.diff solves this in a more appropriate way.
#24
in reply to:
↑ 23
@
10 years ago
As noted above, including additional numbers like that is redundant and not user-friendly, and I'd like to avoid that. 28502.3.diff solves this in a more appropriate way.
Thanks for the explanations, just wanted to have them public too. :-)
Go ahead with 28502.3.diff.
This ticket was mentioned in Slack in #polyglots by ocean90. View the logs.
10 years ago
#28
@
10 years ago
- Keywords needs-patch added; has-patch commit removed
- Resolution fixed deleted
- Status changed from closed to reopened
We've missed trunk/src/wp-admin/network/site-themes.php@31973#L158. The needed strings are already in trunk/src/wp-admin/network/themes.php@31973#L270.
This ticket was mentioned in Slack in #polyglots by drew. View the logs.
10 years ago
#31
follow-up:
↓ 32
@
10 years ago
- Keywords i18n-change added
- Resolution fixed deleted
- Status changed from closed to reopened
Via @bjornjohansen in #polglots on Slack https://wordpress.slack.com/archives/polyglots/p1428177381000724
In 25802.4.diff fixes two string instances from Yes, Delete this theme
to Yes, delete this theme
Props to bjornjohansen please :)
#32
in reply to:
↑ 31
@
10 years ago
- Keywords needs-patch i18n-change removed
- Resolution set to fixed
- Status changed from reopened to closed
Replying to netweb:
Via @bjornjohansen in #polglots on Slack https://wordpress.slack.com/archives/polyglots/p1428177381000724
Please create a new ticket for this. The string was introduced in [17101] and we have some similar strings too:
- No, Return me to the plugin list
- Yes, Delete these files and data
- Yes, Delete these files
(https://wordpress.slack.com/archives/polyglots/p1428398252000768)
This ticket was mentioned in Slack in #polyglots by sergeybiryukov. View the logs.
10 years ago
This ticket was mentioned in Slack in #buddypress by netweb. View the logs.
10 years ago
#36
follow-up:
↓ 37
@
9 years ago
@SergeyBiryukov: I do not understand change in wp-admin/edit.php
.
Why was new string "1 post not updated, somebody is editing it." added? There was "%s post not updated, somebody is editing it." which was sufficient?
#37
in reply to:
↑ 36
@
9 years ago
Replying to pavelevap:
Why was new string "1 post not updated, somebody is editing it." added? There was "%s post not updated, somebody is editing it." which was sufficient?
See comment:17 :) Without that change, in Russian and some other languages, "somebody is editing it" would be displayed for both 1 and 21, 31, etc. The correct form would be "it" for 1 and "them" for 21, 31, etc.
#38
@
9 years ago
Sorry, got it. I did not realize possible change from "it" to "them" and looked only at number which did not make sense for me :-)
Modified version of wp-admin/network/users.php