Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 6 years ago

#34114 closed enhancement (fixed)

Remove the requirement to call load_plugin_textdomain() or load_theme_textdomain()

Reported by: johnbillion's profile johnbillion Owned by: swissspidy's profile swissspidy
Milestone: 4.6 Priority: high
Severity: normal Version:
Component: I18N Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

In the near future we'll start to see some plugins and themes which do not bundle any translations, instead relying on all translations being powered by language packs from translate.wordpress.org. In this situation, the load_[plugin|theme]_textdomain() call falls back to the WP_LANG_DIR/[plugins|themes] directory.

It makes sense then that the requirement for such a plugin or theme to call load_[plugin|theme]_textdomain() could be removed, because the translation now lives in one of two known locations, and as long as MO files were loaded JIT.

The first time core encounters a localised string with a textdomain that has not already been loaded, it should look for a corresponding MO file first in the WP_LANG_DIR/plugins directory and then in the WP_LANG_DIR/themes directory, and load it if it's found.

Attachments (9)

34114.diff (9.4 KB) - added by swissspidy 8 years ago.
34114.2.diff (10.9 KB) - added by swissspidy 8 years ago.
34114.3.diff (10.4 KB) - added by swissspidy 8 years ago.
internationalized-theme-de_DE.mo (715 bytes) - added by swissspidy 8 years ago.
.mo file used in the tests
internationalized-plugin-de_DE.mo (717 bytes) - added by swissspidy 8 years ago.
.mo file used in the tests
34114.4.diff (10.2 KB) - added by swissspidy 8 years ago.
34114.5.diff (9.8 KB) - added by ocean90 8 years ago.
34114.6.diff (9.7 KB) - added by swissspidy 8 years ago.
Fix_notice_when_filter_override_load_textdomain_is_ture.patch (632 bytes) - added by danielhuesken 8 years ago.
Fix notice when filter override_load_textdomain is ture

Download all attachments as: .zip

Change History (47)

#1 @dd32
8 years ago

This is very similar to something I've been working on - allowing JIT translation loading from the "loaded" textdomains, extending it to auto-load textdomains is something I hadn't thought of, but something we could do easily.

One of the reasons I've been looking into JIT is that currently our POMO classes load all translations upon load_textdomain() call, which if you have many plugins which only have strings on a small percentage of pages (or admin only) causes a bunch of extra memory usage (plus loading MO files is slow)

@swissspidy
8 years ago

#2 @swissspidy
8 years ago

  • Keywords has-patch has-unit-tests added; needs-patch removed
  • Milestone changed from Awaiting Review to Future Release

I thought I give this a shot.

What 34114.diff does:

  • In get_translations_for_domain(), tries to load plugin or theme text domain if the domain is unknown. Maybe performance can be improved a bit here, not sure.
  • Creates a new plugin and a new theme with dummy pomo files, calling __() in the tests.

I wasn't sure about the best approach, so now I just copied the theme / plugin files to the appropriate directories. I'm not that familiar with i18n stuff :-)

#3 @nacin
8 years ago

The first time core encounters a localised string with a textdomain that has not already been loaded, it should look for a corresponding MO file first in the WP_LANG_DIR/plugins directory and then in the WP_LANG_DIR/themes directory, and load it if it's found.

Very smart approach, @johnbillion. And @swissspidy, a very elegant patch with some solid tests.

The main problem here is we'll double the number of file_exists() checks, as at the time of call, we don't know whether to look in plugins or themes. I think there's a decent way out of this one.

There's only ever going to be one or two themes, so maybe we can do that one a bit differently — perhaps by always calling it and eating the 2 or 4 file_exists() checks — and then using load_plugin_textdomain() inside get_translations_for_domain(). Thus 2p + 2t + 2c (where c is child theme and can be 0), rather than 4(p + t + c). That's still a change from what we have now, which is 2( p + t + c).

That's still not great, as there's a good chance a plugin is internationalized but not translated, which means we're burning file_exists() checks on those sites. This includes en_US sites, by the way.

The underlying stat() call isn't going to be terribly expensive in most situations, but we should still seek to avoid unnecessary calls with whatever is reasonable.

#4 @dd32
8 years ago

The main problem here is we'll double the number of file_exists() checks, as at the time of call, we don't know whether to look in plugins or themes. I think there's a decent way out of this one.

My thoughts here is that we should pre-cache the files which we know about for plugins:

  • Plugins: glob() languages/plugins/ to find out which files exist once per pageload, never file_exists() just load directly
  • Themes: Can only be one of two, if the textdomain matches STYLESHEET/TEMPLATE then just perform the needed file_exists() checks it's not really worth optimizing this. It could be optimized to not performing file_exists() and just attempting to read the file (which should fail just as fast).

@swissspidy
8 years ago

#5 @swissspidy
8 years ago

Thanks for the quick feedback! It led me to another round of testing.

It's clear that using load_plugin_textdomain() and load_theme_textdomain() isn't the solution. I like the idea of caching the list of translation files, so in 34114.2.diff I added a load_textdomain_just_in_time() function that does this the first time it's called (using get_available_languages()).

I haven't done any in-depth testing, but this method is definitely faster. Presumably even better than having every plugin call load_plugin_textdomain().

Some notes:

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


8 years ago

#7 @ocean90
8 years ago

  • Milestone changed from Future Release to 4.6

@swissspidy
8 years ago

#8 @swissspidy
8 years ago

34114.3.diff is a refreshed version of 34114.2.diff leveraging the fact that WP_LANG_DIR is now defined in the test suite, making the code a bit simpler.

#9 follow-up: @giuseppe.mazzapica
8 years ago

@swissspidy So a theme can load a translation from WP_LANG_DIR . '/plugins' and a plugin from WP_LANG_DIR . '/themes'?

It doesn't feel really right...

#10 in reply to: ↑ 9 ; follow-up: @swissspidy
8 years ago

Replying to giuseppe.mazzapica:

@swissspidy So a theme can load a translation from WP_LANG_DIR . '/plugins' and a plugin from WP_LANG_DIR . '/themes'?

It doesn't feel really right...

Pretty sure that's already possible, i.e. when you use __( 'Primary Menu', 'twentyfifteen' ) in your plugin, and the twentyfifteen text domain is loaded, it will translate that text.

#11 in reply to: ↑ 10 ; follow-up: @giuseppe.mazzapica
8 years ago

Replying to swissspidy:

Replying to giuseppe.mazzapica:

@swissspidy So a theme can load a translation from WP_LANG_DIR . '/plugins' and a plugin from WP_LANG_DIR . '/themes'?

It doesn't feel really right...

Pretty sure that's already possible, i.e. when you use __( 'Primary Menu', 'twentyfifteen' ) in your plugin, and the twentyfifteen text domain is loaded, it will translate that text.

If I explicitly load a language file in my plugin folder, as of now, that language file is loaded. Seems I can't do that anymore.

Am I wrong or with your patch there will be no way to load a language file from plugin / theme folder if there's a file for that domain in either plugins or themes language folder?

#12 in reply to: ↑ 11 ; follow-up: @ocean90
8 years ago

Replying to giuseppe.mazzapica:

Am I wrong or with your patch there will be no way to load a language file from plugin / theme folder if there's a file for that domain in either plugins or themes language folder?

If you want to load the files from another folder you still have to use load_plugin_textdomain() or load_theme_textdomain(). This ticket is about an enhancement for plugins/themes which are using language packs.

#13 in reply to: ↑ 12 @giuseppe.mazzapica
8 years ago

Replying to ocean90:

Replying to giuseppe.mazzapica:

Am I wrong or with your patch there will be no way to load a language file from plugin / theme folder if there's a file for that domain in either plugins or themes language folder?

If you want to load the files from another folder you still have to use load_plugin_textdomain() or load_theme_textdomain(). This ticket is about an enhancement for plugins/themes which are using language packs.

Seems I misread the patch.. For some reason I was convinced that the line

if ( load_textdomain_just_in_time( $domain ) || isset( $l10n[ $domain ] ) ) {

was part of load_plugin_textdomain... but actually is for get_translations_for_domain...

facepalm

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


8 years ago

#15 @ocean90
8 years ago

  • Keywords dev-feedback removed

Some feedback on 34114.3.diff:

  • The tests are failing for me:
) Tests_L10n::test_load_plugin_textdomain_just_in_time
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-Das ist ein Dummy Plugin
+This is a dummy plugin

/tests/phpunit/tests/l10n.php:70

2) Tests_L10n::test_load_theme_textdomain_just_in_time
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-Das ist ein Dummy Theme
+This is a dummy theme

/tests/phpunit/tests/l10n.php:99
  • In load_textdomain_just_in_time() the default value of the static variable should be null and if ( empty( $cached_mofiles ) ) should be replace with if ( null === $cached_mofiles ). This prevents recurring lookups for installs which don't have any translations installed.
  • The tests should remove the test plugin/theme from the plugin/theme directory
    • Do we really have to copy them? Tests_Theme_ThemeDir is changing the global $wp_theme_directories variable, for the plugin we could just require the file like Tests_Import_Import does.

#16 @swissspidy
8 years ago

@ocean90 I forgot to mention that you need to create the .mo files yourself when applying the patch, as these binary files are not part of it. Can upload them to this ticket though.

Thanks for the heads-up regarding the copying. I don't like that method, but didn't see how else I could do it. Will look at Tests_Theme_ThemeDir and Tests_Import_Import

@swissspidy
8 years ago

.mo file used in the tests

@swissspidy
8 years ago

.mo file used in the tests

@swissspidy
8 years ago

#17 @swissspidy
8 years ago

@ocean90 Check out 34114.4.diff, where I took your suggestions to heart.

@ocean90
8 years ago

#18 @ocean90
8 years ago

@swissspidy Thanks. The array needs to be initialized, otherwise you'll get a PHP warning: Warning: in_array() expects parameter 2 to be array, null given in /src/wp-includes/l10n.php on line 819.

In 34114.5.diff:

  • Initialize $cached_mofiles as an array if it's null.
  • Short-circuit if domain is 'default' which is reserved for core.
  • Move tests to a new file l10n/loadTextdomainJustInTime.php.
  • In get_translations_for_domain() swap the order of the isset() check and load_textdomain_just_in_time(). No need to try loading the text domain again if it's already there.

#19 @swissspidy
8 years ago

34114.5.diff looks great!

Would be nice if we could do some testing to see how this improves performance.

#20 @swissspidy
8 years ago

Just did some very basic profiling on a medium-sized site with about a dozen plugins. There were 4 scenarios:

  • No patch, en_US
  • No patch, de_DE
  • Patch applied, en_US
  • Patch applied, de_DE

All plugins had de_DE translation files, so for the tests with the patch applied I removed all the load_plugin_textdomain() calls. What I noticed

  • When using de_DE (no patch applied), the site was 30ms slower in average, using about 10MB more memory.
  • When using en_US (patch applied), load time and memory consumption was identical, not real changes there
  • When using de_DE (patch applied), load time was identical as without the patch, memory consumption went down by 1MB.

So, to sum this up, nothing really changes.

#21 @ocean90
8 years ago

  • Priority changed from normal to high

Thanks for the test results @swissspidy!

@DrewAPicture Do you have any feedback on the docs for load_textdomain_just_in_time()?

#22 @DrewAPicture
8 years ago

@ocean90 The docs look good, though I'd probable mark the function private, e.g. @access private to signal that this is intended for core use.

@swissspidy
8 years ago

#23 @swissspidy
8 years ago

34114.6.diff marks _load_textdomain_just_in_time() as private.

#24 @ocean90
8 years ago

  • Keywords commit added

#25 @swissspidy
8 years ago

  • Owner set to swissspidy
  • Status changed from new to accepted

#26 @swissspidy
8 years ago

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

In 37415:

I18N: Remove the requirement to call load_plugin_textdomain() / load_theme_textdomain().

By initially scanning the wp-content/languages directory and loading available MO files just-in-time, plugins and themes do not need to manually load text domains anymore.

Props swissspidy, ocean90.
Fixes #34114

#27 @swissspidy
8 years ago

In 37416:

I18N: Add changes missed in [37415].

See #34114.

#28 @jrf
8 years ago

By initially scanning the wp-content/languages directory and loading available MO files just-in-time, plugins and themes do not need to manually load text domains anymore.

Please make sure that in any communications around this it will say: "plugins and themes for which translations are provided through translate.wordpress.org do not need to manually load text domains anymore"

AFAICS for plugins/themes hosted elsewhere and therefore without access to translate.wordpress.org, this will not work and could actually be problematic. Also see my comment regarding this in #34213#comment:26

Last edited 8 years ago by jrf (previous) (diff)

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


8 years ago

This ticket was mentioned in Slack in #core-i18n by danielhuesken. View the logs.


8 years ago

#31 @danielhuesken
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

There is a notice wenn the filter 'override_load_textdomain' is true. The notice is a 'Undefined index: ...' message for the $l10n Array.

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

@danielhuesken
8 years ago

Fix notice when filter override_load_textdomain is ture

#32 @ocean90
8 years ago

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

In 37440:

I18N: In get_translations_for_domain() check if the global $l10n was set by _load_textdomain_just_in_time() before accessing it.

Props danielhuesken.
Fixes #34114.

#33 @ocean90
8 years ago

  • Keywords needs-dev-note added

#34 @swissspidy
8 years ago

In 37855:

I18N: Enable unloading of text domains that have been loaded just in time.

[37415] removed the requirement to call load_plugin_textdomain() / load_theme_textdomain(). This caused unload_textdomain() to not work properly anymore in these cases.

With this change, unloaded text domains need to be explicitly loaded manually if you want to use them again.

Props swissspidy, ocean90.
Fixes #37113. See #34114.

#35 @ocean90
8 years ago

  • Keywords needs-dev-note removed

#36 follow-up: @leemon
8 years ago

I'm testing 4.6RC in a dev site. A private plugin I developed is no longer loading its .mo files from the plugin's language folder using the following code:

add_action( 'plugins_loaded', 'my_plugins_loaded' );
function my_plugins_loaded() {
    load_plugin_textdomain( 'domain', false, dirname( plugin_basename( __FILE__ ) ) .  '/languages' );
}

I had no issues in 4.5.3. Do I have to change the way I load the plugin's .mo files, now? Any help would be appreciated.

Last edited 8 years ago by leemon (previous) (diff)

#37 in reply to: ↑ 36 @ocean90
8 years ago

Replying to leemon:

I'm testing 4.6RC in a dev site.

Can you post a new topic in the support forums please? Feel free to mention me.

This ticket was mentioned in Slack in #core-i18n by swissspidy. View the logs.


6 years ago

Note: See TracTickets for help on using tickets.