Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#24197 closed defect (bug) (fixed)

wp_localize_script() doesn't work for jquery handle

Reported by: icypixels's profile IcyPixels Owned by: nacin's profile nacin
Milestone: 3.6 Priority: normal
Severity: major Version: 3.6
Component: General Keywords: dev-feedback
Focuses: Cc:

Description

Here's another odd bug introduced by WP 3.6 latest development version.

I'm using the following code: http://snippi.com/s/s058ms7

In WP 3.5.1 everything worked as it should, and wp_localize_script returned the "icy_ajax".

Now, in 3.6-beta1-24067 , I get the following error in my console: ReferenceError: icy_ajax is not defined

Just try and test it and let me know of any solution :-)

Attachments (3)

24197.patch (1.3 KB) - added by azaozz 11 years ago.
24197-2.patch (1.9 KB) - added by azaozz 11 years ago.
24197.diff (622 bytes) - added by nacin 11 years ago.

Download all attachments as: .zip

Change History (21)

#1 @SergeyBiryukov
11 years ago

  • Keywords reporter-feedback added; needs-patch 2nd-opinion removed

The snippet is a bit confusing. Are you passing data to a modified version of jquery.js?

Seems like your JS file is enqueued under different conditions (when $post is not set), and the script is trying to use icy_ajax without checking if it was added. Or the 'jquery' handle is incorrect.

My steps:

  1. Enqueued my own script under the same if ( isset( $post ) ) check and changed 'jquery' in your snippet to that script handle.
  2. Went to Edit Post screen.
  3. alert( icy_ajax.post_id ) prints the post ID, as expected.
Last edited 11 years ago by SergeyBiryukov (previous) (diff)

#2 @t31os_
11 years ago

There seems to be some specific issue with calling localize with the jquery handle, switching the call to use jquery-ui-core also works, it's definitely specific to trying to create vars for the jquery enqueue.

wp_localize_script( 'jquery', 'myvars', array( 'foo' => 'bar' ) );

No vars output in the page.

wp_localize_script( 'jquery-ui-core', 'myvars', array( 'foo' => 'bar' ) );

Vars are successfully output in the page(assuming you have jquery ui on the given page of course).

wp_localize_script( 'my-enqueued-script', 'myvars', array( 'foo' => 'bar' ) );

Again vars are output to the page.

I also tried calling the localize method directly, and can see they appear in the registered scripts data array.

$wp_scripts->localize( 'jquery', 'myvars', array( 'foo' => 'bar' ) );
echo $wp_scripts->registered['jquery']['extra']['data']; // Prints var myvars = {"foo":"bar"};

However, again the vars are not output in the page, specifically when they are created for the jquery handle. Not sure why any of this occurs, just wanted to confirm i can replicate the issue being described in trunk.

EDIT: I took a look at wp_default_scripts

// jQuery
$scripts->add( 'jquery', false, array( 'jquery-core', 'jquery-migrate' ) );
$scripts->add( 'jquery-core', '/wp-includes/js/jquery/jquery.js', array(), '1.9.1' );
$scripts->add( 'jquery-migrate', '/wp-includes/js/jquery/jquery-migrate.js', array(), '1.1.1' );

And noticed the jquery handle is really just an alias for jquery-core and found this works as expected.

wp_localize_script( 'jquery-core', 'myvars', array( 'foo' => 'bar' ) );
Last edited 11 years ago by t31os_ (previous) (diff)

#3 @IcyPixels
11 years ago

Hey t31os_ !

Thanks for confirming the bug and indeed, it seems that with jquery-core it works! Thanks for letting me know!

Kind Regards.
Paul

#4 @ocean90
11 years ago

  • Keywords dev-feedback added; reporter-feedback removed
  • Milestone changed from Awaiting Review to 3.6

So how many plugins localize jquery (and why…)?

#5 @IcyPixels
11 years ago

Here's what I basically did:

http://snippi.com/s/8o94pgy

Created a small automatization for Meta box creation, and inside the "images" case, I leveraged the new Media Uploader. I needed a variable called icy_ajax which needed to house in different information. Until this version, 3.6, everything worked just right by localizing it to the jquery script, now it only works hooking it to the jquery-core script.

#6 @azaozz
11 years ago

We are not shipping with jquery-migrate.js included, right? When 3.6 is released the 'jquery' handle will behave as before.

#7 @somatic
11 years ago

Confirming same problem here with 3.6 beta2. I localize all the time in my plugins to pass variables from wp/php for my jquery functions. If this "jquery-migrate" is fouling it up, and won't be fixed until final release, you're going to hear a lot of developers wondering why their javascript is bungled while testing the beta...

#8 @azaozz
11 years ago

If this "jquery-migrate" is fouling it up...

The problem is not with jquery-migrate. Currently the jquery handle is not a "real" handle but rather an alias for enqueueing both jQuery and jQuery Migrate at the same time. As jquery is not "printed", any data attached to it is not printed either.

Wondering why plugins attach data to the jquery handle instead of their own scripts. The "localize" functionality makes sure dynamic data is outputted before the inclusion of a script that needs is. Attaching that data to a different script is not right and can break as in this case.

#9 @somatic
11 years ago

Thx for the explanation. As for attaching to jquery handle, you're right, it would be more accurate to attach to our own scripts. So many localize tutorials for passing variables to javascript seem to default to jquery, that I never thought about it. I guess part of the assumption was that jquery handle, being in core, and used by pretty much everything, was a safe bet - and also possibly that it would make that data available even in cases where you weren't necessarily explicitly enqueuing your own script (maybe echoing some inline js).

#10 @kovshenin
11 years ago

If you'd like to make some data available to js from php, don't have a particular script file to enqueue that uses it, and don't need the dependency management WP_Dependencies gives you, you might as well just print your array with json_encode in wp_head, wp_footer or inline. Attaching your data to the jquery handler with wp_localize_script is a bad idea.

#11 @ocean90
11 years ago

  • Summary changed from wp_localize_script() doesn't work on admin_enqueue_scripts action to wp_localize_script() doesn't work for jquery handle

#12 follow-up: @azaozz
11 years ago

In theory there's no reason to skip localizations on handle aliases. In practice we will have to move any localizations from the alias to the first dependency so it's printed just in time. However that has a potential to overwrite a localization on that dependency. Will test the theory/try to make a patch.

#13 in reply to: ↑ 12 @ocean90
11 years ago

Replying to azaozz:

Will test the theory/try to make a patch.

Any results?

@azaozz
11 years ago

#14 @azaozz
11 years ago

Any results?

Yeah, we will need another loop through all handles right before printing/outputting them. That extra loop will slow this down.

This can be in print_scripts() in class.wp-scripts.php or in wp-dependencies.php so it works when adding styles too. The main logic is in 24197.patch and can probably be cleaned up a bit.

To test:

// localize on a group handle
add_action( 'admin_init', '_test_localize_empty' );
function _test_localize_empty() {
	wp_localize_script( 'jquery', 'myvars', array( 'foo' => 'bar' ) );
	// wp_localize_script( 'jquery-core', 'myvars', array( 'foo2' => 'bar2' ) );
}

If the first dependency has any localization (data), it will have priority over the data moved from the group handle (uncomment to test).

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

@azaozz
11 years ago

#15 @azaozz
11 years ago

24197-2.patch seems better. It adds the localization data to the first dependency instead of adding it to the alias handle in WP_Scripts::localize()

@nacin
11 years ago

#16 @nacin
11 years ago

Good work here, but I think we can go about this much simpler for 3.6. From IRC:

1:29	nacin	azaozz: for 3.6, that seems to be good, right?
1:29	azaozz	we may need to support localization on other aliases
1:29	nacin	azaozz: in the future if jquery becomes the main handle again and jquery-core is an alias, we can swap it.
1:30	nacin	azaozz: we didn't add any other aliases in 3.6, though. this is the one we're worried about breaking.
1:30	azaozz	right
1:30	nacin	in fact we have only four aliases total
1:31	nacin	jquery, plupload-all, swfupload-all, and scriptaculous
1:31	nacin	(based on a quick `scripts..add\(.*false.*array` regexp)
1:31	azaozz	yeah.
1:33	nacin	azaozz: so let's just do that

#17 @nacin
11 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 24635:

If someone tries to localize 'jquery', which is now an alias with jquery-core and jquery-migrate dependencies, add the data to jquery-core.

fixes #24197.

#18 @nacin
11 years ago

_doing_it_wrong() was considered, but is left out of 3.6 for now:

1:41 <azaozz> nacin: what text for the _doing_it_wrong: "Localization data should be added only to scripts enqueued by your plugin."
1:42 <nacin> azaozz: I thought about it but didn't care much for 3.6, because the message is going to be kind of vague and probably confusing
1:42 <nacin> attaching to 'jquery' is unexpected and stupid, but not inherently "wrong", and it'd likely confuse people
1:42 <azaozz> that's true too
1:43 <azaozz> ok, no _doing_it_wrong

Note: See TracTickets for help on using tickets.