WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 9 months ago

Last modified 9 months ago

#24197 closed defect (bug) (fixed)

wp_localize_script() doesn't work for jquery handle

Reported by: IcyPixels Owned by: 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 10 months ago.
24197-2.patch (1.9 KB) - added by azaozz 10 months ago.
24197.diff (622 bytes) - added by nacin 9 months ago.

Download all attachments as: .zip

Change History (21)

comment:1 SergeyBiryukov12 months 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 trying to use icy_ajax without checking if it was added.

My steps:

  1. Enqueued my own script 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.
Version 0, edited 12 months ago by SergeyBiryukov (next)

comment:2 t31os_12 months 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 12 months ago by t31os_ (previous) (diff)

comment:3 IcyPixels12 months 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

comment:4 ocean9012 months ago

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

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

comment:5 IcyPixels12 months 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.

comment:6 azaozz12 months ago

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

comment:7 somatic12 months 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...

comment:8 azaozz12 months 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.

comment:9 somatic12 months 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).

comment:10 kovshenin12 months 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.

comment:11 ocean9012 months 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

comment:12 follow-up: azaozz11 months 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.

comment:13 in reply to: ↑ 12 ocean9010 months ago

Replying to azaozz:

Will test the theory/try to make a patch.

Any results?

azaozz10 months ago

comment:14 azaozz10 months 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 10 months ago by azaozz (previous) (diff)

azaozz10 months ago

comment:15 azaozz10 months 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()

nacin9 months ago

comment:16 nacin9 months 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

comment:17 nacin9 months 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.

comment:18 nacin9 months 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.