Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#52534 closed defect (bug) (fixed)

PHP 8: wp_localize_script() throws a warning if third parameter is a string.

Reported by: peterwilsoncc's profile peterwilsoncc Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: php8 has-patch has-unit-tests dev-reviewed commit
Focuses: Cc:

Description (last modified by peterwilsoncc)

Calling wp_localize_script() will throw a warning in PHP 8.0 if the third parameter (localization object) is a string:

<?php
wp_localize_script( 'jquery', 'jqueryL10n', __( 'Some string' ) );

PHP 8.0 Warning:

[16-Feb-2021 03:02:09 UTC] PHP Warning:  Only the first byte will be assigned to the string offset in /vagrant/wordpress-develop/src/wp-includes/class.wp-scripts.php on line 492
[16-Feb-2021 03:02:09 UTC] PHP Stack trace:
[16-Feb-2021 03:02:09 UTC] PHP   1. {main}() /vagrant/wordpress-develop/src/wp-admin/admin-ajax.php:0
[16-Feb-2021 03:02:09 UTC] PHP   2. require_once() /vagrant/wordpress-develop/src/wp-admin/admin-ajax.php:22
[16-Feb-2021 03:02:09 UTC] PHP   3. require_once() /vagrant/wordpress-develop/src/wp-load.php:37
[16-Feb-2021 03:02:09 UTC] PHP   4. include() /vagrant/wordpress-develop/src/wp-config.php:9
[16-Feb-2021 03:02:09 UTC] PHP   5. require_once() /vagrant/wp-config.php:152
[16-Feb-2021 03:02:09 UTC] PHP   6. do_action($tag = 'init') /vagrant/wordpress-develop/src/wp-settings.php:560
[16-Feb-2021 03:02:09 UTC] PHP   7. WP_Hook->do_action($args = [0 => '']) /vagrant/wordpress-develop/src/wp-includes/plugin.php:484
[16-Feb-2021 03:02:09 UTC] PHP   8. WP_Hook->apply_filters($value = '', $args = [0 => '']) /vagrant/wordpress-develop/src/wp-includes/class-wp-hook.php:316
[16-Feb-2021 03:02:09 UTC] PHP   9. PWCC\{closure:/vagrant/content/mu-plugins/pwcc-cpts.php:25-27}('') /vagrant/wordpress-develop/src/wp-includes/class-wp-hook.php:292
[16-Feb-2021 03:02:09 UTC] PHP  10. wp_localize_script($handle = 'jquery', $object_name = 'jqueryL10n', $l10n = 'Some string') /vagrant/content/mu-plugins/pwcc-cpts.php:26
[16-Feb-2021 03:02:09 UTC] PHP  11. WP_Scripts->localize($handle = 'jquery', $object_name = 'jqueryL10n', $l10n = 'Some string') /vagrant/wordpress-develop/src/wp-includes/functions.wp-scripts.php:221

---

wp_localize_script() is known to work with the following formats:

  • associative arrays
  • indexed arrays
  • multidimensional arrays
  • strings

Follow up to #29722

Attachments (2)

52534.diff (3.3 KB) - added by SergeyBiryukov 3 years ago.
52534-with-bc-break.patch (5.0 KB) - added by jrf 3 years ago.
Combined commits from PR 1022 - this patches the issue without BC breaks

Download all attachments as: .zip

Change History (19)

#1 @peterwilsoncc
3 years ago

  • Description modified (diff)

This ticket was mentioned in PR #1007 on WordPress/wordpress-develop by peterwilsoncc.


3 years ago
#2

  • Keywords has-patch has-unit-tests added

#3 @peterwilsoncc
3 years ago

In the pull request:

  • added some tests with strings and arrays
  • I would like to test some of the items that throw (passing stdClass) but not sure how best to test for exceptions
  • added an is_string() check before looping assuming the l10n data is an array
  • convert $l10n to an array rather than only doing so for the loop

#4 @jrf
3 years ago

@peterwilsoncc I had a quick look, but to be honest, that warning seems 100% correct behaviour and warranted.

Both the documentation of `wp_localize_script()` as well as the docs for `WP_Scripts::localize()` state explicitly that the third parameter is (should be) an array, either single or multi-dimensional.

In other words, passing a string as the $l10n parameter is a programmer error and unsupported behaviour, even though WP is "tolerant" of this by doing an (array) $l10n in the foreach() in WP_Scripts::localize().

Warnings like this exist for good reason: to warn programmers about errors they make before those errors can make it into production. They should not be avoided.

If anything, the tolerance for passing the third parameter as a string should be removed and replaced by a doing it wrong warning, but that would break BC...

So, as the warning is kind of obscure, let's look at what the actual problem is.
The problem is in this code snippet - src/wp-includes/class.wp-scripts.php - line 487 - 493:

<?php
foreach ( (array) $l10n as $key => $value ) {
        if ( ! is_scalar( $value ) ) {
                continue;
        }

        $l10n[ $key ] = html_entity_decode( (string) $value, ENT_QUOTES, 'UTF-8' );
}

What is happening is that foreach() operates on a copy of the array and that copy is the one which is cast to array here: foreach ( (array) $l10n as $key => $value ).
But then on line 492, $l10n[ $key ] = html_entity_decode(...), an assignment is made to the original parameter, which is still a string, which is the bit which causes the failure.

The simple solution to this would be to just cast the $l10n parameter to array ahead of the foreach.

<?php
$$l10n = (array) $l10n;
foreach ( $l10n as $key => $value ) {
        if ( ! is_scalar( $value ) ) {
                continue;
        }

        $l10n[ $key ] = html_entity_decode( (string) $value, ENT_QUOTES, 'UTF-8' );
}

Adding any extra code to add support for passing the parameter in an unsupported variable type, should IMO be avoided.

I mean, seriously, this is going down a road where WP would accept any parameter type instead of the documented one and will try to make do with it by casting and juggling and doing all sorts of magic tricks, which is a never-ending road to incredible torture and pain, quite apart from the runtime impact all this type of "overhead" code would have...

P.S.: Very happy to see the tests, though I'd love it if the string test could be changed to a hard failure... 😇

#5 @SergeyBiryukov
3 years ago

I tend to agree with @jrf, if both #25280 and #29722 were a wontfix, it seems like a better _doing_it_wrong() message instead of a somewhat obscure warning would be the preferred path here.

My concern with the current patch is that it attempts to work around the issue for strings, but unnecessarily breaks backward compatibility for integers, converting them to an array instead:

1) Tests_Dependencies_Scripts::test_wp_localize_script_data_formats with data set #2 (1, '"1"')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 '<script type='text/javascript' id='test-example-js-extra'>
 /* <![CDATA[ */
-var testExample = "1";
+var testExample = ["1"];
 /* ]]> */
 </script>
 <script type='text/javascript' src='http://example.com' id='test-example-js'></script>
 '

Previously, as pointed out in the tickets above, passing an integer also caused a PHP warning, but otherwise worked as expected, though the value was ultimately converted to a string.

It seems to me that both strings and integers should be handled consistently here, either by replacing the is_string() check with is_scalar(), to match the check within the loop and in _WP_Dependency::add_data(), or explicitly checking for ! is_array(), which might be even clearer.

See 52534.diff, which adds a test case for an integer, and also a clear _doing_it_wrong() message pointing to wp_add_inline_script() as the alternative.

#6 @jrf
3 years ago

I have had another look and aside from what I said before, both currently proposed patches (as well as the one I proposed above) all constitute BC breaks in one way or another in the output to JS, which in turn can have side-effects and cause quite a lot of follow-on bugs.

See: https://3v4l.org/ODNk5

So realistically, I think that the only course of action open to us without introducing BC-breaks is to add a _doing_it_wrong warning and explicitly handle strings, but not change the behaviour in any way for other types.
For any non-string, non-array input, that means there will be PHP notices/warnings, but as there will be a helpful "doing it wrong" and we're talking programmer errors anyway, I think that is acceptable.

Something along the lines of:

<?php
if ( ! is_array( $l10n ) ) {
        // Throw the _doing_it_wrong_warning.

}

if ( is_string( $l10n ) ) {
        $l10n = html_entity_decode( (string) $l10n, ENT_QUOTES, 'UTF-8' );
} else {        
        foreach ( (array) $l10n as $key => $value ) {
                if ( ! is_scalar( $value ) ) {
                        continue;
                }

                $l10n[ $key ] = html_entity_decode( (string) $value, ENT_QUOTES, 'UTF-8' );
        }
}

@SergeyBiryukov
3 years ago

#7 @peterwilsoncc
3 years ago

Thanks for the prompt feedback Juliette and Sergey.

I've updated my linked PR with 52534.diff, the results appear to be the same on both the main branch and the feature branch. I'll throw various version of PHP at in on my local machine and do some manual testing too.

The PR is set to allow maintainers to push: if either of you want to push to my branch feel free if you'd prefer to bypass uploading patches to trac.

#8 @jrf
3 years ago

Thanks @peterwilsoncc

The problem is that even with the current patch, the behaviour is changed, like I pointed out in https://core.trac.wordpress.org/ticket/52534#comment:6.

What I think we need to do is:

  • Have a clean commit which only adds tests for the existing behaviour (which should pass on current master).
  • Then have a second commit which adds the _doing_it_wrong message and adjusts the test to expect that message when appropriate. Other than potentially setting a $unsupported parameter, the data provider should not change in that commit.

This is similar to how you originally set up your PR, but has become muddled now.

As things stand, the first commit (clean tests) would not pass with the current data set.

This ticket was mentioned in PR #1022 on WordPress/wordpress-develop by jrfnl.


3 years ago
#9

Trac ticket: https://core.trac.wordpress.org/ticket/52534
Replaces patch PR: #1007

These tests will pass on PHP < 8.
On PHP 8, only the Only the first byte will be assigned to the string offset warning for the string test should fail.

Co-authored-by: Peter Wilson <519727+peterwilsoncc@…>
Co-authored-by: Sergey Biryukov <sergeybiryukov@…>

#10 @jrf
3 years ago

I've just opened PR 1022 to do what I propose above. I'll add the "fix" commit once the CI runs for the "test" commit have finished, so both runs can be examined.

@jrf
3 years ago

Combined commits from PR 1022 - this patches the issue without BC breaks

#11 follow-up: @jrf
3 years ago

Argh... please disregard the typo in the patch name - with should be without. It's only the name which is borked, not the patch 🤐

peterwilsoncc commented on PR #1007:


3 years ago
#12

Closing in favour of PR #1022.

#13 in reply to: ↑ 11 @peterwilsoncc
3 years ago

Replying to jrf:

Argh... please disregard the typo in the patch name - with should be without. It's only the name which is borked, not the patch 🤐

Haha, fortunately I have never made a similar mistake. Well... not today, anyway. 🤣

The changes in 52534-with-bc-break.patch look good to me. The tests show nothing has changed between trunk and with the patch.

@SergeyBiryukov @jrf I think this is safe to go in to 5.7 but as the first RC is only a couple of days away, could one of you sign off on that.

#14 follow-up: @jrf
3 years ago

  • Keywords dev-reviewed commit added
  • Milestone changed from Awaiting Review to 5.7

I think this is safe to go in to 5.7 but as the first RC is only a couple of days away, could one of you sign off on that.

IMO, between the three of us, we came up with a clean solution to the issue. The current patch is ready to be committed and doesn't contain any functional changes, nor BC-breaks and has tests which confirm that.

It does add a _doing_it_wrong() notice, but that only shows up when devs were already doing it wrong and "doing it wrong" notices only show when WP_DEBUG is turned on, so would not show with the default (end-user) WP_DEBUG settings.

I also think this patch is a candidate for backporting to the 5.6 branch as it relates to a PHP warning.

@peterwilsoncc Just checking: is there any specific keyword I need to add for the "second committer sign-off"? I can't seem to find the docs about it.

#15 in reply to: ↑ 14 @SergeyBiryukov
3 years ago

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

Replying to jrf:

Just checking: is there any specific keyword I need to add for the "second committer sign-off"? I can't seem to find the docs about it.

dev-reviewed is the one, as described in Trac Workflow Keywords. Thanks!

#16 @peterwilsoncc
3 years ago

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

In 50408:

Script Loader: Prevent wp_localize_script() warnings.

Prevent wp_localize_script() (via WP_Scripts::localize()) throwing warnings in PHP 8 when the translation data is passed as a string. This maintains backward compatibility with earlier versions of PHP.

Introduce a _doing_it_wrong() notice to WP_Scripts::localize() if the translation data is not passed as an array.

Props jrf, peterwilsoncc, SergeyBiryukov.
Fixes #52534.

Note: See TracTickets for help on using tickets.