WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 8 months ago

Last modified 8 months 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:

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

Download all attachments as: .zip

Change History (8)

@pedromendonca
8 months ago

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

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

Full patch

#3 @garrett-eclipse
8 months 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
8 months 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
8 months 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
8 months ago

Thanks @ocean90 I appreciate the explanation and commit. Cheers

Note: See TracTickets for help on using tickets.