is_textdomain_loaded() returns true even if there are no translations for the domain

[nacin]

#10527 introduced is_textdomain_loaded(). It returns true if load_textdomain() has been called, even if no translations were loaded for that domain. I think it should return false if no translations were loaded. As the documentation says, "@return bool Whether there are translations". In this case, no, there are not translations.

Attached patch also does the following:

• Does not store instances of NOOP_Translations inside the $l10n global. Previously, we instantiated NOOP_Translations once for each domain that was missing translations; since we are no longer storing NOOP_Translations instances inside $l10n, we need to avoid instantiating it for every call to get_translations_for_domain(). Thus, NOOP_Translations is now instantiated only once, using a static variable.
• Removes by-references for get_translations_for_domain(), which are no longer needed in PHP5.

[nacin]

I want Nikolay to confirm the original intent here, if possible.

Ultimately, what ends up happening is this: You can load a textdomain where the mo file doesn't exist, and you'll end up with is_textdomain_loaded() returning false, until the first call to get_translations_for_domain(). Then it returns true. The patch fixes that. Unit tests: ​[UT929]. To run the whole class, including the test pointing at this ticket: phpunit --group l10n,21319 Tests_L10n

[nacin]

The static instance of NOOP_Translations was originally suggested by kurtpayne for micro-performance reasons.

[nbachiyski]

Looking at #10527 and the usage of the function I think it's just a matter of semantics.

I think that even if there wasn't a valid MO file, or it didn't contain any translations, we still should consider the textdomain loaded. The purpose of the function is to prevent unwanted expensive double loading of translation files, not to check whether we have actual translations.

I agree that the phpdoc is misleading and we should definitely fix that.

If you, for whatever reason, need a function to check if a textdomain loaded actual MO file, we could have another function for that, but I don't see a use case for it for now.

I am not a big fan of the micro-optimization. First, it makes get_translations_for_domain() harder to read and inverses the logic there. The most common case: return $l10n[ $domain ]; is buried in the middle of the function deeper intended than the rest of the function. Second, we usually have just a handful of textdomains and the noop class is very small, so I don't see the point.

[nacin]

Replying to nbachiyski:

If you, for whatever reason, need a function to check if a textdomain loaded actual MO file, we could have another function for that, but I don't see a use case for it for now.

I am not a big fan of the micro-optimization. First, it makes get_translations_for_domain() harder to read and inverses the logic there. The most common case: return $l10n[ $domain ]; is buried in the middle of the function deeper intended than the rest of the function. Second, we usually have just a handful of textdomains and the noop class is very small, so I don't see the point.

It's actually not a micro-optimization. It was originally suggested as such, but it's necessary if we were to have made this change. No longer would we be storing NOOP_Translations in $l10n, which means we would have instantiated a new NOOP_Translations for every call to get_translations_for_domain() — which can occur hundreds or thousands of times. The static variable was the only sane way to do that.

Seems like a function to check if translations are loaded can check ! isset( $l10n[ $domain ] ) || $l10n[ $domain ] instanceof NOOP_Translations. We do actually have a use case for this — in WP_Theme, there is a load_textdomain() method that is supposed to return true if we have translations, and false if we don't, to avoid unnecessary translate() calls.

The other issue — highlighted by the WP_Theme method — is that the load_textdomain() function (and its plugin/theme/child theme friends) all return true if a MO file was loaded, and false if it was not. So shouldn't is_textdomain_loaded() do the same? I understand the point; it just seems inconsistent.

It's also worth nothing that since "the purpose of the function is to prevent unwanted expensive double loading of translation files," this change wouldn't break that. It would also fix an issue where if a double-loading were to occur with the first one not finding a MO file, it would still trigger an expensive merge_with(). (The isset( $l10n[ $domain ] ) in load_textdomain() should probably also check for ! instanceof NOOP_Translations.)

[nbachiyski]

It's actually not a micro-optimization.

Got it, you're right. If we go this route, we would need something like this. The static would be much harder to test and I would go with a global noop instance, but that's a detail.

I think we should have two different functions for checking whether the loading phase is done and whether we actually loaded a translations file.

I agree about the naming inconsistency. We should think of better names for these functions.

if a double-loading were to occur with the first one not finding a MO file, it would still trigger an expensive merge_with()

It won't be expensive. When we load the domain we merge the current translations onto the new ones, so if the current is empty/noop it will be fast. But, of course, free is better than cheap, so the change would be good.

[MikeHansenMe]

see #30284

[ocean90]

#35442 was marked as a duplicate.

[jrf]

Patch refreshed against current master

[ocean90]

Looking at https://core.trac.wordpress.org/attachment/ticket/35442/35442%20is_textdomain_loaded-unit-test.patch and it's not the result I'd expect from is_textdomain_loaded().

I think this needs to be revisited. 21319.2.diff​ is an updated patch and combines 21319.diff​, 21319-unittests.diff​ and the unit test from #35442 by @jrf.

[jrf]

@ocean90 *grin* looks like we were both refreshing the patch at the same time. Great minds and such... ;-)

[ocean90]

In 36538:

i18n: Prevent is_textdomain_loaded() from returning true even if there are no translations for the domain.

In get_translations_for_domain() don't fill the global $l10n with NOOP_Translations instances, return a NOOP_Translations instance instead.

Props nacin, jrf.
Fixes #21319.