Scripts: Ensure path for JavaScript translations is always a string

[ocean90]

With PHP 8.1 a few rtrim(): Passing null to parameter #1 ($string) of type string is deprecated notices are visible. These have been previously reported in #55411 and #55119 but a separate ticket for this seems to be better so it doesn't get lost again.

The notice is triggered by passing null as the path value for load_script_textdomain() which passes the value to untrailingslashit() which then calls rtrim() on null.

A patch was already submitted by @Chouby in #53635, see 53635-09-script-translations.patch:ticket:53635​.

Related: #55656

Based on https://core.trac.wordpress.org/attachment/ticket/53635/53635-09-script-translations.patch.

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

[SergeyBiryukov]

#56191 was marked as a duplicate.

[ocean90]

#56393 was marked as a duplicate.

Thanks for pointing out this ticket.

Please be aware that for PHP 8.1 (which "continues on error", so issues often go unnoticed), two calls to expectDeprecation() will need to be removed.

I'd also like to suggest adding tests to cover this change.

For both, see PR #3186. You could probably cherrypick the test changes from that PR (and then add some more tests yourself for the other affected functions 😇 )

[jrf]

@ocean90 How can we move this forward ? Either your patch or mine (or a combination of the two) needs to be committed soonish, as it is a prerequisite for getting the PHP 8.1 build to pass.

[SergeyBiryukov]

In 54349:

Code Modernization: Use correct default value for JavaScript translations path.

The $path parameter of load_script_textdomain() had a default value of null, but would be passed onto the untrailingslashit() function without any input validation, even though the latter explicitly only expects/supports a string input.

This commit changes the default value for $path to an empty string, and adds an is_string() check before passing the value to untrailingslashit() to fix the issue at the point where the invalid input is incorrectly (not) validated.

Note: Changing the untrailingslashit() function is outside the scope of this commit.

Includes:

• Adding a dedicated unit test for this issue.
• Correcting the default value for $path from null to an empty string in a few related methods and functions:
  • WP_Dependency::set_translations()
  • WP_Scripts::set_translations()
  • wp_set_script_translations()
  • load_script_textdomain()

This fix also allows to remove a couple of calls to expectDeprecation() in unrelated tests.

Fixes an error when running the test suite:

4) Tests_Dependencies_Scripts::test_wp_external_wp_i18n_print_order
rtrim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/formatting.php:2782
/var/www/src/wp-includes/l10n.php:1068
/var/www/src/wp-includes/class.wp-scripts.php:605
/var/www/src/wp-includes/class.wp-scripts.php:320
/var/www/src/wp-includes/class.wp-dependencies.php:136
/var/www/src/wp-includes/functions.wp-scripts.php:109
/var/www/tests/phpunit/tests/dependencies/scripts.php:1505
/var/www/tests/phpunit/includes/utils.php:436
/var/www/tests/phpunit/tests/dependencies/scripts.php:1507
/var/www/vendor/bin/phpunit:123

Follow-up to [44169], [44607], [51968].

Props jrf, ocean90, Chouby, swissspidy, lovor, iviweb, meysamnorouzi, DarkoG, oneearth27, SergeyBiryukov.
Fixes #55967. See #55656.

Thanks for the PR! Merged in r54349.

@SergeyBiryukov Is it just me or are some of the changes from this PR missing from the commit ?

Agree with @jrfnl.

Only i10n.php made it to SVN; the others were omitted.

Is it just me or are some of the changes from this PR missing from the commit ?

Ah yes, you're right. I went back and forth on whether to make two separate commits or combine them, chose the latter option, but then forgot to include these files. Will do a follow-up commit shortly. Thanks for flagging this!

Appreciated @SergeyBiryukov !

When the other changes from this PR will be committed, then the change from my PR to the ​`src/wp-includes/l10n.php` file can be removed.

When the other changes from this PR will be committed, then the change from my PR to the ​`src/wp-includes/l10n.php` file can be removed.

I thought that's still a good change to include just in case a non-string $path is passed to the function, but happy to remove if that's preferred 🙂

When the other changes from this PR will be committed, then the change from my PR to the ​`src/wp-includes/l10n.php` file can be removed.

I thought that's still a good change to include just in case a non-string $path is passed to the function, but happy to remove if that's preferred 🙂

The problem is that it would _hide_ an error which should be _fixed_ (at the source of the problem) instead.

[SergeyBiryukov]

In 54351:

I18N: Use correct default value for JavaScript translations path.

The $path parameter of some script translation functions had a default value of null, even though the parameter is documented as a string.

This commit corrects the default value for $path in:

• WP_Dependency::set_translations()
• WP_Scripts::set_translations()
• wp_set_script_translations()

Additionally, this commit removes an is_string() check for $path in load_script_textdomain(). Now that the default value for $path in that function has also been corrected to an empty string instead of null, that check is no longer necessary, as it would hide an error which should be fixed (at the source of the problem) instead.

Follow-up to [54349].

Props jrf, johnjamesjacoby.
See #55967, #55656.

Merged the remaining changes in r54351. Thanks again!

Thanks for handling this @SergeyBiryukov !

[swissspidy]

#56735 was marked as a duplicate.