WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 22 months ago

#37819 new defect (bug)

alternative translations are not merged in

Reported by: imath Owned by:
Milestone: Awaiting Review Priority: normal
Severity: major Version: 4.6
Component: I18N Keywords: needs-patch
Focuses: Cc:
PR Number:

Description

Hi there,

First i'm sorry i haven't noticed this annoying behavior before.

Then i'll try to explain my concern about _load_textdomain_just_in_time().

If i understood correctly #34114 :

  • it's there to ease life to plugin developer as they don't have to use load_plugin_textdomain() if they don't include translations anymore in their plugins.
  • It's also there to make sure a translation is loaded for a given domain if found in WP_LANG_DIR plugins/themes.
  • it improves performance.

All 3 points are great. But sometimes it's important for a company (like it is for the one i work for) to be able to use alternative translations. To do so, some plugins like BuddyPress first look into WP_LANG_DIR/buddypress before the regular locations. In this folder it's possible for users to drop an alternative translation. It's really important because for instance, you cannot use "friends" in a company it doesn't make sense at all.

Now that _load_textdomain_just_in_time() is there, the alternative translation is not loading anymore. The WP_LANG_DIR/plugins/buddypress-fr_FR.mo is always used. I've read #37113 and although i agree for a plugin developer it's a way to fix the issue, when you are a site owner with a lot of plugins, it's not great at all.

To me being able to use an alternative translation is almost as much important as having a plugin translated. It's a really important feature.

And in a way this feature is broken since 4.6 :(

Of course i easily found a way to solve my trouble using some filters. But regular corporate site owners could hesitate to upgrade if they loose their alternative translations...

It looks like using the Translations::merge_originals_with() method instead of Translations::merge_with() for these particular cases is fixing the annoying behavior.

See suggested patch attached to this ticket.

Thanks for your comprehension.

Attachments (7)

37819.diff (865 bytes) - added by imath 3 years ago.
37819.fixRefreshed.diff (865 bytes) - added by imath 3 years ago.
37819.unittestOnly.diff (5.9 KB) - added by imath 3 years ago.
37819.fix&unittest.diff (6.8 KB) - added by imath 3 years ago.
mofiles.tar (5.5 KB) - added by imath 3 years ago.
37819.2.diff (12.4 KB) - added by imath 3 years ago.
mofiles.2.tar (10.0 KB) - added by imath 3 years ago.

Download all attachments as: .zip

Change History (20)

@imath
3 years ago

#1 @swissspidy
3 years ago

  • Keywords has-patch needs-unit-tests added

Thanks for the thorough explanation and the patch. I'll try to dig more into it as soon as I can. Plus, ocean90 will be back next week and he knows more about i18n than I do :-)

#2 @imath
3 years ago

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

Sorry to insist on this ticket @swissspidy @ocean90 but i really think this issue should be fixed.

So i'm adding some other files to this ticket:

  • 37819.fixRefreshed.diff is a refreshed version to latest trunk of my initial patch
  • 37819.unittestOnly.diff only contains a unit test so that you can see without the fix applied the issue (3rd assertion will fail).
  • 37819.fix&unittest.diff contains the unit test and the fix and you'll see that all assertions are then Ok.
  • mofiles.tar contains the mo files i had to create to build the unit test.

I had to edit the dummy plugin so that it includes a new string. I wanted to show you that the patch not only fixes custom language packs loading but also why it is imho a real improvement for user.

You'll see that the custom language pack doesn't include one of the string, it's on purpose. It's to show that even if a custom language pack has not been updated, the string will keep on being translated for the user as the languages/plugin.mo is not unloaded but the custom mo is merged with it for the strings that are in common.

@imath
3 years ago

#3 @ocean90
3 years ago

This behaviour is not new. I thought I created a ticket for that but can't find it right now (or haven't created it…).

load_textdomain() can be called multiple times for the same text domain but existing translations are not overwritten.

Would 37819.diff revert the change from #34213?

#4 @imath
3 years ago

I agree the behaviour of load_textdomain is not new, what's new is _load_textdomain_just_in_time(). As it's loaded early, it prevents custom text domain to be used, unless you unload/load for each plugin which is really too bad...

The patch won't change anything to the order of load_plugin_textdomain() or load_theme_textdomain() because as soon as you find a mo file in WP_LANG_DIR . '/plugins' these functions are returning true and never look into the plugin's language directory.

#5 @imath
3 years ago

37819.2.diff contains an alternative and simplier fix: it's using $mo->merge_originals_with() for any cases and is removing the reference operator in $l10n[$domain] = &$mo;

The patch also contains 2 more tests to check multiple/incomplete mofiles.
The mofiles.2.tar contains the .mo files i've used.

@imath
3 years ago

@imath
3 years ago

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


3 years ago

#7 follow-up: @vizcano
3 years ago

Hi, i want to have my own translation. I used to have it but after update it won’t recognize it.

I can see that you have the same problem, did you find the solution?

I don’t understand all the things you said, i am not a programmer, is there a simpler way to fix this? If so... can you explain me in a simple way?
Please excuse my English but i speak Spanish

Thanks

#8 in reply to: ↑ 7 @G3ronim0
3 years ago

Replying to vizcano:

Hi, i want to have my own translation. I used to have it but after update it won’t recognize it.

I can see that you have the same problem, did you find the solution?

I don’t understand all the things you said, i am not a programmer, is there a simpler way to fix this? If so... can you explain me in a simple way?
Please excuse my English but i speak Spanish

Thanks

Hi @vizcano, you can use this plugin to load your custom translation : https://wordpress.org/plugins/wpt-custom-mo-file/

#9 @vizcano
3 years ago

thanks @G3ronim0 i just installed the plugin it works great! thanks a lot!!!

This ticket was mentioned in Slack in #bbpress by netweb. View the logs.


3 years ago

#11 @muranyia
3 years ago

  • Keywords needs-patch added; has-patch has-unit-tests removed
  • Severity changed from normal to major
  • Summary changed from _load_textdomain_just_in_time() is really not nice with alternative translations to alternative translations are not merged in

attachment:37819.2.diff:ticket:37819​ aims to solve the defunct assignment AND to reverse the direction of the merging.

I suggest that the referential assignment in source:tags/4.7.3/src/wp-includes/l10n.php#L593​ (which seems to be totally unnecessary) is fixed a.s.a.p. as it effectively prevents load_textdomain() from merging in any additional translations.

Then the idea of changing the merging logic (the second part of said patch) can be evaluated more carefully later.
(See some more comments at #40392)

#12 @muranyia
3 years ago

#40392 was marked as a duplicate.

This ticket was mentioned in Slack in #buddypress by netweb. View the logs.


22 months ago

Note: See TracTickets for help on using tickets.