wp_localize_script should be able to manage scalars

[Fab1en]

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.

[Fab1en]

Avoid warning when scalar value is passed

[Fab1en]

Avoid warning when scalar value is passed but decode HTML entities

[Fab1en]

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

[kitchin]

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 string values are 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 & Activate"
(was) $script : var v = "Save & 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 to protect an integer from getting made into a string by the intended old behavior (which would not happen anyway because of the bug). 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')),
	...

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

[Fab1en]

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.

[kitchin]

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!

[kitchin]

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.

[jtsternberg]

allow scalars. With supporting unit test

[jtsternberg]

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

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

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.

[jtsternberg]

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.

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

[jtsternberg]

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.

[jtsternberg]

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]

Add documentation and recursively html_entity_decode array values

[adamsilverstein]

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

[johnbillion]

#34590 was marked as a duplicate.

[SergeyBiryukov]

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

[andraganescu]

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" :)

[desrosj]

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

[hellofromTonya]

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

[johnbillion]

#48528 was marked as a duplicate.

## 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

[TimothyBlynJacobs]

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

[hellofromTonya]

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.

[helen]

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.

[azaozz]

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.

[peterwilsoncc]

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.