#11520 closed enhancement (fixed)
print_scripts_l10n() should use json_encode()
Reported by: | scribu | Owned by: | azaozz |
---|---|---|---|
Milestone: | 3.3 | Priority: | highest omg bbq |
Severity: | critical | Version: | |
Component: | JavaScript | Keywords: | has-patch commit |
Focuses: | Cc: |
Description
Two things make me think this is a good ideea:
- json_encode() is already supported by WP 2.9 (see here)
- In practice, wp_localize_script() is used for passing arbitrary data to JS. Currently, you can only pass strings.
Attachments (13)
Change History (106)
#3
in reply to:
↑ 2
@
15 years ago
Replying to azaozz:
Problem is that not all browsers have built-in support for JSON (yet)
Which browser does not support eval()?
#4
@
15 years ago
- Keywords has-patch added
We're just defining a native JavaScript object, same as before. So no JSON support needed.
#5
in reply to:
↑ 2
@
15 years ago
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.
#7
@
14 years ago
- Cc sushkov added
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?
#8
@
14 years ago
- Keywords commit added
- Milestone changed from Future Release to 3.2
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.
#9
@
14 years ago
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.
#13
@
13 years ago
- Milestone changed from Future Release to 3.3
- Status changed from new to reviewing
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.
#15
follow-up:
↓ 16
@
13 years ago
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().
#18
@
13 years ago
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.
#19
@
13 years ago
For ultimate convenience, we could have both wp_add_inline_js() and wp_add_js_data().
#20
follow-up:
↓ 21
@
13 years ago
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. :)
#21
in reply to:
↑ 20
@
13 years ago
Replying to scribu:
Darn, keeping an API consistent is hard work. :)
Haha, agreed. And the naming of the new functions/methods is the hardest. Thinking we should stick to add_script_data()
and add_inline_style()
, sounds good enough.
#23
follow-up:
↓ 25
@
13 years ago
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.
#24
@
13 years ago
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.
#25
in reply to:
↑ 23
;
follow-up:
↓ 26
@
13 years ago
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.
#26
in reply to:
↑ 25
;
follow-up:
↓ 27
@
13 years ago
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?
#27
in reply to:
↑ 26
@
13 years ago
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.
#31
follow-up:
↓ 32
@
13 years ago
- 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
#32
in reply to:
↑ 31
@
13 years ago
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
#33
follow-up:
↓ 35
@
13 years ago
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.
#34
@
13 years ago
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.
#35
in reply to:
↑ 33
@
13 years ago
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.
#36
follow-up:
↓ 37
@
13 years ago
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?
#37
in reply to:
↑ 36
@
13 years ago
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).
#39
in reply to:
↑ 38
;
follow-up:
↓ 40
@
13 years ago
Replying to azaozz:
In [18480]:
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.
#40
in reply to:
↑ 39
;
follow-up:
↓ 41
@
13 years ago
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.
#41
in reply to:
↑ 40
@
13 years ago
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.
#42
@
13 years ago
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.
#43
follow-up:
↓ 44
@
13 years ago
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.
#44
in reply to:
↑ 43
;
follow-up:
↓ 45
@
13 years ago
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.
#45
in reply to:
↑ 44
@
13 years ago
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.
#51
@
13 years ago
Since we already have wp_script_is(), I don't think we really need wp_add_inline_script(). Example:
#52
follow-up:
↓ 54
@
13 years ago
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.
#53
@
13 years ago
Just to confirm:
var_dump( json_encode( 'test\test' ) ); string(12) ""test\\test"" var_dump( esc_js( 'test\test' ) ); string(8) "testtest"
#54
in reply to:
↑ 52
@
13 years ago
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.
#57
@
13 years ago
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.
#61
follow-up:
↓ 64
@
13 years ago
- Keywords revert added; has-patch dev-feedback removed
- Severity changed from normal to critical
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.
#62
@
13 years ago
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."};
#63
@
13 years ago
- Keywords revert removed
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.
#64
in reply to:
↑ 61
@
13 years ago
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.
#67
@
13 years ago
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()
.
#70
@
13 years ago
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.
#71
follow-up:
↓ 72
@
13 years ago
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.
#72
in reply to:
↑ 71
;
follow-up:
↓ 76
@
13 years ago
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.
#73
@
13 years ago
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.
#76
in reply to:
↑ 72
@
13 years ago
Replying to westi:
The current implementation has two (major) improvements:
- 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.
- 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).
#78
follow-up:
↓ 80
@
13 years ago
In hindsight, it's probably time for wp_add_script_data(), and wp_localize_script() as a non-deprecated wrapper for now.
#79
@
13 years ago
time for wp_add_script_data(), and wp_localize_script() as a non-deprecated wrapper for now.
Like 11520.wrapper.patch?
#80
in reply to:
↑ 78
@
13 years ago
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).
#81
@
13 years ago
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.
#82
follow-up:
↓ 83
@
13 years ago
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. :)
#83
in reply to:
↑ 82
@
13 years ago
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.
#84
@
13 years ago
@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 :)
#87
@
13 years ago
- Priority changed from normal to highest omg bbq
- Resolution fixed deleted
- Status changed from closed to reopened
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
#88
@
13 years ago
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.
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.