Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#45528 closed defect (bug) (fixed)

load_script_textdomain() doesn't load translations when WP installed in a subdirectory with custom content dir

Reported by: fierevere's profile fierevere Owned by: herregroen's profile herregroen
Milestone: 5.0.2 Priority: normal
Severity: normal Version: 5.0
Component: I18N Keywords: has-screenshots has-patch has-unit-tests fixed-5.0
Focuses: Cc:

Description (last modified by ocean90)

Translation for Gutenberg editor screen is partial when WP_CONTENT_DIR been redefined. JSON translations are present in WP_CONTENT_DIR/languages but are not used.

Steps to reproduce (fresh WP install):
https://codex.wordpress.org/Giving_WordPress_Its_Own_Directory

  1. install localized wordpress in subfolder ( http://localhost:8080/wordpress in my case )
  2. move installation to root of domain

create index.php, redefine siteurl

  1. define new constants
define( 'WP_CONTENT_DIR', '/siteroot/wp/wp-content' );
define( 'WP_CONTENT_URL', 'http://localhost:8080/wp-content' );
define( 'WP_PLUGIN_DIR', '/siteroot/wp/wp-content/plugins' );
define( 'WP_PLUGIN_URL', 'http://localhost:8080/wp-content/plugins' );
  1. ensure everything is working. (It is!)

Try to load editor, (new post/page) - localization is broken.

Expected: geditor-good.jpg
Got: geditor-bad.jpg

Attachments (10)

geditor-normal.jpg (86.9 KB) - added by fierevere 6 years ago.
Expected (translated) editor screen
geditor-bad.jpg (78.9 KB) - added by fierevere 6 years ago.
Broken translation
index.php (81 bytes) - added by fierevere 6 years ago.
index.php redirector for site root folder. Loads wordpress from /wordpress/
45528.patch (1.5 KB) - added by herregroen 6 years ago.
45528.2.patch (2.1 KB) - added by herregroen 6 years ago.
45528.3.patch (2.4 KB) - added by akirk 6 years ago.
Add support for CDNs that mirror all static assets
45528.4.patch (3.7 KB) - added by akirk 6 years ago.
Add unit tests
45528.5.patch (4.6 KB) - added by akirk 6 years ago.
Improve unit test
45528.6.patch (4.8 KB) - added by herregroen 6 years ago.
45528.7.diff (841 bytes) - added by dimadin 6 years ago.

Download all attachments as: .zip

Change History (55)

@fierevere
6 years ago

Expected (translated) editor screen

@fierevere
6 years ago

Broken translation

#1 @fierevere
6 years ago

#45526 was marked as a duplicate.

@fierevere
6 years ago

index.php redirector for site root folder. Loads wordpress from /wordpress/

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


6 years ago

#3 @ocean90
6 years ago

  • Description modified (diff)
  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 5.0.1
  • Severity changed from major to normal
  • Summary changed from Editor JSON translations fail to load if WP_CONTENT_DIR been redefined to load_script_textdomain() doesn't load translations when WP installed in a subdirectory with custom content dir

Related issue: When WordPress is installed in a /wp/ directory, $relative becomes -includes/js/dist/editor.min.js.

Related: #45488

#4 @fierevere
6 years ago

patch provided for related issue #45488

https://core.trac.wordpress.org/attachment/ticket/45488/45488.diff

did not helped with editor screen translation

Last edited 6 years ago by fierevere (previous) (diff)

@herregroen
6 years ago

#6 @herregroen
6 years ago

  • Keywords has-patch added; needs-patch removed

This patch should work. The path resolve logic was written expecting the full src as it would be passed to the eventual script tags, which is what allows the easiest handling of custom directories.

A later change during the initial patch means we now get the src from the registered object. The associated filter was run but the earlier fixes of the src were not replicated. Adding these should resolve the issue.

Need to think a bit on how to add a proper unit test for this.

#7 follow-up: @fierevere
6 years ago

https://core.trac.wordpress.org/attachment/ticket/45528/45528.patch
applied patch, but still seeing same problem with translation

This ticket was mentioned in Slack in #polyglots by yui. View the logs.


6 years ago

#9 in reply to: ↑ 7 @tmatsuur
6 years ago

Replying to fierevere:

https://core.trac.wordpress.org/attachment/ticket/45528/45528.patch
applied patch, but still seeing same problem with translation

Is there a problem with conditional statements that determine whether host name match?

Current:

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

In this statement, even if the host names match, it will be false.

I propose the following conditional expression.

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

#10 follow-up: @fierevere
6 years ago

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

this didnt helped too.

        // If the source is not from WP.
        if ( false === $relative ) {
                return false;
        }

function exiting here with $relative === false

#11 in reply to: ↑ 10 @tmatsuur
6 years ago

Replying to fierevere:

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

this didnt helped too.

        // If the source is not from WP.
        if ( false === $relative ) {
                return false;
        }

function exiting here with $relative === false

In my environment it works with this.

if (
	strpos( $src_url['path'], $content_url['path'] ) === 0 &&
//	( ! isset( $src_url['host'] ) || $src_url['host'] !== $content_url['host'] )
	( ! isset( $src_url['host'] ) || $src_url['host'] === $content_url['host'] )
) {
	// Make the src relative the specific plugin or theme.
//	$relative = trim( substr( $src, strlen( $content_url['path'] ) ), '/' );
	$relative = trim( substr( $src_url['path'], strlen( $content_url['path'] ) ), '/' );

If $src is http://localhost/wp-content/plugins/myplugin/assets/js/comon.js,
$relative is assets/js/common.js.

And the translation file myplugin-ja-4a9aafdf461c74b88a1c7041526ae1d6.json is successfully loaded.

#12 follow-up: @fierevere
6 years ago

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

changing this statement does not help

the piece of debugging code i have:

        $site_url    = wp_parse_url( site_url() );
ob_start(); var_dump($src_url['path']); error_log(ob_get_clean());
ob_start(); var_dump($content_url['path']); error_log(ob_get_clean());
ob_start(); var_dump($site_url['path']); error_log(ob_get_clean());
ob_start(); var_dump($src_url['host']); error_log(ob_get_clean());
ob_start(); var_dump($content_url['host']); error_log(ob_get_clean());
ob_start(); var_dump($site_url['host']); error_log(ob_get_clean());
        // If the host is the same or it's a relative URL.
if (
        strpos( $src_url['path'], $content_url['path'] ) === 0 &&
        ( ! isset( $src_url['host'] ) || $src_url['host'] === $content_url['host'] )
) {
error_log("branch 1");
       // Make the src relative the specific plugin or theme.
        $relative = trim( substr( $src_url['path'], strlen( $content_url['path'] ) ), '/' );
                $relative = explode( '/', $relative );
                $languages_path = WP_LANG_DIR . '/' . $relative[0];
                $relative = array_slice( $relative, 2 );
                $relative = implode( '/', $relative );
        } 
          elseif ( ! isset( $src_url['host'] ) || $src_url['host'] !== $site_url['host'] ) {
error_log("branch 2");
                if ( ! isset( $site_url['path'] ) ) {
error_log("branch 2a");
                        $relative = trim( $src_url['path'], '/' );
                } elseif ( ( strpos( $src_url['path'], $site_url['path'] ) === 0 ) ) {
error_log("branch 2b");
                        // Make the src relative to the WP root.
                        $relative = substr( $src_url['path'], strlen( $site_url['path'] ) );
                        $relative = trim( $relative, '/' );
                }
        } else error_log("branch neither"); 

ob_start(); var_dump($relative); error_log(ob_get_clean());
        // If the source is not from WP.
        if ( false === $relative ) {
                return false;
        }

variable dumps and branch diagnostics:

[Sat Dec  8 12:47:50 2018] string(44) "/wordpress/wp-includes/js/dist/blocks.min.js"

[Sat Dec  8 12:47:50 2018] string(11) "/wp-content"

[Sat Dec  8 12:47:50 2018] string(10) "/wordpress"

[Sat Dec  8 12:47:50 2018] string(9) "localhost"

[Sat Dec  8 12:47:50 2018] string(9) "localhost"

[Sat Dec  8 12:47:50 2018] string(9) "localhost"

[Sat Dec  8 12:47:50 2018] branch neither
[Sat Dec  8 12:47:50 2018] bool(false)

#13 in reply to: ↑ 12 @tmatsuur
6 years ago

[Sat Dec  8 12:47:50 2018] string(44) "/wordpress/wp-includes/js/dist/blocks.min.js"

[Sat Dec  8 12:47:50 2018] string(11) "/wp-content"

[Sat Dec  8 12:47:50 2018] string(10) "/wordpress"

[Sat Dec  8 12:47:50 2018] string(9) "localhost"

[Sat Dec  8 12:47:50 2018] string(9) "localhost"

[Sat Dec  8 12:47:50 2018] string(9) "localhost"

[Sat Dec  8 12:47:50 2018] branch neither
[Sat Dec  8 12:47:50 2018] bool(false)

The JavaScript file under the wp-includes directory is false under the condition of the previous strpos function.

strpos( $src_url['path'], $content_url['path'] ) === 0

In this case it is not judged whether the host name match.

I think that it is necessary to verify with JavaScript files in the theme and plugins below the wp-content directory.

#14 follow-up: @fierevere
6 years ago

Its not plugin/theme, new core editor (block editor/gutenberg) is using json translations too.
They are not loaded with custom WP_CONTENT_DIR

This issue can affect themes/plugins, but it affects core too.

And yes, js files are under wp-includes and translated strings json files in wp-content/languages/

load_script_textdomain()

must handle core js files translation case too, since its used now for editor translation.

Last edited 6 years ago by fierevere (previous) (diff)

#15 in reply to: ↑ 14 @tmatsuur
6 years ago

Replying to fierevere:

Its not plugin/theme, new core editor (block editor/gutenberg) is using json translations too.
They are not loaded with custom WP_CONTENT_DIR

This issue can affect themes/plugins, but it affects core too.

And yes, js files are under wp-includes and translated strings json files in wp-content/languages/

load_script_textdomain()

must handle core js files translation case too, since its used now for editor translation.

I will understand that.

At the same time I think that this function is an attractive feature of 5.0.0.

Using the wp_set_script_translations function in themes and plugins,
the translation function can be used within JavaScript.

Currently, '[domain]-[locale]-[handle].json' under the theme and plugin can be used.

However, '[domain]-[locale]-[md5-hash].json' can not be used.

I expect that this problem can be solved.
And, I think "Host name comparison condition" is important.

#16 @ocean90
6 years ago

  • Keywords needs-patch added; has-patch removed

45528.patch also doesn't solve comment:3.

I'm wondering if we need another check with includes_url() and if doing a strict replacement with the full URL would be better, like $path = str_replace( includes_url(), '', $src );. Similar is done in _WP_Editors::editor_settings() for the language files.

@herregroen
6 years ago

#17 follow-up: @herregroen
6 years ago

Updated the patch with @tmatsuur's suggestion which should work. Also added a trailingslashit to the site url path comparison which should should solve comment:3 I believe.

Works on my local setup with WP in a subdirectory but I think there's a difference with how it's set up for @fierevere. If anyone could try out this patch that would be awesome.

#18 in reply to: ↑ 17 @tmatsuur
6 years ago

Replying to herregroen:

Updated the patch with @tmatsuur's suggestion which should work. Also added a trailingslashit to the site url path comparison which should should solve comment:3 I believe.

Thank you for creating the patch file.

#19 @fierevere
6 years ago

45528.2.patch

works for me with translating core editor.

var_dump for $relative and $md5_filename

[Sat Dec  8 18:34:08 2018] string(36) "wp-includes/js/dist/api-fetch.min.js"

[Sat Dec  8 18:34:08 2018] string(43) "ru_RU-1bba9045bb07c89671c88a3f328548e8.json"

[Sat Dec  8 18:34:08 2018] string(33) "wp-includes/js/dist/blocks.min.js"

[Sat Dec  8 18:34:08 2018] string(43) "ru_RU-1a0cd6a7128913b15c1a10dd68951869.json"

[Sat Dec  8 18:34:08 2018] string(35) "wp-includes/js/dist/keycodes.min.js"

[Sat Dec  8 18:34:08 2018] string(43) "ru_RU-a25d1cc7bf7ca0b4e114f6bea64943f4.json"

[Sat Dec  8 18:34:08 2018] string(37) "wp-includes/js/dist/components.min.js"

[Sat Dec  8 18:34:08 2018] string(43) "ru_RU-7f13c36c641b114bf18cd0bcc9ecc7e0.json"

[Sat Dec  8 18:34:08 2018] string(30) "wp-includes/js/dist/nux.min.js"

[Sat Dec  8 18:34:08 2018] string(43) "ru_RU-0ce75ad2f775d1cac9696967d484808c.json"

[Sat Dec  8 18:34:08 2018] string(33) "wp-includes/js/dist/editor.min.js"

[Sat Dec  8 18:34:08 2018] string(43) "ru_RU-bf0f094965d3d4a95b47babcb35fc136.json"

[Sat Dec  8 18:34:08 2018] string(40) "wp-includes/js/dist/block-library.min.js"

[Sat Dec  8 18:34:08 2018] string(43) "ru_RU-f8f49d9fc4a9cf7d78ec99285417bd9c.json"

[Sat Dec  8 18:34:08 2018] string(36) "wp-includes/js/dist/edit-post.min.js"

[Sat Dec  8 18:34:08 2018] string(43) "ru_RU-8860e58c20c6a2ab5876a0f07be43bd9.json"

[Sat Dec  8 18:34:08 2018] string(41) "wp-includes/js/dist/format-library.min.js"

[Sat Dec  8 18:34:08 2018] string(43) "ru_RU-68f2cec7514bf8563c723a4d675fcfe6.json"


tested this on a live site with more complex directory move set up.
Works too.

PS: WordPress installed in /wp/ ( shall fix Related: #45488 as well )

Version 1, edited 6 years ago by fierevere (previous) (next) (diff)

This ticket was mentioned in Slack in #polyglots by yui. View the logs.


6 years ago

#21 @fierevere
6 years ago

  • Keywords has-patch added; needs-patch removed

#22 @babaevan
6 years ago

45528.2.patch works for me.
Thanks to @fierevere and @herregroen.

#23 @mypacecreator
6 years ago

It also works in the Japanese locale. Thanks!

#24 @kraftner
6 years ago

For people looking for a workaround who are not willing to patch core:

It seems as if installing Gutenberg as Plugin fixes the translations until this issue is resolved.

This ticket was mentioned in Slack in #core-editor by ocean90. View the logs.


6 years ago

This ticket was mentioned in Slack in #polyglots by yui. View the logs.


6 years ago

#27 @matteospi
6 years ago

All is solved right now for me, with Italian language. Thanks! :)

#28 @pento
6 years ago

  • Milestone changed from 5.0.1 to 5.0.2

@akirk
6 years ago

Add support for CDNs that mirror all static assets

#29 @akirk
6 years ago

Building the proper relative path also fails when the assets are served from another server, like for example for Jetpack CDN. The Javascript URL might be something like https://c0.wp.com/c/5.0.1/wp-includes/js/dist/edit-post.min.js.

I've submitted a 45528.3.patch the patch to accommodate for this case.

#30 @swissspidy
6 years ago

  • Keywords needs-testing added

I think all that's needed here are some unit tests and some manual testing as well. Unfortunately I am not around much over the weekend to help out with that. Any volunteers? :-)

This currently breaks I18N for lots of people, so it'd be really great to get this committed as soon as possible.

@akirk
6 years ago

Add unit tests

#31 @akirk
6 years ago

I've added some unit tests. I'm not super happy about how it depends on the siteurl option. The default content is http://example.org, we also need to test against http://example.org/wp/. Any thoughts on how to improve that unit test?

#32 @herregroen
6 years ago

@akirk a somewhat cleaner option might be to temporarily add a filter to site_url instead and remove it again after the assertion?

@akirk
6 years ago

Improve unit test

#33 @herregroen
6 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

Latest patch looks good to me. I've asked @fierevere on slack to give it a test-run as well.

If that gives no issues I hope to commit it either today or tomorrow, depending on how soon some manual testing can be done.

#34 @fierevere
6 years ago

45528.5.patch
still works,
works in test environment.
works on live server with WP_CONTENT_DIR in /data/ and WordPress in /wp/

@herregroen
6 years ago

#35 @herregroen
6 years ago

Added another patch that instead of attempting to resolve CDNs adds a filter that may be used to correctly resolve the relative path.

I don't think core should be responsible for resolving the relative path regardless of the format it takes, that should be up to the plugin or site using the CDN.

@akirk could you check if the filter in the latest patch would be sufficient for your use-case?

#36 @akirk
6 years ago

@herregroen Sure, that works. A new filter definitely is a viable option. Thanks!

#37 @fierevere
6 years ago

tested .6

works as fine as .5

#38 @herregroen
6 years ago

  • Owner set to herregroen
  • Resolution set to fixed
  • Status changed from new to closed

In 44209:

I18N: Fix JavaScript translations for subdirectory installations.

Fixes the load_script_textdomain function not resolving the md5 hash based on the relative path for WordPress installations in a subdirectory. Also adds a filter to allow sites using CDNs or other alternative asset locations to filter the relative path resolution.

Props akirk, fierevere, swissspidy, mypacecreator, babaevan, tmatsuur, ocean90.
Fixes #45528.

#39 @herregroen
6 years ago

  • Keywords fixed-5.0 added; needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

#40 @dimadin
6 years ago

  • Keywords fixed-5.0 removed

Let me start by saying that JavaScript translations feature was rushed, without consideration what backward compatibility it will break (like mentioned in comment:10:ticket:45425 or #45530), and since WP 5.0 is rushed, without having a chance for proper testings.

I discovered this problem while testing [44232] that is included in 5.0.2-RC1. Accidentally, site I was testing had activated plugin I made just recently and it turned out that it prevented JavaScript translations for block editor from loading with 5.0.2-RC1 while it didn't do that with 5.0 and 5.0.1. Since plugin uses script_loader_src filter, it was obviously something related to [44209], and I decided to investigate load_script_textdomain function and how it actually gets relative path it uses.

few hours later

So what load_script_textdomain does is gets handle's _WP_Dependency object and from object's src property (handle's source) tries to get relative path. It assumes that handle's source is in WordPress installation, if it's not, it fails.

Problem is that (as far as I can see) load_script_textdomain unnecessarily applies script_loader_src filter on handle's source before it starts getting relative path. Why unnecessarily? Because script_loader_src filter should change handle's source just before it's printed on a page, while load_script_textdomain function needs raw handle's source, like when it was registered, it doesn't depend on final source used for printing. If source is not on WP, it'll fail anyway. With filter, we are only increasing chances of source not being on WP and failing.

In any case, $src parameter that load_script_textdomain function passes to script_loader_src filter is different than one that is originally passed to the filter in WP_Scripts::do_item() method: it lacks ver query argument. Pre-[44209] it also didn't make full URL for handle's source when it's relative path, this is the reason why I didn't notice problem before (source wasn't full URL).

After removing script_loader_src filter, I didn't see any problem.

Building the proper relative path also fails when the assets are served from another server, like for example for Jetpack CDN.

This is only true for plugins, like Jetpack, that don't use filters to change source and instead forcefully change src property of handle's _WP_Dependency object. We shouldn't require that now all plugins that use script_loader_src filter also need to use load_script_textdomain_relative_path filter, this is backward incompatible and unannounced, and it kinda punishes those that used standard way from the start because some didn't.

tl;dr I propose to remove script_loader_src filter from load_script_textdomain function.

@dimadin
6 years ago

#41 @ocean90
6 years ago

The filter was added as part of [43825]. I don't see a specific need for having the filter here as it's mostly used for plugins to rewrite the URL to a CDN version. I'm fine with removing it now that we also have load_script_textdomain_relative_path to customize the path.

#42 @pento
6 years ago

In 44288:

i18n: Remove the script_loader_src filter from load_script_textdomain().

This filter is superfluous here, the load_script_textdomain_relative_path should be used for customising the textdomain path.

See #45528.
Props dimadin.

#43 @pento
6 years ago

  • Keywords fixed-5.0 added

#44 @desrosj
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 44310:

I18N: Fix JavaScript translations for subdirectory installations.

Fixes the load_script_textdomain function not resolving the md5 hash based on the relative path for WordPress installations in a subdirectory. Also adds a filter to allow sites using CDNs or other alternative asset locations to filter the relative path resolution.

Props akirk, fierevere, swissspidy, mypacecreator, babaevan, tmatsuur, ocean90, herregroen.

Merges [44209] to trunk.

Fixes #45528.

#45 @SergeyBiryukov
6 years ago

In 44318:

i18n: Remove the script_loader_src filter from load_script_textdomain().

This filter is superfluous here, the load_script_textdomain_relative_path should be used for customising the textdomain path.

Props dimadin.
Merges [44288] to trunk.
See #45528.

Note: See TracTickets for help on using tickets.