Make WordPress Core

Opened 14 years ago

Closed 12 years ago

Last modified 11 years ago

#11520 closed enhancement (fixed)

print_scripts_l10n() should use json_encode()

Reported by: scribu's profile scribu Owned by: azaozz's profile 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:

  1. json_encode() is already supported by WP 2.9 (see here)
  1. In practice, wp_localize_script() is used for passing arbitrary data to JS. Currently, you can only pass strings.

Attachments (13)

print_scripts_l10n.diff (1.7 KB) - added by scribu 14 years ago.
print_scripts_l10n.2.diff (8.1 KB) - added by scribu 13 years ago.
get rid of 'l10n_print_after' hack
11520-3.patch (20.6 KB) - added by azaozz 13 years ago.
11520-4.patch (27.4 KB) - added by azaozz 13 years ago.
get_data.11520.diff (5.9 KB) - added by scribu 13 years ago.
11520.autosave.patch (581 bytes) - added by ocean90 13 years ago.
footer_scripts.patch (2.8 KB) - added by azaozz 13 years ago.
wp_localize_script.test-case.diff (1.6 KB) - added by nacin 13 years ago.
11520.wrapper.patch (1.6 KB) - added by ocean90 12 years ago.
11520.wrapper.2.patch (1.7 KB) - added by ocean90 12 years ago.
Some more inline doc
11520-5.patch (20.2 KB) - added by azaozz 12 years ago.
11520-6.patch (18.1 KB) - added by azaozz 12 years ago.
11520.diff (1.8 KB) - added by nacin 12 years ago.

Download all attachments as: .zip

Change History (106)

#1 @scribu
14 years ago

  • Cc scribu@… added

#2 follow-ups: @azaozz
14 years ago

  • Keywords has-patch removed

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.

#3 in reply to: ↑ 2 @hakre
14 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 @scribu
14 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 @scribu
14 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.

#6 @nacin
14 years ago

  • Milestone changed from 3.0 to Future Release

#7 @sushkov
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 @scribu
13 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.

Last edited 13 years ago by scribu (previous) (diff)

@scribu
13 years ago

get rid of 'l10n_print_after' hack

#9 @scribu
13 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.

#10 @scribu
13 years ago

  • Keywords dev-feedback added; commit removed

#11 @ryan
13 years ago

  • Milestone changed from 3.2 to Future Release

#12 @scribu
13 years ago

Related: #7665

#13 @azaozz
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.

@azaozz
13 years ago

@azaozz
13 years ago

#14 @iandunn
13 years ago

  • Cc ian_dunn@… added

#15 follow-up: @scribu
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().

#16 in reply to: ↑ 15 @azaozz
13 years ago

Replying to scribu:

Agreed. Will change the names.

#17 @scribu
13 years ago

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

#18 @scribu
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 @scribu
13 years ago

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

Last edited 13 years ago by scribu (previous) (diff)

#20 follow-up: @scribu
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 @azaozz
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.

Last edited 13 years ago by azaozz (previous) (diff)

#22 @azaozz
13 years ago

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

#23 follow-up: @nacin
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 @scribu
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.

Last edited 13 years ago by scribu (previous) (diff)

#25 in reply to: ↑ 23 ; follow-up: @azaozz
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: @westi
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 @azaozz
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. That was the main reason for this enhancement.

Last edited 13 years ago by azaozz (previous) (diff)

#28 @sirzooro
13 years ago

  • Cc daniel@… added

#29 @ocean90
13 years ago

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

#30 @scribu
13 years ago

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

#31 follow-up: @scribu
13 years ago

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

#32 in reply to: ↑ 31 @azaozz
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

Last edited 13 years ago by azaozz (previous) (diff)

#33 follow-up: @scribu
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 @scribu
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 @azaozz
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: @scribu
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 @azaozz
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).

#38 follow-up: @azaozz
13 years ago

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

#39 in reply to: ↑ 38 ; follow-up: @westi
13 years ago

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.

#40 in reply to: ↑ 39 ; follow-up: @azaozz
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 @westi
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 @SidHarrell
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: @scribu
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: @westi
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 @azaozz
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.

#46 @azaozz
13 years ago

In [18490]:

Bring back wp_localize_script(), see #11520

#47 @ocean90
13 years ago

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

#48 @azaozz
13 years ago

In [18491]:

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

#49 @SidHarrell
13 years ago

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

#50 @azaozz
13 years ago

In [18496]:

Optimize script-loader a bit, see #11520

#51 @scribu
13 years ago

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

#52 follow-up: @nacin
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 @nacin
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 @azaozz
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.

#55 @GaryJ
13 years ago

  • Cc gary@… added

#56 @iandunn
13 years ago

  • Cc ian_dunn@… removed

#57 @aaroncampbell
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.

#58 @tmtrademark
13 years ago

  • Cc toby.mckes@… added

#59 @batmoo
13 years ago

  • Cc batmoo@… added

#60 @azaozz
13 years ago

In [18610]:

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

#61 follow-up: @nacin
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.

Last edited 13 years ago by nacin (previous) (diff)

#62 @SergeyBiryukov
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 @scribu
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.

Last edited 13 years ago by scribu (previous) (diff)

#64 in reply to: ↑ 61 @azaozz
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.

#65 @azaozz
13 years ago

In [18804]:

Convert html entities in script localization strings, see #11520

#66 @markjaquith
13 years ago

[18804] broke wp-admin comment replying.

#67 @markjaquith
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().

#68 @nacin
13 years ago

In [18810]:

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

#69 @azaozz
13 years ago

In [18813]:

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

#70 @westi
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: @scribu
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: @westi
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 @scribu
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.

#74 @sirzooro
13 years ago

  • Cc daniel@… removed

#75 @sirzooro
13 years ago

  • Cc sirzooro added

#76 in reply to: ↑ 72 @azaozz
13 years ago

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

#77 @ocean90
12 years ago

Fixed?

#78 follow-up: @nacin
12 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 @ocean90
12 years ago

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

Like 11520.wrapper.patch?

@ocean90
12 years ago

Some more inline doc

#80 in reply to: ↑ 78 @azaozz
12 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).

@azaozz
12 years ago

#81 @azaozz
12 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: @nacin
12 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 @westi
12 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.

@azaozz
12 years ago

#84 @azaozz
12 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 :)

#85 @azaozz
12 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In [19217]:

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

#86 @azaozz
12 years ago

In [19218]:

Phpdoc fix for wp_add_script_before(), see #11520

#87 @nacin
12 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 @scribu
12 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.

@nacin
12 years ago

#89 @nacin
12 years ago

  • Keywords has-patch commit added

#90 @azaozz
12 years ago

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

#91 @ryan
12 years ago

Fine by me.

#92 @nacin
12 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In [19573]:

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

#93 @ethitter
11 years ago

  • Cc erick@… added
Note: See TracTickets for help on using tickets.