Text length should be localizable

[miyauchi]

There are variables to define the text length and they are used in the function wp_trim_words().
And also, there are flag to switch word count type in .po file.

They should be localizable because the CJK (Chinese, Japanese, Korean) has double-width characters like Japanese Kanji and they are counted by chars.

• The text length of the excerpt.
• The text length of the excerpt in RSS feed.
• The text length of comment in dashboard.
• The text length of draft in dashboard.

My plan is that adds a number of text length into the .po to be able to localize.

These fixes are included in the WP Multibyte Patch plugin in the Japanese package.
We will open some other tickets to remove this plugin from Japanese package.

[miyauchi]

All of tickets that I want to open to remove WP Multibyte Patch plugin from Japanese package are listed on following issues.

​https://github.com/miya0001/wpdev/issues?q=is%3Aissue+is%3Aopen+label%3Awp-multibyte-patch

[miyauchi]

I ran CI for this patch. ​https://github.com/miya0001/wpdev/pull/6

[birgire]

Hi @miyauchi

I applied the patch on trunk and ran:

grunt watch --phpunit --group=l10n

but got

FAILURES!
Tests: 101, Assertions: 247, Failures: 12.

I see that travis runs this successfully, so it looks like I'm missing something.

ps: The @ticket 44541 annotation is missing on the test methods.

[netweb]

Thanks @miyauchi moving this to 5.0 to coincide with the existing related tickets

Related: ​#meta3163, #43829, #44296

[miyauchi]

Addde annotation

[miyauchi]

Hi @birgire

I added annotation. :)

I ran same command on my mac and it looks good to me.

$ grunt watch --phpunit --group=l10n

...
...
...

...............................................................  63 / 101 ( 62%)
......................................                          101 / 101 (100%)

Time: 2.82 seconds, Memory: 102.25MB

OK (101 tests, 247 assertions)

Thanks :)

[birgire]

Thanks @miyauchi, it's most likely something regarding my install :)

So instead I just skimmed through the tests and here are few suggestions:

We should consider calling restore_previous_locale() before any $this->assert*, otherwise it will not restore the previous locale on failure, affecting other tests as well.

I noticed the

require_once dirname( dirname( dirname( dirname( __FILE__ ) ) ) ) . '/src/wp-admin/includes/dashboard.php';

Shouldn't it refer to the build directory?

What about:

require_once ABSPATH . 'wp-admin/includes/dashboard.php';

instead?

For these assertions:

$this->assertTrue( !! strpos( $result, $expect ) );

isn't there the possibility of a "false negative" when the string position is 0 ?

What about $this->assertContains() or $this->expectOutputRegex() instead?.

There are some unused variables, like:

$post = $this->factory()->post->create( $args );

what about:

$this->factory()->post->create( $args );

instead to simplify?

Could we define the long version of the "Lorem Ipsum" text to reuse it?

Cheers

[miyauchi]

fixed: https://core.trac.wordpress.org/ticket/44541#comment:6

[miyauchi]

Hi @birgire

Thanks! I fixed as you recommended. :)

[miyauchi]

I created a list of tickets related to fix multibyte problems.

​https://docs.google.com/spreadsheets/d/13oGbc7AqEN6OUvmze-JDCKXDdruFqsxqSAdI7-b6Lho/edit#gid=0

[birgire]

Replying to miyauchi:

Hi @birgire

Thanks! I fixed as you recommended. :)

@miyauchi that's great, thanks, just two more suggestions I have for now :)

1) Make sure the previous locale is restored in case of a test failure. So instead of:

/**
 * @ticket 44541
 */
function test_trims_to_20_counted_by_chars() {
	switch_to_locale( 'ja_JP' );
	$expected = substr( $this->long_text, 0, 20 ) . '…';
	$this->assertEquals( $expected, wp_trim_words( $this->long_text, 20 ) );
	restore_previous_locale();
}

we could consider moving restore_previous_locale() above the assertion:

/**
 * @ticket 44541
 */
function test_trims_to_20_counted_by_chars() {
	switch_to_locale( 'ja_JP' );
	$expected = substr( $this->long_text, 0, 20 ) . '…';
        $actual = wp_trim_words( $this->long_text, 20 );
	restore_previous_locale();
	$this->assertEquals( $expected, $actual );
}

Similar in these methods:

• Tests_Formatting_WPTrimWords::test_trims_to_20_counted_by_chars_with_double_width_chars()
• Tests_L10n::test_length_of_comment_excerpt_should_be_counted_by_words()
• Tests_L10n::test_length_of_comment_excerpt_should_be_counted_by_chars()
• Tests_L10n::test_length_of_comment_excerpt_should_be_counted_by_chars_in_Japanese()

2) In tests/phpunit/data/languages/ja_JP.po I noticed the src/ reference:

#. translators: This sets the text length for the comment excerpt.
#: src/wp-includes/comment-template.ph:599
msgctxt "comment_excerpt_length"
msgid "20"
msgstr "40"

#. translators: This sets the text length for the comment excerpt.
#: src/wp-admin/includes/dashboard.php:591
msgctxt "draft_length"
msgid "10"
msgstr "40"

The other cases seems to skip it, e.g.:

#. translators: This sets the text length for the excerpt.
#: wp-includes/formatting.php:3640
msgctxt "excerpt_length"
msgid "55"
msgstr "110"

[miyauchi]

fixed: https://core.trac.wordpress.org/ticket/44541#comment:9

[miyauchi]

@birgire

Make sure the previous locale is restored in case of a test failure. So instead of:

Oh... Yes! Good catch!

Thanks!

[miyauchi]

Related: https://core.trac.wordpress.org/ticket/44548

[birgire]

The reason why the earlier tests didn't pass for me, was that I missed the updated ja_JP.mo binary file :)

I tested with:

​https://github.com/miya0001/wpdev/blob/2415c793e52740b4e68b954703797a9d4f762069/tests/phpunit/data/languages/ja_JP.mo

and the tests now pass as expected.

I attached it to the ticket.

The patch in 44541.5.diff​

• Makes sure the locale is also restored before the assertions in Tests_Formatting_WPTrimWords methods.
• Makes adjustments according to WPCS.
• Adds a docblock for the private $long_text property, in both test files.

[birgire]

I wonder if the comment search excerpt and post title, in wp-admin/edit-comments.php, should be handled here too?

Here are the those two cases:

wp_html_excerpt( _draft_or_post_title( $post_id ), 50, '…' )

https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-admin/edit-comments.php#L207

wp_html_excerpt( esc_html( wp_unslash( $_REQUEST['s'] ) ), 50, '…' )

https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-admin/edit-comments.php#L220

The current patch also suggests using wp_trim_words() within get_comment_excerpt().

Would the context be needed for the wp_trim_words filter, i.e. post vs comment?

https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/formatting.php#L3334

[pento]

Patch needs review and decision.

[desrosj]

This still needs review and a decision.

[kirasong]

I was just asked about this at WordCamp Haneda.

I see it's been punted a couple of times -- who would be the best person to review?

[johnbillion]

This looks good to me. In 44541.diff​ I tweaked the translator comments so they better reflect the purpose of the value.

I'm unable to get the tests to pass locally even with a generated .mo file in place. Investigating.

[johnbillion]

In 45505:

I18N: Allow the length of automatically generated excerpts to be localized.

This introduces three new strings that can be used to control the maximum length of automatically generated excerpts for posts, comments, and draft post previews in the dashboard. Optionally combined with the existing word count type control this allows languages which include many multibyte characters to specify more appropriate maximum excerpt lengths.

Props miyauchi, birgire, johnbillion

Fixes #44541

[joyously]

I have a concern about the excerpt length. When the theme wants to provide a Customizer option to change the excerpt length, the default value should be shown, but the theme has no way to know what the default value is. It can only use the old default value of 55 and so that defeats the purpose of translating the core value.

@johnbillion

[johnbillion]

It's quite difficult to ensure that the default for any filterable value is accurate even when it's hard-coded. For example the previously hard-coded default of 55 is only the default as long as there isn't also a plugin on the site that's filtering that value. In that case, the Customizer option could be showing an incorrect default if it's hard-coded to 55.

The best option is to manually call _x( '55', 'excerpt_length' ) in your theme (with an appropriate PHPCS exclusion above it if necessary) and use that as the default, but remember that even then your default may be incorrect if another plugin is also unconditionally filtering the value.

In addition, I would question whether the user needs to be aware of the default value if the theme provides an option to change this. Presumably the option is shown so the user can have fine grained control over excerpt lengths according to their layout, in which case a default value is of little concern. Off the top of my head I don't recall any setting in WordPress that shows the user the "default" for a configurable value.