WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 5 months ago

#29722 closed defect (bug) (wontfix)

wp_localize_script should be able to manage scalars

Reported by: Fab1en Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.3
Component: Script Loader Keywords: needs-docs has-unit-tests needs-patch
Focuses: javascript Cc:

Description

We should be able to do this :

wp_localize_script( 'twentyfourteen-script', 'localized_variable', 1 );

As of version 4.0, this is working : it generates a JavaScript scalar value, but a PHP Warning is thrown :
Warning: Cannot use a scalar value as an array in /server/wordpress/versions/git-repo/src/wp-includes/class.wp-scripts.php on line 167
Note that the correct JavaScript code is output :

<script type='text/javascript'>
/* <![CDATA[ */
var localized_variable = 1;
/* ]]> */
</script>

The warning is not triggered when the scalar variable is a string.

wp_localize_script( 'twentyfourteen-script', 'localized_variable', 'a string' );

This is just because strings can be accessed like arrays, but this is not the intended effect.

A simple test for scalar values can fix that.

Attachments (4)

29722.patch (493 bytes) - added by Fab1en 7 years ago.
Avoid warning when scalar value is passed
29722.2.patch (857 bytes) - added by Fab1en 7 years ago.
Avoid warning when scalar value is passed but decode HTML entities
29722.3.diff (2.6 KB) - added by jtsternberg 6 years ago.
allow scalars. With supporting unit test
29722.4.diff (5.0 KB) - added by jtsternberg 6 years ago.
Add documentation and recursively html_entity_decode array values

Download all attachments as: .zip

Change History (40)

@Fab1en
7 years ago

Avoid warning when scalar value is passed

@Fab1en
7 years ago

Avoid warning when scalar value is passed but decode HTML entities

#1 @Fab1en
7 years ago

This ticket and the patch 29722.2.patch are linked to #25280

#2 @kitchin
7 years ago

To document this in wp_localize_script() I ran some tests. Inline doc would go:

-  * @param array $l10n         The data itself. The data can be either a single or multi-dimensional array.
+  * @param mixed $l10n         The data to pass through to json_encode().
                                If the data is a string, html entities are decoded.
                                If the data is an array, first dimension scalar values
                                are converted to string and html decoded.

Here is how the patch changes current behavior:

No change:

 null, bool(true), integer, float, array

Changes:

#1. var_dump(v): bool(false)

(was) $script : var v = [""];
(now) $script : var v = false;

#2. var_dump(v): string(19) "Save &amp; Activate"
(was) $script : var v = "Save &amp; Activate";
(now) $script : var v = "Save & Activate";

#3. var_dump(v): object(stdClass)#1 (1) {
  ["foo"]=>
  string(3) "bar"
}
(was) [die]
(now) $script : var v = {"foo":"bar"};

The WP calls in script-loader.php are not affected because they all use arrays. One oddball uses a multidimensional array. Another oddball wraps the array elements in esc_attr() which could be nixed:

did_action( 'init' ) && $scripts->localize( 'quicktags', 'quicktagsL10n', array(
	'closeAllOpenTags' => esc_attr(__('Close all open tags')),
	...
Last edited 7 years ago by kitchin (previous) (diff)

#3 follow-up: @kitchin
7 years ago

I had to edit my inline doc above. It is quite a custom function. This part in particular might be up for debate, but matches current behavior:

If the data is an array, first dimension scalar values are converted to string and html decoded.

#4 in reply to: ↑ 3 @Fab1en
7 years ago

Ideally, I would like to remove the "first dimension scalar values are converted to string" part, in relation to ticket #25280. Only strings should be "html decoded".

Replying to kitchin:

I had to edit my inline doc above. It is quite a custom function. This part in particular might be up for debate, but matches current behavior:

If the data is an array, first dimension scalar values are converted to string and html decoded.

#5 @kitchin
7 years ago

I'm out of time, but yeah, this is the wrong fix to be compatible with https://core.trac.wordpress.org/ticket/25280 and its tests AFAICT. I think you want:

	if ( is_array($l10n) ) {
		foreach ( $l10n as $key => $value ) {
			if ( is_string($value) ) {
				$l10n[$key] = html_entity_decode( $value, ENT_QUOTES, 'UTF-8');
			}
		}
	} elseif ( is_string($l10n) ) {
		$l10n = html_entity_decode( $l10n, ENT_QUOTES, 'UTF-8');
	}

And object users can now bypass it all!

Last edited 7 years ago by kitchin (previous) (diff)

#6 @kitchin
7 years ago

Should satisfy:

 * @param mixed $l10n         The data to pass through to json_encode().
                              If the data is a string, html entities are decoded.
                              If the data is an array, first dimension string values 
                              are html entity decoded.

#7 @boonebgorges
7 years ago

  • Version changed from trunk to 3.3

@jtsternberg
6 years ago

allow scalars. With supporting unit test

#8 @jtsternberg
6 years ago

  • Keywords has-patch dev-feedback 2nd-opinion added
  • Type changed from enhancement to defect (bug)

Big +1 here. Scalars should work (they actually do, but not w/o warnings). I've attached a patch (29722.3.diff) which allows scalars, and adds a supporting unit test. Changing this to a defect/bug because it's actually a bug in the way it's written (looping through $l10n by casting $l10n to an array but not checking if it's an array before stuffing an array key/value).

#9 follow-up: @boonebgorges
6 years ago

  • Keywords needs-docs added; dev-feedback 2nd-opinion removed

Scalars should work (they actually do, but not w/o warnings).

Changing this to a defect/bug because it's actually a bug in the way it's written (looping through $l10n by casting $l10n to an array but not checking if it's an array before stuffing an array key/value).

They almost work. The $l10n[$key] bit means that HTML entities in scalars aren't currently decoded. The fact that this use throws a PHP notice is probably helpful to developers, as it lets them know that the decoding/sanitization is not taking place properly.

I don't see the huge hardship in just passing a singleton array as $l10n :) but I don't see any harm in accepting scalars here. The patch looks OK to me, but we need to have improved documentation - the docs for wp_localize_script() are already subpar, and this syntax enhancement makes it even less clear how the function is intended to be used.

#10 in reply to: ↑ 9 @jtsternberg
6 years ago

Replying to boonebgorges:

Scalars should work (they actually do, but not w/o warnings).

Changing this to a defect/bug because it's actually a bug in the way it's written (looping through $l10n by casting $l10n to an array but not checking if it's an array before stuffing an array key/value).

They almost work. The $l10n[$key] bit means that HTML entities in scalars aren't currently decoded. The fact that this use throws a PHP notice is probably helpful to developers, as it lets them know that the decoding/sanitization is not taking place properly.

Ah, yes, I missed that part (that the value was not being decoded/sanitized).

I don't see the huge hardship in just passing a singleton array as $l10n :) but I don't see any harm in accepting scalars here. The patch looks OK to me, but we need to have improved documentation - the docs for wp_localize_script() are already subpar, and this syntax enhancement makes it even less clear how the function is intended to be used.

I'll add additional documentation, but I also discovered another path where the values would not be decoded/sanitized, and that is if $l10n had a multi-dimensional array. Any values deeper than the first level would be left untouched. This is because of the if ( ! is_scalar( $value ) ) { continue; } check in the loop. I'm thinking maybe we want to do the html_entity_decode( (string) $value, ENT_QUOTES, 'UTF-8') recursively? If you agree, I can update this patch to create a recursive method for doing so.

#11 follow-up: @boonebgorges
6 years ago

I'm thinking maybe we want to do the html_entity_decode( (string) $value, ENT_QUOTES, 'UTF-8') recursively?

Groan :) If you can recurse without losing performance for one-dimensional arrays, then OK.

#12 in reply to: ↑ 11 ; follow-up: @jtsternberg
6 years ago

Replying to boonebgorges:

I'm thinking maybe we want to do the html_entity_decode( (string) $value, ENT_QUOTES, 'UTF-8') recursively?

Groan :) If you can recurse without losing performance for one-dimensional arrays, then OK.

I don't believe it will be an issue unless you believe calling in_array for each value in a one-dimensional array will cause significant performance loss. The other option is to to check something like $is_multi = is_array( array_values( $l10n1 )[0] ), but to determine which was more performant would def. require some benchmarks. I would not be a bit surprised if multi-dimensional arrays were often used.

#13 in reply to: ↑ 12 @jtsternberg
6 years ago

Replying to jtsternberg:

The other option is to to check something like $is_multi = is_array( array_values( $l10n1 )[0] )

Derp, that will only check if the first item in the array is an array. Looks like we'll have to check is_array on every value.

@jtsternberg
6 years ago

Add documentation and recursively html_entity_decode array values

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


5 years ago

#15 @swissspidy
5 years ago

  • Component changed from I18N to Script Loader

#16 @adamsilverstein
4 years ago

I added a new patch on 25280 that proposes adding a new helper that bundles the code and tests from the latest patch here.

#17 @johnbillion
3 years ago

#34590 was marked as a duplicate.

#18 @SergeyBiryukov
11 months ago

Related: #48528, #51108.

Just noting that passing a string value to wp_localize_script() is now causing a warning in PHP 8.0 Beta 2:

Warning: Only the first byte will be assigned to the string offset in wp-includes/class.wp-scripts.php on line 492

#19 @SergeyBiryukov
11 months ago

  • Keywords php8 added

#20 @andraganescu
11 months ago

A quick fix for PHP 8 is to do the array casting before the foreach loop, in /wp-includes/class.wp-scripts.php:localize like:

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

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

But it seems so "hacky" :)

#21 @desrosj
11 months ago

  • Keywords needs-dev-note added

This may be worth mentioning PHP 8 dev note in the context of what developers need to know as warning swill now be thrown.

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


10 months ago

#23 @hellofromTonya
10 months ago

  • Milestone changed from Awaiting Review to 5.6

Per yesterday's scrub conversation in slack, this ticket is part of the PHP8 5.6 scope. Moving into the milestone.

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


9 months ago

#25 @johnbillion
9 months ago

#48528 was marked as a duplicate.

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


9 months ago

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


9 months ago

This ticket was mentioned in PR #744 on WordPress/wordpress-develop by draganescu.


9 months ago

  • Keywords has-unit-tests added

## Description

  • Refreshed the latest patch on 29722
  • Passing scalars just outputs them,
  • Passing strings now also works on PHP 8.

## Reference

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

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


8 months ago

#30 @TimothyBlynJacobs
8 months ago

Has this been looked at in the context of #51159?

#31 @hellofromTonya
8 months ago

Notes from conversation with @SergeyBiryukov

I didn't have a chance to review 29722 yet, will try to catch up with the discussion today before the party. I have no objections for it to continue into RC, it might seem a bit late, but if it helps prevent some PHP 8 breakage, still might be a good fit for this release.

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


8 months ago

#33 @helen
8 months ago

  • Milestone changed from 5.6 to 5.7

It's late to be committing something that's potentially an unexpected change in that scalar values will now be included and decoded whereas previously they were skipped. Consider this scenario from @TimothyBlynJacobs and whether its outcome would change with this patch:

wp_localize_script( 'handle', 'LOCALIZED', [
	'input' => [
		'attr' => esc_html( get_unsafe_value() )
	]
] );
?>
<script type="application/javascript">
	var html = '<p attr="' + LOCALIZED.input.attr + '">'
</script>

Defensive programming to account for PHP 8 compat with plugins/themes/the broader ecosystem is something we're targeting for a later release, so I'm punting from 5.6 because this isn't an issue that core triggers in itself but rather is a potential issue with custom code.

#34 @azaozz
7 months ago

  • Keywords needs-patch added; has-patch removed

Been chatting with @andraganescu about what's the right solution here.

This ticket is a partial duplicate of #25280. Specifically see this comment: https://core.trac.wordpress.org/ticket/25280#comment:45. In short:

  1. wp_localize_script() should only be used to localize scripts, not to pass arbitrary data to them. There is wp_add_inline_script() for that.
  2. For translations wp_localize_script() and the underlaying WP_Scripts::localize() have been phased out in core, in favor of WP_Scripts::set_translations().

In that terms thinking this ticket should be "wontfix", same as #25280. The only thing that needs fixing is the PHP 8.0 bug, and perhaps some more inline docs to explain why not to use this function for arbitrary data.

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


6 months ago

#36 @peterwilsoncc
5 months ago

  • Keywords php8 needs-dev-note removed
  • Milestone 5.7 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

I agree with @azaozz that this is a wontfix for the reasons stated about the intent of script localization.

I'm going to close this as such and have opened #52534 for managing the PHP 8.0 warnings.

Docblock improvements can be done on a "Docblock improvements for ..." ticket such as #51800.

Note: See TracTickets for help on using tickets.