Make WordPress Core

Opened 2 years ago

Closed 20 months ago

Last modified 20 months ago

#55967 closed defect (bug) (fixed)

Scripts: Ensure path for JavaScript translations is always a string

Reported by: ocean90's profile ocean90 Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: has-patch php81
Focuses: Cc:

Description

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

Change History (19)

#2 @SergeyBiryukov
2 years ago

  • Keywords php81 added

#3 @SergeyBiryukov
23 months ago

#56191 was marked as a duplicate.

#4 @ocean90
21 months ago

#56393 was marked as a duplicate.

jrfnl commented on PR #2801:


21 months ago
#5

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

#6 @jrf
20 months ago

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

Last edited 20 months ago by jrf (previous) (diff)

#7 @SergeyBiryukov
20 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#8 @SergeyBiryukov
20 months ago

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

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.

SergeyBiryukov commented on PR #2801:


20 months ago
#9

Thanks for the PR! Merged in r54349.

jrfnl commented on PR #2801:


20 months ago
#10

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

JJJ commented on PR #2801:


20 months ago
#11

Agree with @jrfnl.

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

https://i0.wp.com/user-images.githubusercontent.com/88951/192866767-7d522f64-0c95-4a70-9010-1554f648adc2.png

SergeyBiryukov commented on PR #2801:


20 months ago
#12

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!

jrfnl commented on PR #2801:


20 months ago
#13

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.

SergeyBiryukov commented on PR #2801:


20 months ago
#14

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 🙂

jrfnl commented on PR #2801:


20 months ago
#15

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.

#16 @SergeyBiryukov
20 months ago

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.

SergeyBiryukov commented on PR #2801:


20 months ago
#17

Merged the remaining changes in r54351. Thanks again!

jrfnl commented on PR #2801:


20 months ago
#18

Thanks for handling this @SergeyBiryukov !

#19 @swissspidy
20 months ago

#56735 was marked as a duplicate.

Note: See TracTickets for help on using tickets.