Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong usage of _n() in WP 5.9 beta 3 translatable strings #37471

Closed
audrasjb opened this issue Dec 17, 2021 · 6 comments · Fixed by #40292
Closed

Wrong usage of _n() in WP 5.9 beta 3 translatable strings #37471

audrasjb opened this issue Dec 17, 2021 · 6 comments · Fixed by #40292
Assignees
Labels
Internationalization (i18n) Issues or PRs related to internationalization efforts [Package] Editor /packages/editor [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@audrasjb
Copy link
Contributor

audrasjb commented Dec 17, 2021

Description

This issue is copy pasted from https://core.trac.wordpress.org/ticket/54638.
Thanks @tobifjellner for the report.


The new code for WordPress contains three instances where the "Template Part"/"Template Parts" string pair is used.
However, this is usage of _n() is incorrect.

If you want to handle just the singular and plural terms, then this should be handled in the code (if 1 == n then ...) and not by using _n()
Alternatively, if these strings are used together with the relevant number, then a placeholder for the count variable should be present in the string (i.e. printf)
Three references (to the current as-built package)
https://build.trac.wordpress.org/browser/trunk/wp-includes/js/dist/editor.js?marks=5220#L5220
const entityLabel = name === 'wp_template_part' ? Object(external_wp_i18n__n?)('Template Part', 'Template Parts', list.length) : entity.label; Set description based on type of entity.

https://build.trac.wordpress.org/browser/trunk/wp-includes/js/dist/block-library.js?marks=38830#L38830
https://build.trac.wordpress.org/browser/trunk/wp-includes/js/dist/editor.js?marks=5220#L5220

Step-by-step reproduction instructions

const entityLabel =
name === 'wp_template_part'
? _n( 'Template Part', 'Template Parts', list.length )
: entity.label;

Screenshots, screen recording, code snippet

No response

Environment info

  • WordPress 5.9 beta 3
  • Gutenberg not installed

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@ocean90 ocean90 added [Package] Editor /packages/editor [Type] Bug An existing feature does not function as intended Internationalization (i18n) Issues or PRs related to internationalization efforts labels Dec 17, 2021
@ocean90 ocean90 added this to 📥 To do in WordPress 5.9 Must-Haves via automation Dec 17, 2021
@ocean90
Copy link
Member

ocean90 commented Dec 17, 2021

The same applies to

function getEntityDescription( entity, length ) {
switch ( entity ) {
case 'site':
return _n(
'This change will affect your whole site.',
'These changes will affect your whole site.',
length
);
case 'wp_template':
return _n(
'This change will affect pages and posts that use this template.',
'These changes will affect pages and posts that use these templates.',
length
);
case 'page':
case 'post':
return __( 'The following content has been modified.' );
}
}
.

@aristath
Copy link
Member

aristath commented Dec 20, 2021

Before making any changes to this behavior, please keep in mind that not all languages have the same concept of singular/plural as English. For example, there are languages where the plural for 2 is different than the plural for 3 and so on.

@Mamaduka
Copy link
Member

Mamaduka commented Jan 5, 2022

I'm removing this issue from WP 5.9 project board since RC1 was released yesterday.

Let's try and land a fix for this in a minor release.

@Mamaduka Mamaduka removed this from 📥 To do in WordPress 5.9 Must-Haves Jan 5, 2022
@Mamaduka Mamaduka added this to 📥 To do in WordPress 5.9.x via automation Jan 19, 2022
@Mamaduka Mamaduka removed this from 📥 To do in WordPress 5.9.x Apr 12, 2022
@Mamaduka
Copy link
Member

Mamaduka commented Apr 12, 2022

Maybe we can drop the label and description variations altogether. Instead, use singular form for labels and plural for descriptions. All other labels are in this form, making it consistent.

@ockham, what do you think?

@ockham
Copy link
Contributor

ockham commented Apr 12, 2022

Hey @Mamaduka, thanks for the ping!

@aristath Your point about differentiating between plural forms is very valid and the reason why I used _n() in the first place. However, since we're dealing with "indefinite" plurals here (without a number), we might not have to worry about those. (E.g. I think pretty much all languages have a unique translation of These changes or these templates, where it's unknown if it's 2, 3, or x.) So we can probably use the if ( 1 === n ) { ... } else { ... } logic here.

That said, I was a bit surprised here since I believed that _n() was the right way of providing singular/plural versions of a string, even if it didn't include the number itself. I don't see that documented in the docs for the JS version of _n() -- is that something we need to add? I'd also be curious to learn why this is discouraged 🙂

@Mamaduka That could be an alternative; however, IIRC, I added the different versions for singular and plural in response to a designer's feedback that suggested doing so in order to make the copy correspond better to the number of changes (i.e. checkboxes) the user sees:

image

@Mamaduka
Copy link
Member

Thanks for the feedback, @ockham.

I think ( 1 === n ) is a simple enough change for this case so that we can go with it. I will follow up with PR later today or tomorrow morning.

Re _n usage, I was only able to find this comment explaining the issue - https://developer.wordpress.org/reference/functions/_n/#comment-2397.

@Mamaduka Mamaduka self-assigned this Apr 12, 2022
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internationalization (i18n) Issues or PRs related to internationalization efforts [Package] Editor /packages/editor [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants