WordPress.org

Make WordPress Core

Opened 22 months ago

Closed 22 months ago

Last modified 18 months ago

#20971 closed defect (bug) (fixed)

Delaying wp_default_scripts() prevents enqueing

Reported by: nacin Owned by: nacin
Milestone: 3.4.1 Priority: normal
Severity: major Version: 3.4
Component: General Keywords: has-patch commit
Focuses: Cc:

Description

wp_enqueue_script() only works on a handle that is already registered. That means that the delaying tactic in [19865] can break JavaScript enqueueing of default scripts (mostly by plugins, potentially by themes), as they no longer work until init has fired.

This is significantly mitigated by our change in 3.3 that caused some trouble, as most people fixed their code then, and more developers are watching for _deprecated and _doing_it_wrong notices. But, it still affects some poorly coded themes.

We can fix this by double-calling wp_default_scripts() — calling it immediately but then calling it again on init in case it fires too early.

The double-calling isn't new for us -- we do this when registering default taxonomies and post types as well.

Attachments (2)

20971.diff (527 bytes) - added by nacin 22 months ago.
20971.2.diff (11.9 KB) - added by nacin 22 months ago.

Download all attachments as: .zip

Change History (16)

nacin22 months ago

comment:1 azaozz22 months ago

wp_enqueue_script() does both: $wp_scripts->add() and then $wp_scripts->enqueue(). When it's called too early, wondering if we could delay these two calls until the right time, i.e. the 'wp_enqueue_scripts' or 'admin_enqueue_scripts' actions.

Another way to fix this would be to have a "default order" for some of the core scripts like Prototype before jQuery, libraries first, etc. Been thinking to add this to WP_Scripts and possibly to WP_Styles (and get rid of wp_prototype_before_jquery()).

comment:2 scribu22 months ago

wp_enqueue_script() only works on a handle that is already registered.

Why don't we fix that instead?

comment:3 follow-up: scribu22 months ago

Actually, nevermind. I assume you mean calling wp_enqueue_script( 'jquery' ). I was thinking about using unregistered dependencies: wp_enqueue_script( 'my-js', ..., array( 'jquery' ) ).

Last edited 22 months ago by scribu (previous) (diff)

comment:4 in reply to: ↑ 3 azaozz22 months ago

Replying to scribu:

Yes this works:

wp_enqueue_script( 'my-js', 'url', array( 'jquery' ), ... );

And this doesn't work when my-other-js is not registered beforehand:

wp_enqueue_script( 'my-js', 'url', array( 'my-other-js' ), ... );

comment:5 scribu22 months ago

Ok, but that seems like an artificial limitation, since some other code can call wp_deregister_script( 'my-other-js' ) afterwards.

We should check the dependencies right before outputting the script tags, not before.

Last edited 22 months ago by scribu (previous) (diff)

comment:6 scribu22 months ago

What I mean is that this should work:

wp_enqueue_script( 'my-js', 'url', array( 'my-other-js' ), ... );

wp_register_script( 'my-other-js', ... );

If that would not fix this bug, I apologize for the misunderstanding.

Last edited 22 months ago by scribu (previous) (diff)

comment:7 nacin22 months ago

  • Owner set to nacin
  • Status changed from new to assigned

nacin22 months ago

comment:8 nacin22 months ago

  • Keywords has-patch commit added

20971.2.diff does the following:

  • On WP_Scripts construction, it has wp_default_scripts() fire immediately and again on init, rather than only firing immediately (3.3) or only firing on init (3.4). By firing it immediately, this allows a default-registered handle to be enqueued prior to init. By firing it again on init, it makes sure that all localizations are properly translated.
  • Any localize() calls in script-loader.php are delayed until did_action('init'). This is necessary because calling localize() multiple times does result in duplicative JavaScript. This allows us to add scripts immediately but delay their localization until init.
  • did_action('init') is true the moment init fires (even before it completes), and the add_action( 'init' ) to re-fire wp_default_scripts() only works before the moment that init fires. So normally, WP_Scripts would only be instantiated once.
  • It provides coverage in load-scripts.php for did_action()'s use in script-loader.

This works in all of the following issues:

  • When a default handle is registered prior to init. For example, this in a plugin:
    wp_enqueue_script( 'jquery-ui-droppable' );
    
  • When a default handle that has a localize() component is registered prior to init. For example, this in a plugin:
    wp_enqueue_script( 'password-strength-meter' );
    
  • When a handle and source is registered prior to init. For example, this in a plugin:
    wp_enqueue_script( 'some-random-script',
        'http://s.gravatar.com/js/gprofiles.js' );
    
  • When a handle and source that has a localize component is registered prior to init. Note that localize() won't re-run on init; so any translations *won't work*. We still need localize() to run as they may be passing other kinds of data to the script. For example, this in a plugin:
    wp_enqueue_script( 'some-random-script',
        'http://s.gravatar.com/js/gprofiles.js' );
    wp_localize_script( 'some-random-script', 'someRandomL10n',
        array( 'test' => __( 'Post' ) );
    

Wrapping all of these in add_action( 'admin_enqueue_scripts', function() { and } ); should similarly get you the same results. The only exception would be that someRandomL10n's 'Post' string would properly be translated when fired on admin_enqueue_scripts.

comment:9 nacin22 months ago

MarkJaquith and I thoroughly flexed this. IRC log

comment:10 nacin22 months ago

In [21132]:

If a plugin triggers the WP_Scripts constructor prior to init, fire wp_default_scripts()
then and again on init.

Only add our localized strings once we've fired init, in case we do have the situation
where we fire wp_default_scripts() twice.

Fixes issues where plugins or themes try to enqueue a default script handle prior to
init. Does not allow #19959 to regress.

see #20971 for trunk.

comment:11 nacin22 months ago

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

In [21134]:

If a plugin triggers the WP_Scripts constructor prior to init, fire wp_default_scripts()
then and again on init.

Only add our localized strings once we've fired init, in case we do have the situation
where we fire wp_default_scripts() twice.

Fixes issues where plugins or themes try to enqueue a default script handle prior to
init. Does not allow #19959 to regress.

fixes #20971 for 3.4.

comment:12 follow-up: azaozz22 months ago

If we are going to continue supporting "bad" plugins, wouldn't it be better to abstract this

did_action( 'init' ) && $scripts->localize( 'handle', 'handleL10n', array(...) );

into another method? So add localise_on_init() or similar to WP_Scripts and call that from script-loader.

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

Replying to azaozz:

If we are going to continue supporting "bad" plugins, wouldn't it be better to abstract this

did_action( 'init' ) && $scripts->localize( 'handle', 'handleL10n', array(...) );

into another method? So add localise_on_init() or similar to WP_Scripts and call that from script-loader.

Maybe. I was mainly concerned with keeping the diff small and logic simple for 3.4.1. We discussed a few options in that IRC log.

The main issue about supporting these "bad" plugins is that they have no idea they are doing things wrong because everything works as normal. The only exception is it breaks JS strings in localized installs, which most developers will never notice.

For WP_Scripts to work before init was a flaw in our original API. We have been issuing _doing_it_wrong() since 3.3, and, along with the bugs in 3.3 (fixed in 3.3.1) and 3.4 (fixed in 3.4.1), most developers have indeed fixed their stuff. So I'd probably hesitate to do anything further here.

comment:14 SergeyBiryukov18 months ago

#20976 was marked as a duplicate.

Note: See TracTickets for help on using tickets.