WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 20 months ago

#21319 accepted defect (bug)

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

Reported by: nacin Owned by: nbachiyski
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: I18N Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description

#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.

Attachments (1)

21319.diff (2.2 KB) - added by nacin 2 years ago.

Download all attachments as: .zip

Change History (9)

nacin2 years ago

comment:1 nacin2 years ago

  • Keywords has-patch commit added
  • Owner set to nbachiyski
  • Status changed from new to reviewing

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

comment:2 nacin2 years ago

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

comment:3 follow-up: nbachiyski2 years ago

  • Status changed from reviewing to accepted

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.

Last edited 2 years ago by nbachiyski (previous) (diff)

comment:4 in reply to: ↑ 3 nacin2 years ago

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.)

comment:5 nbachiyski2 years ago

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.

comment:6 greenshady2 years ago

  • Cc justin@… added

comment:7 nacin22 months ago

  • Keywords 2nd-opinion added; commit removed

comment:8 nacin20 months ago

  • Milestone changed from 3.5 to Future Release
Note: See TracTickets for help on using tickets.