Make WordPress Core

Opened 4 months ago

Closed 3 months ago

#60888 closed defect (bug) (fixed)

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

Reported by: swissspidy's profile swissspidy Owned by: swissspidy's profile swissspidy
Milestone: 6.5.3 Priority: normal
Severity: normal Version:
Component: I18N Keywords: has-patch has-unit-tests fixed-major dev-reviewed
Focuses: Cc:

Description

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.

Change History (22)

#1 @swissspidy
4 months ago

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

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


4 months ago

#3 @lots.0.logs
4 months ago

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.

#4 @lots.0.logs
4 months ago

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.

#5 @swissspidy
4 months ago

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

This ticket was mentioned in PR #6347 on WordPress/wordpress-develop by @swissspidy.


4 months ago
#6

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

@knutsp commented on PR #6347:


4 months ago
#7

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.

@swissspidy commented on PR #6347:


4 months ago
#8

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.

#9 @Gabriel Reguly
4 months ago

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

#10 @swissspidy
4 months ago

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

The current patch should cover that.

#11 @swissspidy
4 months ago

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

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.

#13 @swissspidy
4 months ago

  • Keywords fixed-major dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

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


4 months ago

#15 @jorbin
4 months ago

  • Keywords dev-reviewed added; dev-feedback removed

[57925] looks good for backport to 6.5

#16 @davidbaumwald
3 months ago

  • Milestone changed from 6.5.1 to 6.5.2

Milestone renamed

#17 @jorbin
3 months ago

  • Milestone changed from 6.5.2 to 6.5.3

#18 @rawb
3 months ago

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

#19 @swissspidy
3 months ago

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

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


3 months ago

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


3 months ago

#22 @swissspidy
3 months ago

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

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.

Note: See TracTickets for help on using tickets.