#44541 closed enhancement (fixed)
Text length should be localizable
Reported by: | miyauchi | Owned by: | johnbillion |
---|---|---|---|
Milestone: | 5.3 | Priority: | normal |
Severity: | normal | Version: | |
Component: | I18N | Keywords: | has-patch has-unit-tests |
Focuses: | Cc: |
Description
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.
Attachments (7)
Change History (29)
#2
@
6 years ago
I ran CI for this patch. https://github.com/miya0001/wpdev/pull/6
#3
@
6 years ago
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.
#4
@
6 years ago
- Milestone changed from Awaiting Review to 5.0
Thanks @miyauchi moving this to 5.0 to coincide with the existing related tickets
Related: #meta3163, #43829, #44296
#5
@
6 years ago
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 :)
#6
@
6 years ago
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
#8
@
6 years ago
I created a list of tickets related to fix multibyte problems.
https://docs.google.com/spreadsheets/d/13oGbc7AqEN6OUvmze-JDCKXDdruFqsxqSAdI7-b6Lho/edit#gid=0
#9
in reply to:
↑ 7
@
6 years ago
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"
#10
@
6 years ago
@birgire
Make sure the previous locale is restored in case of a test failure. So instead of:
Oh... Yes! Good catch!
Thanks!
#12
@
6 years ago
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:
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.
#13
@
6 years ago
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
#17
@
6 years ago
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?
#19
@
6 years ago
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.
#21
@
6 years ago
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
#22
@
6 years ago
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.
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