Opened 5 months ago
Closed 4 months ago
#60888 closed defect (bug) (fixed)
Fatal error when passing non-strings to `WP_Translation_Controller::load_file()`
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
This ticket was mentioned in Slack in #core by swissspidy. View the logs.
5 months ago
#4
@
5 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
@
5 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.
5 months ago
#6
- Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed
Trac ticket: https://core.trac.wordpress.org/ticket/60888
5 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:
5 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
@
5 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
@
5 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.
@swissspidy commented on PR #6347:
5 months ago
#12
#13
@
5 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.
5 months ago
#15
@
5 months ago
- Keywords dev-reviewed added; dev-feedback removed
[57925] looks good for backport to 6.5
#18
@
5 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
@
5 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.
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.