WordPress.org

Make WordPress Core

Opened 20 months ago

Closed 13 months ago

Last modified 9 months ago

#34213 closed enhancement (fixed)

Change priority for loading theme/plugin translations

Reported by: sebastian.pisula Owned by: swissspidy
Milestone: 4.6 Priority: high
Severity: normal Version:
Component: I18N Keywords: has-patch commit
Focuses: Cc:

Description

I think that translations should be load first from wp-content/languages/themes/.... If file not exists then should be loaded from theme dir.

Attachments (5)

34213.patch (843 bytes) - added by sebastian.pisula 20 months ago.
342131.patch (831 bytes) - added by sebastian.pisula 20 months ago.
Support for plugins
34213.2.patch (1.8 KB) - added by swissspidy 19 months ago.
34213.diff (2.7 KB) - added by swissspidy 13 months ago.
34213.2.diff (5.4 KB) - added by swissspidy 13 months ago.

Download all attachments as: .zip

Change History (38)

#1 @SergeyBiryukov
20 months ago

  • Summary changed from Change priorytet for load theme translations to Change priority for loading theme translations

#2 @Clorith
20 months ago

  • Resolution set to wontfix
  • Status changed from new to closed

Hi,

If we do this then theme authors have no way of "opting out" of using core provided translations over their own.

Authors who wish to ship their own translations shouldn't be denied the right to do so just because we offer an alternative.

#3 @swissspidy
20 months ago

  • Milestone Awaiting Review deleted

#4 @ocean90
20 months ago

  • Milestone set to Future Release
  • Resolution wontfix deleted
  • Status changed from closed to reopened

Actually this is something @dd32 and I want to support, maybe not now, but in future. Same for plugins.

#5 @sebastian.pisula
20 months ago

I think that we don't blocked authors because translates are in theme lang directory. So if user will be want change translations then overwrite default translations.

@sebastian.pisula
20 months ago

Support for plugins

#6 @ocean90
20 months ago

  • Component changed from Themes to I18N
  • Keywords has-patch added
  • Summary changed from Change priority for loading theme translations to Change priority for loading theme/plugin translations

#7 @swissspidy
20 months ago

  • Keywords needs-refresh added

Would you mind changing the docs in the patches and adding curly braces around the if statement? Thanks.
Also, 1 patch for themes and plugins would be great.

#8 @swissspidy
19 months ago

  • Keywords needs-refresh removed

34213.2.patch simplifies the load process and also reflects the change in the comments.

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


17 months ago

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


16 months ago

#11 @ocean90
14 months ago

#35653 was marked as a duplicate.

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


13 months ago

#13 @ocean90
13 months ago

  • Milestone changed from Future Release to 4.6

@swissspidy
13 months ago

#14 @swissspidy
13 months ago

Patch still applies cleanly. 34213.diff extends this to load_muplugin_textdomain() as well.

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


13 months ago

#16 @ocean90
13 months ago

34213.diff reverts [29984] for load_theme_textdomain(). Should this be added to WP_Theme::load_textdomain() instead?

Last edited 13 months ago by ocean90 (previous) (diff)

#17 follow-up: @swissspidy
13 months ago

@ocean90 Good catch! I think we're fine by adding that to the last line where $path is used:

… load_textdomain( $domain, $path . '/' . $mofile ); …

I can whip up a new patch with that if it sounds good. Also, I was thinking of adding @since notes to document the change.

#18 in reply to: ↑ 17 ; follow-up: @ocean90
13 months ago

Replying to swissspidy:
The other functions have the $path handling before the loading part. Should we change that for all? Example for load_plugin_textdomain():

<?php
function load_plugin_textdomain( $domain, $deprecated = false, $plugin_rel_path = false ) {
        $locale = get_locale();
        /**
         * Filter a plugin's locale.
         *
         * @since 3.0.0
         *
         * @param string $locale The plugin's current locale.
         * @param string $domain Text domain. Unique identifier for retrieving translated strings.
         */
        $locale = apply_filters( 'plugin_locale', $locale, $domain );

        $mofile = $domain . '-' . $locale . '.mo';

        // Try to load from the languages directory first.
        $loaded = load_textdomain( $domain, WP_LANG_DIR . '/plugins/' . $mofile );
        if ( $loaded ) {
                return true;
        }

        // Otherwise, load the textdomain according to the plugin.
        if ( false !== $plugin_rel_path ) {
                $path = WP_PLUGIN_DIR . '/' . trim( $plugin_rel_path, '/' );
        } elseif ( false !== $deprecated ) {
                _deprecated_argument( __FUNCTION__, '2.7' );
                $path = ABSPATH . trim( $deprecated, '/' );
        } else {
                $path = WP_PLUGIN_DIR;
        }

        return load_textdomain( $domain, $path . '/' . $mofile );
}

#micro-optimization


I also just noticed that $mofile in 34213.diff for load_theme_textdomain() is only valid for language packs. The .mo file has a different naming scheme, from the DocBlock: "The .mo files must be named based on the locale exactly.".

#19 in reply to: ↑ 18 ; follow-up: @swissspidy
13 months ago

Replying to ocean90:

Replying to swissspidy:
The other functions have the $path handling before the loading part. Should we change that for all?
#micro-optimization

Sorry, what exactly would you suggest to change?

I also just noticed that $mofile in 34213.diff for load_theme_textdomain() is only valid for language packs. The .mo file has a different naming scheme, from the DocBlock: "The .mo files must be named based on the locale exactly.".

Ugh, yeah, that's true. Thanks for pointing this out! I should've known as I've run into this before.

#20 in reply to: ↑ 19 @ocean90
13 months ago

Replying to swissspidy:

Sorry, what exactly would you suggest to change?

In comment:18 I've moved the $path initialization after load_textdomain( $domain, WP_LANG_DIR . '/plugins/' . $mofile ); because we don't need it there.

But I'd be fine with leaving it where it is. Can you come up with a refresh for load_theme_textdomain() so we can get this closed in the next days?

#21 @ocean90
13 months ago

  • Priority changed from normal to high

@swissspidy
13 months ago

#22 @swissspidy
13 months ago

34213.2.diff is an updated patch based on the previous suggestions by @ocean90, notably changing the order of $path initialization to make the functions more readable and fixing the $mofile in load_theme_textdomain().

Also, I added @since 4.6.0 entries to the docblocks to document the change.

#23 @ocean90
13 months ago

  • Keywords commit added

#24 @swissspidy
13 months ago

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

#25 @swissspidy
13 months ago

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

In 37414:

I18N: Reverse the order of loading plugin and theme translations.

load_theme_textdomain(), load_plugin_textdomain() and load_muplugin_textdomain() now try to load the .mo file from the wp-content/languages directory first. After the introduction of language packs, translation files are more likely to be located there.

Props swissspidy, sebastian.pisula.
Fixes #34213.

#26 follow-up: @jrf
13 months ago

I foresee problems due to this commit.

There are thousands of (commercial) themes hosted elsewhere and the chances that some of these will use the same text-domain as a theme hosted on wordpress.org are quite large.

Similarly, lots of plugins have a free (hosted on wordpress.org) version and a commercial version hosted elsewhere often using the same text-domain.

Loading translations downloaded from translate.wordpress.org first, disregarding the load location preference of the theme/plugin author, will in that case result in:

  • for the theme example: text strings from another theme with the same name being loaded
  • for the plugin example: only the text strings for the free version being loaded

Both would break localization for the affected themes/plugins.

I realize that theme/plugin authors can still filter the file path with the load_textdomain_mofile filter to overrule the location, but the net effect in that case would be that any potential additional translations in wp-content/languages would be disregarded completely.

Similarly, they could unload_textdomain() an incorrectly loaded text-domain from languages and re-load the correct file using a direct call to load_textdomain(), but again, this seems like a work-around which would be needed because this commit breaks existing and expected functionality.

#27 in reply to: ↑ 26 @dd32
13 months ago

Replying to jrf:

I foresee problems due to this commit.
..
Similarly, lots of plugins have a free (hosted on wordpress.org) version and a commercial version hosted elsewhere often using the same text-domain.

The translation files are only downloaded/installed when the update API's detect the theme/plugin as being the one we host.
If update's are offered for the not-w.org-hosted items then yes, this will cause problems, if the item doesn't trigger update notifications (or the item has included it's own update mechanism), then there's nothing to worry about.

Any issues surrounding translations of not-w.org-hosted items that become more evident with this change were already affected by it, but most likely only for a select few users (Such as those using a language the author doesn't have a translation for).

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


13 months ago

#29 @ocean90
12 months ago

  • Keywords needs-dev-note added

#30 @casiepa
12 months ago

  • Keywords changed from has-patch, commit, needs-dev-note to has-patch commit needs-dev-note

As written elsewhere, I support 200% the switch of priority, but I'm still wondering if all plugin authors will be ready for this. Is there a way to leave an option to the plugin author to indicate to still use the local mo/po and not the centralized translation ?
Pascal.

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


12 months ago

#32 @ocean90
10 months ago

  • Keywords needs-dev-note removed

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


9 months ago

Note: See TracTickets for help on using tickets.