print_scripts_l10n() should use json_encode()

[scribu]

Two things make me think this is a good ideea:

1. json_encode() is already supported by WP 2.9 (see ​here)

2. In practice, wp_localize_script() is used for passing arbitrary data to JS. Currently, you can only pass strings.

[azaozz]

Problem is that not all browsers have built-in support for JSON (yet), so this will need json2.js always included. Also perhaps we can standardize a way to pass data to JS and use that for translations making wp_localize_script() obsolete.

[hakre]

Replying to azaozz:

Problem is that not all browsers have built-in support for JSON (yet)

Which browser does not support eval()?

[scribu]

We're just defining a native JavaScript object, same as before. So no JSON support needed.

[scribu]

Replying to azaozz:

Also perhaps we can standardize a way to pass data to JS and use that for translations making wp_localize_script() obsolete.

+1 on that.

[sushkov]

There's more to consider about this ticket.
Currently wp_localize_script() doesn't work if you have nested arrays of data, it will return you first level and then output : Array.

Can we rise the priority to get it fixed sooner?

[scribu]

PHP JSON support is covered (PHP 5)

Browser JSON support doesn't even come into question: we're just generating an inline script tag.

As an additional bonus, if we use JSON, we can ditch convertEntities(), which is a very ugly hack.

I think it's better if we move in small, manageable steps:

• take care of wp_localize_script() in WP 3.2
• think about a more general API later (if it's even needed)

Patch still applies cleanly.

[scribu]

get rid of 'l10n_print_after' hack

[scribu]

print_scripts_l10n.2.diff​ also gets rid of the 'l10n_print_after' hack, since it's already taken care of by json_encode().

Wondering wether we could do away with l10n.dev.js entirely. I don't imagine plugins calling convertEntities() directly.

[scribu]

Related: #7665

[azaozz]

If we add this it could be used to output any dynamically generated JS from PHP (plugins, themes, etc.). Wondering if it's worth renaming it as "l10n" doesn't really describe it properly. Also need to support more than one addition per script, would make it a lot more flexible.

[scribu]

11520-4.patch looks great.

I think print_inline_js() would be a more descriptive name, instead of print_extra_js().

Similarly, wp_add_inline_js() instead of wp_add_js().

[azaozz]

Replying to scribu:

Agreed. Will change the names.

[scribu]

Also, wp_localize_script() should call _deprecated_function(), no?

[scribu]

Hm... since you can only define a variable, I don't think wp_add_inline_js() is an accurate name either. wp_add_js_data() would be better.

For CSS, wp_add_inline_css() is ok.

[scribu]

For ultimate convenience, we could have both wp_add_inline_js() and wp_add_js_data().

[scribu]

Finally, we should continue to use 'script' and 'style', rather than 'js' and 'css'. So it would be wp_add_script_data() and wp_add_inline_style().

Darn, keeping an API consistent is hard work. :)

[azaozz]

Replying to scribu:

Darn, keeping an API consistent is hard work. :)

Haha, agreed. And the naming of the new functions/methods is the hardest.

[azaozz]

In [18464]:

Use json_encode() for adding script data (formerly l10n). Add the same functionality to WP_Styles for adding inline css after a stylesheet has been outputted. See #11520

[nacin]

I'm not sure I see a need to actually deprecate a function here. wp_localize_script() is for localizing a script.

add_script_data() is for adding script data. To me, it sounds less of a replacement, and more like a lower level of the API, like *_metadata(). wp_localize_script() can be a wrapper.

[scribu]

wp_localize_script() is for localizing a script.

IMO, that's an arbitrary distinction, since there's no native way to do that in JavaScript. It's just variables that you can use for any purpose.

Conversely, add_script_data() could be a wrapper for add_inline_script(), if we decide to add it.

[azaozz]

Replying to nacin:

add_script_data() is for adding script data. To me, it sounds less of a replacement, and more like a lower level of the API, like *_metadata(). wp_localize_script() can be a wrapper.

The deprecation here is only about the (old) names: localize and ..._l10n. Even before this patch plugins were using wp_localize_script() to add arbitrary JS data. On the other hand having two different functions that do exactly the same thing may be confusing.

[westi]

Replying to azaozz:

Replying to nacin:

add_script_data() is for adding script data. To me, it sounds less of a replacement, and more like a lower level of the API, like *_metadata(). wp_localize_script() can be a wrapper.

The deprecation here is only about the (old) names: localize and ..._l10n. Even before this patch plugins were using wp_localize_script() to add arbitrary JS data. On the other hand having two different functions that do exactly the same thing may be confusing.

I don't see why we have done this deprecation.

The function name was better and we didn't need to do it - can't we keep the old function name and just do the switch to json here?

With this new code can we do away with the l10n.js file now too?

[azaozz]

Replying to westi:

The function name was better and we didn't need to do it - can't we keep the old function name and just do the switch to json here?

We could but IMHO wp_localize_script() doesn't sound right for a function that can pass any data from PHP to JS.

With this new code can we do away with the l10n.js file now too?

It's gone. That was the main reason for this enhancement.

[ocean90]

Would it be possible to handle #14853 and #16494 via add_script_data?

[scribu]

Nope. We would need add_inline_script(), mentioned above.

[scribu]

get_data.11520.diff​:

• introduce WP_Dependencies::get_data() method, for simplified data access
• fix print_inline_style() printing the styles in reverse order
• add_inline_style() stores code in 'after' instead of 'data', as an array

[azaozz]

Replying to scribu:

• fix print_inline_style() printing the styles in reverse order

Was actually following the rest of the logic for WP_Dependencies: if there is a conflict (in names or values) the first added object wins. For CSS that should be printed last so it overwrites previous styles.

For scripts this is here: https://core.trac.wordpress.org/browser/trunk/wp-includes/class.wp-scripts.php#L168

[scribu]

Was actually following the rest of the logic for WP_Dependencies: if there is a conflict (in names or values) the first added object wins.

It's not the same thing, since inline styles don't have names.

[scribu]

Even for scripts, I don't see why it would be a good idea for the first one to win.

It prevents a third plugin from overwriting initial values, when that might actually be needed some times.

[azaozz]

Replying to scribu:

It's not the same thing, since inline styles don't have names.

True, but if two styles cover the same selector(s), the last printed one wins.

Even for scripts, I don't see why it would be a good idea for the first one to win.

This is the current logic in WP_Dependencies. You cannot re-declare already added script or style there, i.e. trying to add 'jquery' after it's been queued (with different args, src, etc.) will fail.

Don't mind reversing that when adding inline scripts and styles but will need to cover it nicely in the phpdoc there.

[scribu]

This is the current logic in WP_Dependencies. You cannot re-declare already added script or style there, i.e. trying to add 'jquery' after it's been queued (with different args, src, etc.) will fail.

Sure, but in the case of external scripts, you have wp_unregister_script(). There's no wp_remove_script_data(). Should there be?

[azaozz]

Replying to scribu:

Sure, but in the case of external scripts, you have wp_unregister_script(). There's no wp_remove_script_data(). Should there be?

Good point. Don't think we need wp_remove_script_data() as both JS and CSS can easily be overwritten/overloaded at any time. Then it makes more sense to have the last added script/style win in case of conflict (i.e. they follow the "natural" order in HTML).

[azaozz]

In [18480]:

Introduce WP_Dependencies::get_data() method, change scripts and styles priority to follow the "natural" order in HTML, i.e. the last one wins, props scribu, see #11520

[westi]

Replying to azaozz:

In [18480]:

Introduce WP_Dependencies::get_data() method, change scripts and styles priority to follow the "natural" order in HTML, i.e. the last one wins, props scribu, see #11520

This is just changing the order in which we process localisation and other associated data not the way in which multiple enqueues of scripts with the same identifier worked?

Are we breaking any backward compatible behaviour here - it's not clear from a quick read of the ticket.

[azaozz]

Replying to westi:

Are we breaking any backward compatible behaviour here...

No, adding second l10n array with the same name was replacing the first (never seen that happen but it was possible), now it appends it overwriting only conflicting keys.

[westi]

Replying to azaozz:

Replying to westi:

Are we breaking any backward compatible behaviour here...

No, adding second l10n array with the same name was replacing the first (never seen that happen but it was possible), now it appends it overwriting only conflicting keys.

Cool - I was unsure from the commit message thanks for confirming.

[SidHarrell]

Is there a patch I can apply to my plugin code to silence the warning:
Notice: wp_localize_script is deprecated since version 3.3! Use wp_add_script_data() instead.

From what I could gather, the new function is introduced in 3.3, so we can't really switch to the new function until enough of the user base has moved to a version that has it.

[scribu]

You could replace this:

wp_localize_script( 'my-script', 'MyScriptL10N', $data );

with this:

$func = function_exists( 'wp_add_script_data' ) ? 'wp_add_script_data' : 'wp_localize_script';
$func( 'my-script', 'MyScriptL10N', data );

There are a lot of plugins that would need to make this rather ugly and unnecessary change.

Maybe we should remove the deprecation warning for now.

[westi]

Replying to scribu:

You could replace this:

wp_localize_script( 'my-script', 'MyScriptL10N', $data );

with this:

$func = function_exists( 'wp_add_script_data' ) ? 'wp_add_script_data' : 'wp_localize_script';
$func( 'my-script', 'MyScriptL10N', data );

There are a lot of plugins that would need to make this rather ugly and unnecessary change.

Maybe we should remove the deprecation warning for now.

I don't understand the need to deprecate this function at all.

We should just keep the wrapper function for now as the naming and usage for l10n is correct.

[azaozz]

Replying to westi:

We should just keep the wrapper function for now as the naming and usage for l10n is correct.

Was thinking about this earlier today. Perhaps we should keep the old function and explain (in the phpdoc) that the name is "localize" because of historical reasons.

[azaozz]

In [18490]:

Bring back wp_localize_script(), see #11520

[ocean90]

11520.autosave.patch to prevent the fatal error from script-loader.php.

[azaozz]

In [18491]:

Completely remove wp_add_script_data(), props ocean90, see #11520

[SidHarrell]

Working now. No fatal error, no warning. Thanks, guys.

[azaozz]

In [18496]:

Optimize script-loader a bit, see #11520

[scribu]

Since we already have wp_script_is(), I don't think we really need wp_add_inline_script(). Example:

​http://wordpress.stackexchange.com/questions/24851/wp-enqueue-inline-script-due-to-dependancies/24863#24863

[nacin]

Daryl mentioned that wp_localize_script() escapes data differently than json_encode(). We may need to resurrect it, and create wp_add_script_data() as a new way to do this.

I'm not sure I like [18496].

Removing l10n.js is probably guaranteed to trigger fatal errors in a plugin that is trying to use it (such as on the frontend). Mind you, they might be calling convertEntities(). Also, we didn't update all of the references to it, such as in install.php.

[nacin]

Just to confirm:

var_dump( json_encode( 'test\test' ) );
string(12) ""test\\test""
var_dump( esc_js( 'test\test' ) );
string(8) "testtest"

[azaozz]

Replying to nacin:

Daryl mentioned that wp_localize_script() escapes data differently than json_encode().

Not exactly, wp_localize_script() didn't escape anything. We have esc_js() hard coded on some strings (not sure why not all) in script_loader. Seems we have to remove it there as json_encode() does that now.

I'm not sure I like [18496].

This only affects making the initial queue in script_loader, i.e. it's internal use only.

Removing l10n.js is probably guaranteed to trigger fatal errors in a plugin that is trying to use it (such as on the frontend). Mind you, they might be calling convertEntities().

By default it's (supposed to be) used in a try/catch statement, so removing the function shouldn't affect anything. Perhaps there are couple of plugins that try to call it directly, we can put a dummy function function convertEntities(s){return s;} in the head to keep them from breaking.

[aaroncampbell]

It looks like moving wp_print_footer_scripts in [18464] caused the code here (​https://gist.github.com/392765) to break. Toby M (@trademark) noticed it...he apparently uses that code.

[azaozz]

In [18610]:

Fix action 'wp_print_footer_scropts' (on the front-end), see #11520

[nacin]

wp_localize_script.test-case.diff​ shows that wp_localize_script() escaping behavior has changed from 3.2 to 3.3. The test is janky, as it has to parse JS output, but the result is clear, and illustrates what comment 53 outlined.

The test works in 3.2, and fails in 3.3. This convinces me that we should leave wp_localize_script() alone and switch to wp_add_script_data().

We can't wait until beta to see if any changes broke anything. It's better to be proactive about avoiding any backwards incompatible changes, no matter how small they may seem.

[SergeyBiryukov]

FWIW, I've noticed that non-ASCII characters are also escaped now:

3.2.1:

var commonL10n = {
	warnDelete: "Вы собираетесь навсегда удалить выделенные объекты.\n  «Отмена» — оставить, «OK» — удалить."
};

3.3-trunk:

var commonL10n = {"warnDelete":"\u0412\u044b \u0441\u043e\u0431\u0438\u0440\u0430\u0435\u0442\u0435\u0441\u044c \u043d\u0430\u0432\u0441\u0435\u0433\u0434\u0430 \u0443\u0434\u0430\u043b\u0438\u0442\u044c \u0432\u044b\u0434\u0435\u043b\u0435\u043d\u043d\u044b\u0435 \u043e\u0431\u044a\u0435\u043a\u0442\u044b.\n  \u00ab\u041e\u0442\u043c\u0435\u043d\u0430\u00bb \u2014 \u043e\u0441\u0442\u0430\u0432\u0438\u0442\u044c, \u00abOK\u00bb \u2014 \u0443\u0434\u0430\u043b\u0438\u0442\u044c."};

[scribu]

The important thing to test isn't that the strings look the same in the HTML source, but that they have the same value when they're actually loaded into JavaScript.

In the HTML, whether you write:

var x = "a\'s";

or

var x = "a's";

in JavaScript, the value will be the same in both cases: a's.

Therefore, wp_localize_script.test-case.diff​ is not a relevant test, nor the fact that non-ascii characters are now escaped.

[azaozz]

Replying to nacin:

wp_localize_script.test-case.diff​ shows that wp_localize_script() escaping behavior has changed...

The test works in 3.2, and fails in 3.3.

Unfortunately you cannot "guess" how JS works from PHP. For this testcase to be relevant it will have to parse the JS and compare the output after that. This is possible to do with tools like Mozilla's Rhino. Currently this testcase will fail minimized JS too but (as we know) minimization works very well.

We can't wait until beta to see if any changes broke anything.

Why? If we have to put the old (inconsistent and buggy) code back, we can do that at any time. There's no indication that using json breaks anything at the moment, however I'm sure we will find newly introduced bugs and regressions in many places while beta-testing.

[azaozz]

In [18804]:

Convert html entities in script localization strings, see #11520

[markjaquith]

[18804] broke wp-admin comment replying.

[markjaquith]

WARNING: wp-includes/class.wp-scripts.php:73 - html_entity_decode() expects parameter 1 to be string, array given

So I guess we need a recursive html_entity_decode().

[nacin]

In [18810]:

Revert [18804] until we have a proper solution. see #11520.

[azaozz]

In [18813]:

Recursively convert html entities in script localization strings, see #11520

[westi]

I'm still no happy or sure we need the underlying change here - we seem to have spent a chunk of time on fixing something that wasn't a bug and worked perfectly well.

wp_localize_script() is used for passing arbitrary data to JS. Currently, you can only pass strings. 

I think that people doing this are writing bugs.

[scribu]

I'm still no happy or sure we need the underlying change here - we seem to have spent a chunk of time on fixing something that wasn't a bug and worked perfectly well.

It's not a bug; it's an enhancement.

I think that people doing this [passing arrays/objects instead of strings] are writing bugs.

Please provide an example where doing this would introduce a bug and also please provide the correct way of doing it.

[westi]

Replying to scribu:

I'm still no happy or sure we need the underlying change here - we seem to have spent a chunk of time on fixing something that wasn't a bug and worked perfectly well.

It's not a bug; it's an enhancement.

I think that people doing this [passing arrays/objects instead of strings] are writing bugs.

Please provide an example where doing this would introduce a bug and also please provide the correct way of doing it.

The function was for localisation not passible random data to scripts.

If we wanted to introduce something separate for that we should have done but not overloaded the meaning of an existing process and changed how it was implemented for no real benefit for the intended usage.

[scribu]

This was already discussed. In summary:

• The difference between localization strings and other data is artificial. JavaScript doesn't have special constructs for localization. Therefore, there should be a single function.
• wp_localize_script() was renamed to wp_add_script_data() to reflect it's broader purpose, but it was reverted, since it was argued that it's not worth the work that plugin developers would have to do.

[azaozz]

Replying to westi:

The current implementation has two (major) improvements:

1. json_encode() properly encodes all characters. This was a long standing shortcoming masked by the fact that WordPress sets UTF-8 as default charset in the browser. But as long as we support other charsets, we need to make sure all JS strings containing non ASCII chars are properly encoded (JS expects UFT-8). For example using English sets most browsers to ISO-8859-1 and any non ASCII chars in the JS localization scripts are garbled.

2. Replaces the (hacky) HTML entities decoding in JS with html_entity_decode(). Not sure why we didn't use that the first time (in WP 2.7), but seems to work well now (PHP 5.2).

We still can split the functions and revert localize_script but will need to use html_entity_decode() and json_encode() for it. Can even make add_script_data() to accept one string only leaving the encoding to the plugins that would use it (that would actually allow passing of JS functions, not just data).

[ocean90]

Fixed?

[nacin]

In hindsight, it's probably time for wp_add_script_data(), and wp_localize_script() as a non-deprecated wrapper for now.

[ocean90]

time for wp_add_script_data(), and wp_localize_script() as a non-deprecated wrapper for now.

Like 11520.wrapper.patch​?

[ocean90]

Some more inline doc

[azaozz]

Replying to nacin:

Agreed. Was thinking/testing the usercases and having another method to add some JS before or after a JS file is included would be the best option.

In that terms we should convert_entities only on localize_script() allowing add_script_data() to be used to pass JS code directly (non-modified strings).

[azaozz]

11520-5.patch restores WP_Scripts::localize() and wp_localize_script(), renames add_script_data to add_script_extra and makes WP_Scripts::add_script_extra() accept a string (the JS source) so it can be used to pass any JS directly.

Both WP_Scripts::localize() and wp_localize_script() are backwards compatible and can be used as before to output a JS object holding translated strings. The change is that now HTML entities are converted with html_entity_decode() and the array is json encoded.

[nacin]

I really, really like 11520-5.patch​. Curious as to westi's opinion as well.

There's a lot of changes, but it's grok-able. If we can't get this into 3.3, what should we get into 3.3 in order to set us up for 3.4?

I would like to see a diff that doesn't mess with the _doing_it_wrong() stuff. Ultimately, moving it into one function makes it more difficult to backtrace, and it really doesn't do *that* much to abstract things out. I know it's not very DRY, but still. :)

[westi]

Replying to nacin:

I really, really like 11520-5.patch​. Curious as to westi's opinion as well.

There's a lot of changes, but it's grok-able. If we can't get this into 3.3, what should we get into 3.3 in order to set us up for 3.4?

I would like to see a diff that doesn't mess with the _doing_it_wrong() stuff. Ultimately, moving it into one function makes it more difficult to backtrace, and it really doesn't do *that* much to abstract things out. I know it's not very DRY, but still. :)

This looks like it gets us back somewhere more sensible and doesn't deprecate something for no reason.

The {_doing_it_wrong()} calls should not be wrapped in another function - one persons DRY is another persons over abstraction and this is over abstraction (the _doing_it_wrong() function is the DRY bit). This is especially true because we are calling it from many different places and outputting the same message where instead the message should be much more contextual telling you which function you are calling too early and when you should call it.

[azaozz]

@nacin, @westi, agreed, putting back the 8 _doing_it_wrong() in functions.wp-scripts.php.

11520-6.patch is actually two steps away from being able to output JS after the main script tag. Something to think for 3.4. For that we will need to improve wp-admin/load-scripts.php so we can guarantee the extra bits are right after the main script. Then we would finally be able to take jQuery.noConflict() out of jquery.js :)

[azaozz]

In [19217]:

Ressurect WP_Scripts::localize() and fix public function names, fixes #11520

[azaozz]

In [19218]:

Phpdoc fix for wp_add_script_before(), see #11520

[nacin]

We should eliminate wp_add_script_before() until it handles the encoding of an array on its own. See IRC conversation (ongoing):

​https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2011-12-08&sort=asc#m342977

[scribu]

After a lengthy talk on IRC, some conclusions:

<koopersmith>to test: wp_add_script_before( 'utils', 'alert( !! jQuery );' );
<koopersmith>with SCRIPT_DEBUG on, all works fine. with concat, you get an undefined error.

This is a fixable problem with the current wp_add_script_before() and a non-fixable problem with the proposed wp_add_callback_before_script().

Either way, to prevent plugins from shooting themselves in the foot with this, the consensus was that it's indeed better to remove wp_add_script_before() altogether, for now, at least.

[azaozz]

Looks good. No point in having a separate function and method for adding JS when an action can be used to do it.

[ryan]

Fine by me.

[nacin]

In [19573]:

Remove wp_add_script_before() from 3.3. Continue to use wp_localize_script() for your data-passing needs. fixes #11520.