Make WordPress Core

Opened 11 years ago

Closed 5 years ago

Last modified 4 years ago

#25280 closed enhancement (wontfix)

wp_localize_script unexpectedly converts numbers to strings

Reported by: adamsilverstein's profile adamsilverstein Owned by: adamsilverstein's profile adamsilverstein
Milestone: Priority: normal
Severity: normal Version: 3.2
Component: Script Loader Keywords: has-patch dev-feedback has-unit-tests
Focuses: javascript Cc:

Description

Running wp_localize_script on an an array has the unexpected result of converting numbers into strings. Core WordPress files work around this issue by explicitly casting variables to int, for example in revisions.js.

Although it is easy to work around the issue (if you are expecting it), JavaScript is loosely typed and will merrily add numbers as strings without any warning - introducing potential bugs when code that expects a number is instead passed a string. Properly passing numbers as numbers would resolve this issue.

The root of the problem is that in PHP versions before 5.3.3, json_encode encoded numbers as strings. PHP 5.3.3 includes an option JSON_NUMERIC_CHECK that eliminates this issue.

For example & testing this bug, add the following code to your template's setup function:

	wp_enqueue_script( 'localize_test', get_template_directory_uri() . '/localize_script.js', array(), '1.0.0', true );
	$array_to_localize = array( 
		'a_string' => __( 'Some String' ),
		'a_number' => 10,
		);

	wp_localize_script( 'localize_test', 'localized_variable', $array_to_localize );

Then add a file named localize_script.js in your theme folder containing the following code:

console.log(localized_variable);

Reloading shows that the number has been converted into a String (console screenshot).

The attached patch corrects the issue by checking for the presence of the JSON_NUMERIC_CHECK constant and if present, passes it to json_encode - telling it to encode numbers as numbers and resulting in the expected behavior (console screenshot). I checked this under PHP 5.2.17 verifying that no errors are created (although the old behavior of converting numbers still occurs). Under PHP 5.3.3 the variable is now properly passed as a number. The Codex documentation needs to be updated if this patch is accepted.

Attachments (10)

25280.diff (682 bytes) - added by adamsilverstein 11 years ago.
correct wp_localize_script encoding numbers as strings
25280.2.patch (737 bytes) - added by c3mdigital 11 years ago.
Don't run booleans through html_entity_decode() cast as a string
25280.patch (737 bytes) - added by adamsilverstein 11 years ago.
update against trunk
2580.3.patch (584 bytes) - added by c3mdigital 11 years ago.
Only run strings through html_entity_decode
25280.4.patch (829 bytes) - added by Fab1en 10 years ago.
Allow wp_localize_script to use scalar variable
25280.2.diff (1.5 KB) - added by adamsilverstein 10 years ago.
add unit test
25280.3.patch (1.5 KB) - added by adamsilverstein 10 years ago.
auto name try 2
25280.5.patch (1.5 KB) - added by adamsilverstein 10 years ago.
refresh against trunk, code standards cleanup
25280.3.diff (6.5 KB) - added by adamsilverstein 8 years ago.
25280.4.diff (6.5 KB) - added by adamsilverstein 7 years ago.

Download all attachments as: .zip

Change History (59)

@adamsilverstein
11 years ago

correct wp_localize_script encoding numbers as strings

#1 @c3mdigital
11 years ago

  • Keywords close added; dev-feedback removed

It also converts booleans to strings as well and if you don't have control of the javascript reading the vars you can't use numbers or booleans.

json_encode is not why numbers are converted to strings. They are cast as a string on purpose:

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

I know the general accepted practice is to use wp_localize_script to pass variables to javascript but that is not what the function was intended for (it's a bonus feature and I blame Otto's awesome blog post). It's for string translations and I don't think it should be changed because numbers don't need to be translated.

I'm thinking wontfix.

Last edited 11 years ago by c3mdigital (previous) (diff)

#2 follow-up: @c3mdigital
11 years ago

Also to note the way JSON_NUMERIC_CHECK handles booleans.

wp_localize_script( 'testing', 'TESTING', array( 'false' => false, 'true' => true, 'integer' => 234 ) );

Before patch:
var TESTING = {"false":"","true":"1","integer":"234"};

after patch:
var TESTING = {"false":"","true":1,"integer":234};

Last edited 11 years ago by c3mdigital (previous) (diff)

#3 in reply to: ↑ 2 ; follow-up: @adamsilverstein
11 years ago

Replying to c3mdigital:

Also to note the way JSON_NUMERIC_CHECK handles booleans.

wp_localize_script( 'testing', 'TESTING', array( 'false' => false, 'true' => true, 'integer' => 234 ) );

Before patch:
var TESTING = {"false":"","true":"1","integer":"234"};

after patch:
var TESTING = {"false":"","true":1,"integer":234};

interesting points, thanks for the feedback! the boolean evaluation in JS won't change, right? "1" and 1 are both truthy.

the patch does not rely on PHP 5.3; instead it improves the behavior of if 5.3 is available. in 5.2 nothing changes.

agree wp_localize_script is not intended for this purpose, but in practice thats how its used- even internally by core. if we aren't going to try to fix the function, how about adding a new function for passing non-string values from php to js - providing a standard method is better than leaving it up to developers in my opinion.

#4 in reply to: ↑ 3 @c3mdigital
11 years ago

Replying to adamsilverstein:

interesting points, thanks for the feedback! the boolean evaluation in JS won't change, right? "1" and 1 are both truthy.

the patch does not rely on PHP 5.3; instead it improves the behavior of if 5.3 is available. in 5.2 nothing changes.

agree wp_localize_script is not intended for this purpose, but in practice thats how its used- even internally by core. if we aren't going to try to fix the function, how about adding a new function for passing non-string values from php to js - providing a standard method is better than leaving it up to developers in my opinion.

Also those results were after the booleans and numbers were run through html_entity_decode cast as a string. I don't fully understand why scaler variables are run through html_entity_decode()?

I would love a better way to pass data to javascript as a script dependency. wp_localize_script would be fine for most use if there was a way to pass a real boolean. Maybe we could start with omitting booleans from the is_scalar check. I'm not against JSON_NUMERIC_CHECK but know core doesn't like to use stuff dependent on php 5.3 even if there is a sanity check.

#5 @c3mdigital
11 years ago

  • Keywords dev-feedback added; close removed
  • Type changed from defect (bug) to enhancement
  • Version changed from trunk to 3.2

Related: #19825, #11520

@c3mdigital
11 years ago

Don't run booleans through html_entity_decode() cast as a string

#6 @adamsilverstein
11 years ago

that looks reasonable, haven't checked locally: will that pass actual true and false to JS now?

@adamsilverstein
11 years ago

update against trunk

#7 @adamsilverstein
11 years ago

  • Keywords needs-testing added

#8 follow-up: @tomauger
11 years ago

I'm liking this fix, but actually agree with adam that it might be good to have an entirely new way of passing object dependencies into our JS. Right now I'm using a lot of JSDoc to document the "localize" global JS var, but it certainly would be nice to have something a little more robust.

Forking wp_localize_script() to wp_pass_vars_to_script() (for lack of a better name) seems like a pretty good (and backward-compatible) way to go.

#9 in reply to: ↑ 8 @adamsilverstein
11 years ago

Replying to tomauger:

I'm liking this fix, but actually agree with adam that it might be good to have an entirely new way of passing object dependencies into our JS. Right now I'm using a lot of JSDoc to document the "localize" global JS var, but it certainly would be nice to have something a little more robust.

Forking wp_localize_script() to wp_pass_vars_to_script() (for lack of a better name) seems like a pretty good (and backward-compatible) way to go.

I agree a new function probably makes the most sense, that would leave backward compatibility with what we have while allowing a more robust design. i believe this function was originally built with translation in mind and has obviously been used for other purposes to the point where it has become the recommended way to pass variables from PHP->JS in WordPress.

The attached patch corrects some unexpected behavior and might still be worth getting in - what would you envision a new function doing other than having a new name and passing numbers/booleans correctly? If you have some ideas, please respond or go ahead an open a new ticket proposing the new function!

Thanks :)

#10 @adamsilverstein
11 years ago

  • Summary changed from wp_localize_script unexpectedly convert numbers to strings to wp_localize_script unexpectedly converts numbers to strings

#11 @tomauger
11 years ago

I kind of blew my mind when I started modeling how to create a tighter coupling between PHP and JS, particularly in such a way that modern IDEs (I use Netbeans, for example) would be able to pick up on the dependencies and perhaps even offer code completion. At the moment, I can't think of anything more innovative than a spin-off of wp_localize_script(), which is frankly a pretty user-friendly way of communicating between the two "worlds".

#12 @c3mdigital
11 years ago

  • Keywords dev-feedback needs-testing removed

Latest patch looks good. Would love to see this get added to core.

#13 @wonderboymusic
11 years ago

  • Milestone changed from Awaiting Review to 4.0

PHP, ladies and gentlemen

@c3mdigital
11 years ago

Only run strings through html_entity_decode

#14 follow-up: @c3mdigital
11 years ago

As per discussion with @nacin, 2580.3.patch changes "if" check to only pass strings to html_entity_decode() and removes cast to string.

#15 in reply to: ↑ 14 @adamsilverstein
11 years ago

Replying to c3mdigital:

As per discussion with @nacin, 2580.3.patch changes "if" check to only pass strings to html_entity_decode() and removes cast to string.

This seems like a great solution. The sooner we get this in, the longer we have to test it :) My only concern is that we may break plugins that are expecting their numbers to be converted to strings. Not sure we can search for such breakage, and may have to be satisfied with making sure the change is clearly documented. Commit!

#16 @adamsilverstein
11 years ago

  • Focuses javascript added
  • Keywords dev-feedback added

Looks good, I verified this works correctly passing numbers as numbers and booleans as booleans.

I added the following code to my template:

wp_enqueue_script( 'localize_test', get_template_directory_uri() . '/localize_script.js', array(), '1.0.0', true ); 
	$array_to_localize = array(
		'a_string'  => __( 'Some String' ),
		'a_number'  => 10,
		't_boolean' => true,
		'f_boolean' => false,
		);
wp_localize_script( 'localize_test', 'localized_variable', $array_to_localize );

the created a localize_script.js file with the single line console.log( localized_variable );

result was as expected:
http://cl.ly/image/1N1T3a2O0J3B/revisions%20test%20%7C%20Local%20WordPress%20Trunk%20Dev%202014-05-07%2010-41-45%202014-05-07%2010-41-48.jpg

by comparison, before the patch is applied all values are passed as strings:
http://cl.ly/image/241S0m2j2c0Y/revisions%20test%20%7C%20Local%20WordPress%20Trunk%20Dev%202014-05-07%2010-42-40%202014-05-07%2010-42-41.jpg

Last edited 11 years ago by adamsilverstein (previous) (diff)

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


10 years ago

#18 follow-up: @helen
10 years ago

  • Keywords 4.1-early added
  • Milestone changed from 4.0 to Future Release

Given concerns about plugin breakage, punting and marking as early. Can possibly bring back with discussion, though. Sorry it didn't get attention earlier.

#19 in reply to: ↑ 18 @adamsilverstein
10 years ago

Replying to helen:

Given concerns about plugin breakage, punting and marking as early. Can possibly bring back with discussion, though. Sorry it didn't get attention earlier.

Agree this needs to go in early in a cycle to avoid potential conflicts and give developers time to adapt.

I don't think we'll see many issues, since mostly developers are forced to 'parseInt' numbers back to numbers when we expect them and they are passed as strings instead; if we pass actual ints, 'parseInt' should still work fine. Booleans might be more of an issue, still should be easy to fix.

If we want to avoid any chance of a conflict, we could add a new option to the function to turn on this new behavior (yuck) or add a new function entirely (ugh).

Once we do get this in, we should go thru every place we added parseInt in core and take them out.

#20 follow-up: @Fab1en
10 years ago

I think this fix is good. Before this fix, you would get the same behavior by wrapping your variables in another level of array (because the variable will fail the is_scalar test) :

wp_enqueue_script( 'localize_test', get_template_directory_uri() . '/localize_script.js', array(), '1.0.0', true ); 
	$array_to_localize = array(
		'a_string'  => __( 'Some String' ),
		'a_number'  => 10,
		't_boolean' => true,
		'f_boolean' => false,
		);
wp_localize_script( 'main', 'localized_variable', array('fixed' => $array_to_localize) );

So the fix will add consistency.

JavaScript provides type coercion depending on the context, so plugin breakage risks are really low. To break, a plugin would have to

  1. Expect a number as a string, but declare it as a number in PHP
  2. Use it in a context where it could be interpreted as a number without any check. (for example, using 'The value is ' + localized_variable.a_number would automatically cast the variable to a string).

#21 follow-up: @Fab1en
10 years ago

I thinks that another behavior is to be fixed at the same time : using a scalar variable.
We should be able to do this :

wp_localize_script( 'main', 'localized_variable', 'a scalar value' );

As of version 3.9.1, 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/projects/other/quatreheures/esope/htdocs/wp-includes/class.wp-scripts.php on line 167

I suggest to add another test to avoid this issue.

@Fab1en
10 years ago

Allow wp_localize_script to use scalar variable

#22 in reply to: ↑ 20 @adamsilverstein
10 years ago

Nice trick, thats a great suggestion for a workaround!

Replying to Fab1en:

I think this fix is good. Before this fix, you would get the same behavior by wrapping your variables in another level of array (because the variable will fail the is_scalar test) :

wp_enqueue_script( 'localize_test', get_template_directory_uri() . '/localize_script.js', array(), '1.0.0', true ); 
	$array_to_localize = array(
		'a_string'  => __( 'Some String' ),
		'a_number'  => 10,
		't_boolean' => true,
		'f_boolean' => false,
		);
wp_localize_script( 'main', 'localized_variable', array('fixed' => $array_to_localize) );

So the fix will add consistency.

JavaScript provides type coercion depending on the context, so plugin breakage risks are really low. To break, a plugin would have to

  1. Expect a number as a string, but declare it as a number in PHP
  2. Use it in a context where it could be interpreted as a number without any check. (for example, using 'The value is ' + localized_variable.a_number would automatically cast the variable to a string).

@adamsilverstein
10 years ago

add unit test

@adamsilverstein
10 years ago

auto name try 2

#23 @adamsilverstein
10 years ago

25280.3.patch

  • adds a unit test, verified ints and booleans passed properly
  • apologies for the funky naming
  • leaving out 25280.4.patch for now, passing scalar values outside scope of this ticket

#24 in reply to: ↑ 21 ; follow-up: @adamsilverstein
10 years ago

I agree this would be a nice feature to add, however its outside the scope of the issue I am trying to address with this ticket.

Could you please open up a new ticket for this enhancement and post your patch there?

Thanks!

Replying to Fab1en:

I thinks that another behavior is to be fixed at the same time : using a scalar variable.
We should be able to do this :

wp_localize_script( 'main', 'localized_variable', 'a scalar value' );

As of version 3.9.1, 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/projects/other/quatreheures/esope/htdocs/wp-includes/class.wp-scripts.php on line 167

I suggest to add another test to avoid this issue.

This ticket was mentioned in IRC in #wordpress-dev by doc-bot. View the logs.


10 years ago

#26 in reply to: ↑ 24 @Fab1en
10 years ago

I have created ticket #29722 for the scalar value.

Replying to adamsilverstein:

I agree this would be a nice feature to add, however its outside the scope of the issue I am trying to address with this ticket.

Could you please open up a new ticket for this enhancement and post your patch there?

Thanks!

@adamsilverstein
10 years ago

refresh against trunk, code standards cleanup

#27 @adamsilverstein
10 years ago

25280.5.patch:

  • Refresh against trunk
  • Fix some code standards issues - whitespace, brackets around single line conditional

#28 follow-up: @jtsternberg
9 years ago

I'm not sure https://core.trac.wordpress.org/attachment/ticket/25280/25280.5.patch is a good idea as it will break backwards-compatibility. I'm guessing there are many scripts which strictly check for string values (I know I have), and if we changed this, it would break all of them.

#29 in reply to: ↑ 28 ; follow-up: @Fab1en
9 years ago

  • Keywords 4.1-early removed

Replying to jtsternberg:

it will break backwards-compatibility

The only way to break thinks here would be the following :

  • in PHP, declare an integer at the first level of the array
  • in JavaScript, check that the variable has been transformed to a string (whereas you should normally expect it to be an integer)

If this is really what you are doing (checking that an integer is transformed into a string), you just have to wrap the integer into a string in PHP to fix this.

With the proposed patch, only integers are concerned. Strings containing integers are left untouched. Moreover, JavaScript is casting the string into an integer when using in a integer context, so breaking something is not very likely.

Could you provide your script as an example ?

#30 in reply to: ↑ 29 @jtsternberg
9 years ago

Replying to Fab1en:

  • in JavaScript, check that the variable has been transformed to a string (whereas you should normally expect it to be an integer)

Most javascript linters expect strict type-checking, so yes, every time I use the values to compare, I check strictly, as a string, because that is what is always provided by wp_localize_script.

If this is really what you are doing (checking that an integer is transformed into a string), you just have to wrap the integer into a string in PHP to fix this.

Yes, I know how to fix my scripts/code. My point is for all the unsuspecting victims who are type-checking strictly and have no idea this change is in the works. That's what back-compatibility is all about.

#31 @ocean90
9 years ago

  • Component changed from I18N to Script Loader

#32 @adamsilverstein
8 years ago

What about leaving wp_localize_script as is for backward compatibility and introducing a new helper with better behavior?

25280.3.diff

  • Add a new helper function for passing data from PHP to JS: wp_pass_data_to_script
  • Includes passing tests from this ticket and #29722
  • Includes multidimensional array html entity decoding from #29722

#33 @ocean90
7 years ago

#43766 was marked as a duplicate.

#34 @jason_the_adams
7 years ago

Just discovered this by creating a duplicate ticket. I'd like to pass long a couple of my points and chime in.

Part of what makes this so painful is that we're dealing with a very commonly used built-in method that's not terribly well built. The method supports objects/arrays, but not very well. The casting and decoding only happen on the root level, so while we're concerned about backwards-compatibility, the existing functionality is inconsistent.

I appreciate the tension between wanting a function that produces reliable server-to-browser JSON, but also don't want to break the existing codebases.

So I support @adamsilverstein 's idea to add recursion to the existing functionality and break out a new helper. I think, though, that there may be more decisions to be made regarding adding the recursion than the helper. So perhaps that should be broken out into its own ticket so we can get this helper added sooner than later.

Would love to see that helper get into core!

#35 @adamsilverstein
7 years ago

  • Keywords has-unit-tests added

In 25280.4.diff

  • refresh against trunk plus slight cleanup
Version 0, edited 7 years ago by adamsilverstein (next)

#36 @adamsilverstein
6 years ago

  • Milestone changed from Future Release to 5.0
  • Owner set to adamsilverstein
  • Status changed from new to assigned

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


6 years ago

#38 @peterwilsoncc
6 years ago

  • Milestone changed from 5.0 to 5.1

Moving to the 5.1 milestone due to the WordPress 5.0 focus on the new
editor (Gutenberg).

#39 @swissspidy
6 years ago

#45599 was marked as a duplicate.

#40 @pento
6 years ago

  • Milestone changed from 5.1 to 5.2

This patch needs reviewing and a decision.

#41 @desrosj
6 years ago

@adamsilverstein are you able to review this before 5.2 beta 1? If not, let's punt this to 5.3.

#42 @adamsilverstein
6 years ago

@desrosj This patch is ready to go...

I think the main thing we need here is a decision: do we want to add this new helper wp_pass_data_to_script or should we instead encourage developers to use a different approach (eg. add_inline_script) to pass their data?

#43 @desrosj
6 years ago

The 5.2 beta deadline is just about here. Going to punt this one. Let's not rush this one since it still needs a decision.

#44 @desrosj
6 years ago

  • Milestone changed from 5.2 to 5.3

#45 @nerrad
6 years ago

I'd like to recommend this trac ticket get closed. wp_add_inline_script should be used for passing data to javascript and preserving type. wp_localize_scripts should only be used for its original intent (localizing scripts) which was abused over the years for passing other data because at the time there was nothing more useful (now there is).

#46 @adamsilverstein
5 years ago

  • Milestone 5.3 deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

Closing this as wontfix - as outlined in @nerrad's comment above, this function can be left as it as we currently have better approaches for passing data to JavaScript.

#47 @andraganescu
4 years ago

@nerrad I was looking up this function and the docs here actually recommend using it "to make any data available to your script that you can normally only get from the server side of WordPress.".

I would be good to update that. When you say now there is can you point me to a specific alternative?

#48 @nerrad
4 years ago

@andraganescu "now there is" refers to usage of wp_add_inline_script as the alternative. You can find more about usage of this function here: https://developer.wordpress.org/reference/functions/wp_add_inline_script/.

It looks like Ian added a comment on the wp_localize_script page clarifying its use too (and pointing to wp_add_inline_script for arbitrary data - https://developer.wordpress.org/reference/functions/wp_localize_script/#comment-3213). However, if you're planning on updating the main documentation that's a great idea!

#49 @andraganescu
4 years ago

Yes, we should remove the wrong advice and replace it with Ian's comment.

Note: See TracTickets for help on using tickets.