Fatal error when passing non-strings to `WP_Translation_Controller::load_file()`

[swissspidy]

Some plugins/themes such as Divi for example call load_plugin_textdomain() with a $domain value of null instead of a string, which is plain wrong and unsupported: load_plugin_textdomain(NULL, false, 'includes/langua...').

This leads to this fatal error:

PHP Fatal error:  Uncaught TypeError: WP_Translation_Controller::load_file(): Argument #2 ($textdomain) must be of type string, null given,

Props @verygoode for investigating and reporting ​in this Slack thread.

While a non-string text domain is really not supported and will lead to way more undefined behavior than just this error, there seem to be enough affected plugins (basically any Divi extension) that it's worth fixing this in core by adding some hardening (or well, softening).

We could add some hardening to load_textdomain() and load_plugin_textdomain() et al to basically do if ( ! is_string( $domain ) ) { $domain = 'default'; }. That would be the most robust solution at the right level.

Definitely needs some unit tests to verify this.

[lots.0.logs]

Thanks for bringing this to our attention. After investigating the issue it looks like the problem is Divi expects the Divi Extensions to set the domain property when extending the base extension class. We weren't checking if its set before passing it to load_plugin_textdomain(). We are adding a check as I type this and will be releasing an updated version of Divi today.

[lots.0.logs]

The issue has been fixed in Divi v4.24.3 which is available now. Note: The change log entry for the fix didn't make into the change log due to it being so last minute. Here it is for reference:

Fixed a fatal error that can occur with WordPress 6.5 while using a 3rd-party Divi extension which does not properly set the domain property when extending the DiviExtension base class.

[swissspidy]

@lots0logs Awesome, thank you so much! Appreciate the quick turnaround. That might make this less urgent to fix in a 6.5.1. Still, I am looking into it now as other plugins might be affected as well.

Trac ticket: https://core.trac.wordpress.org/ticket/60888

Why does WordPress need to handle this developer error gracefully? It seems the suggested patch just silently hides the error for invalid parameter by replacing it with a valid one before passing it on to the object, still letting load_textdomain accepting it in the first place.

IMHO: load_textdomain should immeditalely trigger the same error, or, if badly needed, transform it to a warning.

That's a legit question.

Up until now, WordPress has actually somehow gracefully handled this scenario. As per the bug report, plugins have been doing it wrong like this for years, even though the behavior was entirely undefined. No warning or error happened though. Now with the stricter type hint in the new L10N library in 6.5, there suddenly is a fatal error. That's an implicit backward compatibility break and a bad user experience.

At second glance, we should just bail early instead of defaulting to default, as that more closely resembles the previous behavior.

IMHO: load_textdomain should immeditalely trigger the same error, or, if badly needed, transform it to a warning.

For 6.6 we can consider a _doing_it_wrong warning for sure. Similar to https://core.trac.wordpress.org/ticket/44937

For 6.5.1 however, we should just ensure there is no fatal error.

[Gabriel Reguly]

Looks like Gravity Forms also experiences that same issue.

/wp-content/plugins/gravityforms/includes/addon/class-gf-addon.php(6531):

 GFCommon::load_gf_text_domain(NULL, 'gforms-addon-fo...')

[swissspidy]

Most likely caused by a GF add-on not following the ​documentation and defining $_slug is expected.

The current patch should cover that.

[swissspidy]

In 57925:

I18N: Bail early if an invalid text domain is passed to load_textdomain() et al.

Some plugins pass invalid values such as null instead of a string, which has never been supported by WordPress (no translations are loaded) and was technically undefined behavior. With the introduction of the new l10n library in #59656, which has stricter type hints, this could end up causing warnings or even fatal errors.

This change adds a deliberate short-circuit to load_textdomain() & co. to better handle such a case and document that it is not supported.

Props verygoode, swissspidy.
Fixes #60888.

https://core.trac.wordpress.org/changeset/57925

[jorbin]

[57925] looks good for backport to 6.5

[davidbaumwald]

Milestone renamed

[rawb]

Thanks for fixing this! It completely broke our network of sites and was fun to track down. We'll stay on 6.4.4 until 6.5.3 is released. :)

[swissspidy]

@rawb Instead of waiting to update, I recommend fixing/updating the offending plugin/theme. Report it to the author that they should fix their incorrect code.

[swissspidy]

In 58066:

I18N: Bail early if an invalid text domain is passed to load_textdomain() et al.

Some plugins pass invalid values such as null instead of a string, which has never been supported by WordPress (no translations are loaded) and was technically undefined behavior. With the introduction of the new l10n library in #59656, which has stricter type hints, this could end up causing warnings or even fatal errors.

This change adds a deliberate short-circuit to load_textdomain() & co. to better handle such a case and document that it is not supported.

Merges [57925] to the 6.5 branch.
Reviewed by jorbin.

Props verygoode, swissspidy.
Fixes #60888.