Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#45488 closed defect (bug) (worksforme)

wp_set_script_translations does not load md5-based PO files

Reported by: ettoredn's profile ettoredn Owned by:
Milestone: Priority: normal
Severity: normal Version: 5.0
Component: I18N Keywords: has-patch needs-testing needs-unit-tests close
Focuses: Cc:

Description

Given js/myscript.js used in a WordPress theme:

const { __, _x, _n, _nx } = wp.i18n;
console.debug(__('Translatable string', 'tctextdomain'));

use the following steps to reproduce the issue:

  1. register a script
    <?php
    wp_register_script('myscript', get_theme_file_path('js/myscript.js'));
    
  1. generate POT and PO files via wp-cli, in particular let wp-cli i18n make-json generate a PO file having a md5-based filename e.g. ;
  1. in my case the generated JSON file with the localized strings extracted from the PO is to be found in <my theme>/languages/it_IT-<md5 of myscript.js contents>.json with the following content
    {"translation-revision-date":"2018-12-05 11:51+0000","generator":"WP-CLI\/2.1.0-alpha","domain":"messages","locale_data":{"messages":{"":{"domain":"messages","lang":"it_IT","plural-forms":"nplurals=2; plural=n != 1;"},"Translatable string":["Stringa traducibile da JavaScript!"]}}}
    
  1. set the script as translatable (which automatically adds wp-i18n dependency)
    <?php
    wp_set_script_translations('myscript', 'tctextdomain', get_theme_file_path('languages'));
    
  1. enqueue the script wp_enqueue_script('myscript')

The string won't be localized because WP does not output translations for the script in wp.i18n. This is because wp-includes/l10n.php::load_script_textdomain() returns false in class.wp-scripts.php::print_translations():528. This happend because for some counterintuitive reason the md5-based filename check is executed after the check for relative source which returns false.

Solution: move the md5-based filename check

<?php
        $md5_filename = $file_base . '-' . md5( $relative ) . '.json';
        if ( $path && file_exists( $path . '/' . $md5_filename ) ) {
                return file_get_contents( $path . '/' . $md5_filename );
        }
        if ( file_exists( $languages_path . '/' . $md5_filename ) ) {
                return file_get_contents( $languages_path . '/' . $md5_filename );
        }

before load_script_textdomain():913, possible right after the check for handle-based filename existence at load_script_textdomain():897.

Attachments (1)

45488.diff (11.0 KB) - added by swissspidy 5 years ago.

Download all attachments as: .zip

Change History (13)

#1 @swissspidy
5 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.0.1
  • Severity changed from blocker to normal

+1000 for using the brand-new make-json command! 🎉

I think we should change the behavior of load_script_textdomain to work like load_plugin_textdomain():

  1. First look in the wp-content/languages directory. Even if a path is provided.return translations.
  2. If a path is provided, check the custom path for MD5 hash file name.
  3. If a path is provided, check the custom path for script handle file name.

@swissspidy
5 years ago

#2 @swissspidy
5 years ago

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

#3 @ettoredn
5 years ago

I actually dug deeper into the issue and it seems to me that it is an interop issue between the filename generation strategy used by wp-cli and the one used by WP.

wp-cli i18n make-json generates

<lang>-<filename_in_PO>.json

whereas WP sets $md5_filename to

<textdomain>-<lang>-<md5>.json

As previously stated, I'm assuming executing wp-cli i18n as

wp i18n make-json <my_theme>/languages

I also realized wp-cli's md5 hash is computed from the relative filename found in the PO file (e.g. "js/myscript.js") which is relative to the path given to wp i18n make-pot's mandatory <source> parameter, whereas WP

Moreover, I wonder whether l18n.php::load_script_textdomain():916 should actually have the equality operator i.e.

<?php
( ! isset( $src_url['host'] ) || $src_url['host'] === $content_url['host'] )

instead of

<?php
( ! isset( $src_url['host'] ) || $src_url['host'] !== $content_url['host'] )

since the comment above states "If the host is the same or it's a relative URL.". If so, the the same comparison at :926 appears to need to be changed.

Version 2, edited 5 years ago by ettoredn (previous) (next) (diff)

#4 @ettoredn
5 years ago

I manually applied your patch to the source code (don't ask), and load_script_textdomain() keeps returning false because

<?php
$md5_filename = false


because of

<?php
$relative = false

I believe my previous comment describes why $relative is wrongful false.

Even if we fix that, we still get

<?php
$relative = "themes/<mytheme>/js/myscript.js";
$md5_filename = $file_base . '-' . md5( $relative ) . '.json' = "<domain>-<lang>-<md5($relative);

and, of course,

md5("themes/<mytheme>/js/myscript.js") != md5(<relative path in PO file used by wp-cli>).

There seems to be an underlying interop problem between WP and wp-cli.

#5 @swissspidy
5 years ago

Sounds like we create wrong file names in WP-CLI. I‘ll have a look asap!

Last edited 5 years ago by swissspidy (previous) (diff)

#6 @ocean90
5 years ago

  • Keywords needs-unit-tests added

#7 @pento
5 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#8 @swissspidy
5 years ago

$relative should be js/myscript.js, not themes/<mytheme>/js/myscript.js. That's what GlotPress uses for the file name generation and what WP-CLI should use as well.

I'll need to test this again, unless anyone beats me to it.

Note that any patch would need to be rebased against #45528.

Also, I might open a new ticket for the change of loading order

#9 @herregroen
5 years ago

@ettoredn any chance you could test the latest patch for #45528?

I think that may solve this issue as well, it features several improvements to resolving the relative file name.

In themes and plugins this should indeed be resolved to the filename relative to the root of each specific plugin/theme.

#10 @pento
5 years ago

  • Milestone changed from 5.0.2 to 5.0.3

#11 @swissspidy
5 years ago

  • Keywords close added

I finally got to test this with a custom plugin and [Traduttore](https://github.com/wearerequired/traduttore). The Traduttore plugin creates JSON files just like WordPress.org and it's being used successfully to translate some Gutenberg bloccks.

The plugin creates JSON files with identical MD5 hashes in the file names as wp i18n make-json.

So maybe the issues described here are indeed caused by #45528 after all.

@ettoredn Please test WordPress 5.0.2 RC2 to see if it works for you.

#12 @swissspidy
5 years ago

  • Milestone 5.0.3 deleted
  • Resolution set to worksforme
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.