WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 3 weeks ago

#37057 new enhancement

Creation of an esc_html functions for _n(), _nx(), _ex(), and number_format_i18n()

Reported by: zakkath Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: I18N Keywords: needs-patch
Focuses: Cc:

Description (last modified by dd32)

Using a lot of the translation functions generates an error using the WordPress Coding Standards under PHPCS such as:

Expected next thing to be an escaping function (see Codex for 'Data Validation'), not '_nx'

Certain functions like _x(), _e(), __() all have equivalent esc_html functions. But there are no esc_html equivalent for these other functions.

Granted one could simply wrap the statements in esc_html(), for the sake of consistency and standardization for theme developers, I feel that esc_html versions of these functions should be created.

Attachments (1)

37057.diff (1.3 KB) - added by zakkath 4 years ago.

Download all attachments as: .zip

Change History (19)

#1 @zakkath
5 years ago

There's a formating error in the original ticket, where it starts to underline is actually supposed to be __()

#2 @dd32
5 years ago

  • Description modified (diff)

This ticket was mentioned in Slack in #core by zakkath. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by zakkath. View the logs.


4 years ago

#5 follow-up: @johnbillion
4 years ago

  • Component changed from General to I18N
  • Focuses template removed
  • Keywords needs-patch dev-feedback added
  • Version 4.5.2 deleted

Related: #37191, particularly the reason it was wontfixed.

esc_html_ex() makes sense, but I don't think any of the others do because numbers are almost always passed through a formatting function such as sprintf() and therefore need later escaping.

#6 in reply to: ↑ 5 @zakkath
4 years ago

I understand that, but you actually presented the code along the lines of what I'm proposing be added in the case of an esc_html_n() function - just a simple wrapper for esc_html() or esc_attr().

Having the new functions just allows for consistency for theme developers.

#7 follow-up: @johnbillion
4 years ago

What's the use case for esc_html_n() without passing it through sprintf() and therefore needing further late escaping?

#8 in reply to: ↑ 7 @zakkath
4 years ago

Replying to johnbillion:

What's the use case for esc_html_n() without passing it through sprintf() and therefore needing further late escaping?

It's not so much of a use case thing but more along the lines of consistency for theme/plugin developers. Since WPCS requires that these things be escaped, _e() gets esc_html_e() and so forth but, using the current version of Underscores, it uses esc_html() as a wrapper around _nx() as demonstrated in [comments.php](https://github.com/Automattic/_s/blob/master/comments.php#L40).

So my proposal would be to create esc_html_nx() as that wrapper. I created a diff file with the wrapper and attached it to the ticket. The change was tested using a fresh copy of Underscores and resulted in desired output and nothing in the error log.

@zakkath
4 years ago

#9 @zakkath
4 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

This ticket was mentioned in Slack in #core by zakkath. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by johnbillion. View the logs.


4 years ago

#12 @johnjamesjacoby
2 years ago

I am +1, at least to: esc_html_ex( 'Word', 'context', 'domain' );.

It is an obvious improvement over: echo esc_html_x( 'Word', 'context', 'domain' );, while also being less confusing to any developer using modern autocomplete tools, who will wonder why _ex() exists and the escaping equivalent does not.

For that reason, I personally think having full coverage of escaped variants is ideal, and find the lack of an escaped _ex() the most problematic on a regular basis.

#13 @johnbillion
2 years ago

  • Keywords needs-patch added; dev-feedback has-patch needs-testing removed

+1 for esc_html_ex(), but I stand by my comments above about the _n() functions always needing later escaping.

#14 follow-up: @gonssal
7 weeks ago

This would be nice to have.

Simplified use case for _n(). Imagine I have an array of user names I want to list. I want the list to have a translatable heading specifying what the names are about:

esc_html_n( 'User:', 'Users:', count( $users ), 'textdomain' );

#15 @bedas
3 weeks ago

  • Type changed from feature request to enhancement

Usage case of _nx() printed:

<?php
printf(
  /* translators: 1: number of comments, 2: post title */
  _nx(
    '%1$s comment on %2$s',
    '%1$s comments on %2$s',
    $comments_number,
    'comments title',
    'twentysixteen'
  ),
  number_format_i18n( $comments_number ),
  get_the_title()
);

By my knowledge there is not escape wrapper for this and WPCS flags it.
It would be nice to have the wrapper just for consistency if anything.

#16 in reply to: ↑ 14 ; follow-up: @SergeyBiryukov
3 weeks ago

Replying to gonssal:

Simplified use case for _n(). Imagine I have an array of user names I want to list. I want the list to have a translatable heading specifying what the names are about:

esc_html_n( 'User:', 'Users:', count( $users ), 'textdomain' );

Just noting that this is not the correct usage of _n(), see a note in the plugin i18n handbook:

Note that some languages use the singular form for other numbers (e.g. 21, 31 and so on, much like ’21st’, ’31st’ in English). If you would like to special case the singular, check for it specifically:

if ( 1 === $count ) {
    printf( esc_html__( 'Last thing!', 'my-text-domain' ), $count );
} else {
    printf( esc_html( _n( '%d thing.', '%d things.', $count, 'my-text-domain' ) ), $count );
}

_n() should be used for strings that include a number, like %s user, %s users, but not for User vs. Users.

Similar issues were fixed in WordPress core a few years ago, see these changesets:

  • [31941]: Decouple strings where the singular and plural form are not just the same string with different numbers, but essentially two different strings.
  • [31951]: After [31941], use the decoupled strings from WP_Customize_Manager::register_controls() on the Menus screen.
  • [32029]: After [31941], use the decoupled strings from wp-admin/network/themes.php in wp-admin/network/site-themes.php as well.
  • [34521]: Plugins: Don't use _n() for singular/plural strings which have no placeholder for a number.
  • [35611]: About: Don't use _n_noop() for singular/plural strings which provide no placeholder for a number.

and these comments for more context:

So the correct code for the use case above would look like this:

if ( 1 === count( $users ) ) {
    esc_html_e( 'User:', 'textdomain' );
} else {
    esc_html_e( 'Users:', 'textdomain' );
}

#17 in reply to: ↑ 16 ; follow-up: @gonssal
3 weeks ago

Replying to SergeyBiryukov:

Replying to gonssal:

Simplified use case for _n(). Imagine I have an array of user names I want to list. I want the list to have a translatable heading specifying what the names are about:

esc_html_n( 'User:', 'Users:', count( $users ), 'textdomain' );

Just noting that this is not the correct usage of _n(), see a note in the plugin i18n handbook:

Note that some languages use the singular form for other numbers (e.g. 21, 31 and so on, much like ’21st’, ’31st’ in English). If you would like to special case the singular, check for it specifically:

if ( 1 === $count ) {
    printf( esc_html__( 'Last thing!', 'my-text-domain' ), $count );
} else {
    printf( esc_html( _n( '%d thing.', '%d things.', $count, 'my-text-domain' ) ), $count );
}

_n() should be used for strings that include a number, like %s user, %s users, but not for User vs. Users.

Similar issues were fixed in WordPress core a few years ago, see these changesets:

  • [31941]: Decouple strings where the singular and plural form are not just the same string with different numbers, but essentially two different strings.
  • [31951]: After [31941], use the decoupled strings from WP_Customize_Manager::register_controls() on the Menus screen.
  • [32029]: After [31941], use the decoupled strings from wp-admin/network/themes.php in wp-admin/network/site-themes.php as well.
  • [34521]: Plugins: Don't use _n() for singular/plural strings which have no placeholder for a number.
  • [35611]: About: Don't use _n_noop() for singular/plural strings which provide no placeholder for a number.

and these comments for more context:

So the correct code for the use case above would look like this:

if ( 1 === count( $users ) ) {
    esc_html_e( 'User:', 'textdomain' );
} else {
    esc_html_e( 'Users:', 'textdomain' );
}

It was just a quick example for an use case. You can modify the docs example like this:

if ( 1 === $count ) {
    printf( esc_html__( 'Last thing!', 'my-text-domain' ), $count );
} else {
    printf( esc_html_n( '%d thing.', '%d things.', $count, 'my-text-domain' ), $count );
}

One function call instead of 2, there's your use case.

#18 in reply to: ↑ 17 @johnbillion
3 weeks ago

Replying to gonssal:

if ( 1 === $count ) {
    printf( esc_html__( 'Last thing!', 'my-text-domain' ), $count );
} else {
    printf( esc_html_n( '%d thing.', '%d things.', $count, 'my-text-domain' ), $count );
}

This is still incorrect usage of this function if you want escaped and localised output. Granted WordPress core itself doesn't fully escape its output so it doesn't lead by example.

The value of $count is not escaped in this example. The output is only safe because $count is formatted with %d thus coercing it to an integer, which is not the correct format to use for a user-facing number. The number needs to support a thousands separator for correct localisation and therefore must use the %s format.

Correct usage of an _n() function always requires its output to be wrapped in an esc_*() function:

if ( 1 === $count ) {
    printf( esc_html__( 'Last thing!', 'my-text-domain' ), $count );
} else {
    echo esc_html(
        sprintf(
            _n( '%s thing.', '%s things.', $count, 'my-text-domain' ),
            number_format_i18n( $count )
        )
    );
}
Note: See TracTickets for help on using tickets.