WordPress.org

Make WordPress Core

Opened 6 weeks ago

Closed 6 weeks ago

Last modified 6 weeks ago

#48210 closed defect (bug) (fixed)

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

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

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 6 weeks ago.
Change single variables in string from $1$s to %s
48210-1.diff (12.4 KB) - added by pedromendonca 6 weeks ago.
Full patch

Download all attachments as: .zip

Change History (8)

@pedromendonca
6 weeks ago

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

#1 follow-up: @ocean90
6 weeks 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
6 weeks 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
6 weeks ago

Full patch

#3 @garrett-eclipse
6 weeks 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
6 weeks 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
6 weeks 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
6 weeks ago

Thanks @ocean90 I appreciate the explanation and commit. Cheers

Note: See TracTickets for help on using tickets.