WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 18 months ago

#29722 new defect (bug)

wp_localize_script should be able to manage scalars

Reported by: Fab1en Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.3
Component: Script Loader Keywords: has-patch needs-docs
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 5 years ago.
Avoid warning when scalar value is passed
29722.2.patch (857 bytes) - added by Fab1en 5 years ago.
Avoid warning when scalar value is passed but decode HTML entities
29722.3.diff (2.6 KB) - added by jtsternberg 4 years ago.
allow scalars. With supporting unit test
29722.4.diff (5.0 KB) - added by jtsternberg 4 years ago.
Add documentation and recursively html_entity_decode array values

Download all attachments as: .zip

Change History (21)

@Fab1en
5 years ago

Avoid warning when scalar value is passed

@Fab1en
5 years ago

Avoid warning when scalar value is passed but decode HTML entities

#1 @Fab1en
5 years ago

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

#2 @kitchin
5 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 5 years ago by kitchin (previous) (diff)

#3 follow-up: @kitchin
5 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
5 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
5 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 5 years ago by kitchin (previous) (diff)

#6 @kitchin
5 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
5 years ago

  • Version changed from trunk to 3.3

@jtsternberg
4 years ago

allow scalars. With supporting unit test

#8 @jtsternberg
4 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
4 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
4 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
4 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
4 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
4 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
4 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.


3 years ago

#15 @swissspidy
3 years ago

  • Component changed from I18N to Script Loader

#16 @adamsilverstein
3 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
18 months ago

#34590 was marked as a duplicate.

Note: See TracTickets for help on using tickets.