Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#48210 closed defect (bug) (fixed)

I18n: consistent strings variables across admin and network sub-projects

Reported by: pedromendonca's profile pedromendonca Owned by: ocean90's profile ocean90
Milestone: 5.3 Priority: normal
Severity: normal Version: 5.3
Component: I18N Keywords: has-patch needs-refresh
Focuses: multisite Cc:

Description

As already exist in 'admin', there is no need to numbered variables $1$s in a single variable string. $1$s -> %s

Example already existent in 'admin' sub-project:
https://build.trac.wordpress.org/browser/trunk/wp-admin/includes/class-wp-comments-list-table.php#L248

/* translators: %s: Number of comments. */
'spam'      => _nx_noop(
        'Spam <span class="count">(%s)</span>',
        'Spam <span class="count">(%s)</span>',
        'comments'
),

/* translators: %s: Number of comments. */
'trash'     => _nx_noop(
        'Trash <span class="count">(%s)</span>',
        'Trash <span class="count">(%s)</span>',
        'comments'
),

Attachments (2)

48210.diff (1.7 KB) - added by pedromendonca 4 years ago.
Change single variables in string from $1$s to %s
48210-1.diff (12.4 KB) - added by pedromendonca 4 years ago.
Full patch

Download all attachments as: .zip

Change History (8)

@pedromendonca
4 years ago

Change single variables in string from $1$s to %s

#1 follow-up: @ocean90
4 years ago

  • Focuses multisite added
  • Keywords has-patch needs-refresh added
  • Milestone changed from Awaiting Review to 5.3
  • Type changed from enhancement to defect (bug)
  • Version set to trunk

Thanks for the report and the patch! There's still room for improvements here.

  • The labels for "All" and "Spam" should use _nx() and define "sites" as context
  • The translator comments are missing. See the example from the comments list table. Feel free to use the same formatting.

Would you like to refresh the patch based on the feedback?

This was introduced in [46254].

#2 in reply to: ↑ 1 ; follow-up: @pedromendonca
4 years ago

Thank you @ocean90,

I've added the contexts and the translator comments as suggested, plus to some other tables.
I'm adding a second version of the patch, with the complete changes.

@pedromendonca
4 years ago

Full patch

#3 @garrett-eclipse
4 years ago

Thanks for closing #37392 and pointing me here @ocean90 reviewing this patch applies cleanly.
My one question is _nx vs _nx_noop as I noticed it's a mix of the two functions throughout core for these count strings. What is the rule of thumb? Should they all be one or the other or is there circumstances that depends on?
Appreciated

#4 @ocean90
4 years ago

  • Owner set to ocean90
  • Resolution set to fixed
  • Status changed from new to closed

In 46385:

Networks and Sites: Improve newly added strings for i18n.

  • Remove unnecessary numbered placeholders.
  • Add context to "All" and "Spam" status.
  • Add translator comments.

Props pedromendonca.
Fixes #48210.

#5 in reply to: ↑ 2 @ocean90
4 years ago

Replying to pedromendonca:

I've added the contexts and the translator comments as suggested

Thanks! In [46385] I only committed the changes to the sites table. The context is only necessary if we use the same string in different contexts.

Replying to garrett-eclipse:

My one question is _nx vs _nx_noop as I noticed it's a mix of the two functions throughout core for these count strings.

_nx_noop() is a no-operation function and only used to register a string if you don't have the number yet. Once you have the number you can translate the string with translate_nooped_plural().

#6 @garrett-eclipse
4 years ago

Thanks @ocean90 I appreciate the explanation and commit. Cheers

Note: See TracTickets for help on using tickets.