Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#54529 closed defect (bug) (fixed)

Allow for `wp_register_script()` to be called after `wp_enqueue_script()`.

Reported by: dd32's profile dd32 Owned by: audrasjb's profile audrasjb
Milestone: 5.9 Priority: normal
Severity: normal Version: 5.9
Component: Script Loader Keywords: has-patch has-unit-tests commit has-dev-note
Focuses: Cc:

Description (last modified by dd32)

This is a partially thought out PR, in response to fixing issues with a plugin and FSE.

When a plugin registers styles/scripts on wp_enqueue_scripts (as we encourage plugins to do), and conditionally enqueues their script/style on the_content filter, things "just work".

Unfortunately with FSE, things do not just work.

In FSE themes, the_content is run prior to the header being processed, which results in the above scenario failing.

The attached PR makes a wp_enqueue_script( 'example' ); wp_register_script( 'example', 'https://.....' ); work, where as currently the enqueue silently fails (no doing it wrong, etc) and the following register has no impact.

Example plugin:

// Classic Theme: This will be called first
// FSE: This will be called second.
add_action( 'wp_enqueue_scripts', function() {
   wp_register_script( 'example', 'https://example.com/example.js' );
} );

// Classic Theme: This function will be called second, and enqueue the above script
// FSE: This function will be called first, and fail to enqueue
add_filter( 'the_content', function( $content ) {
   wp_enqueue_script( 'example' );

   return $content;
} );

(note: not tested, just written in trac)

I'm not convinced this is a perfect option to take, and FSE might be able to adjust how it's template loading is done, but it's done in the current way specifically so that blocks can enqueue scripts/styles into the header.

This also doesn't deal with certain functions, like wp_add_inline_style() that attach data to an existing registered item, only dealing with the enqueue happening before a register (or 'add' in WP_Dependancy terms).

Change History (13)

This ticket was mentioned in PR #1968 on WordPress/wordpress-develop by dd32.


3 years ago
#1

  • Keywords has-patch has-unit-tests added

#2 @dd32
3 years ago

  • Description modified (diff)

#3 @kraftbj
3 years ago

  • Milestone changed from Awaiting Review to 5.9
  • Version set to trunk

I'm adding this to the 5.9 milestone so it can be considered before FSE becomes stable. It seems like a good change to me and the ripe time to do it is before requiring plugin authors to update their code.

#4 @kraftbj
3 years ago

  • Type changed from enhancement to defect (bug)

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 years ago

#6 follow-up: @azaozz
3 years ago

  • Keywords commit added

The patch/PR looks good here, scripts can be enqueued and dequeued (by "handle") before they are registered.

This also doesn't deal with certain functions, like wp_add_inline_style() that attach data to an existing registered item.

Thinking that's okay. Enqueueing is often used as one-step registering + enqueueing. Then adding before/after should work. When doing two steps: registering and then enqueueing the before/after should always be added after the registering and seems it's going to work with this patch.

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

#7 in reply to: ↑ 6 ; follow-up: @dd32
3 years ago

Replying to azaozz:

This also doesn't deal with certain functions, like wp_add_inline_style() that attach data to an existing registered item.

Thinking that's okay.

As do I, it's not perfect, but it's not horrible.

This is an example of what wouldn't work though:

add_action( 'wp_enqueue_scripts', function() {
   wp_register_script( 'example', 'https://example.com/example.js' );
} );

add_filter( 'the_content', function( $content, $post ) {
   wp_enqueue_script( 'example' );
   wp_localize_script( 'example', 'exampleData', [ 'postID' => $post->ID ] );

   return $content;
}, 10, 2 );

but like @azaozz said, that doesn't seem like a real-world example, usually you'd have something more akin to this, which would still work, or you'd not use register and just a straight enqueue + url + localise at the same time.

add_action( 'wp_enqueue_scripts', function() {
   wp_register_script( 'example', 'https://example.com/example.js' );
   wp_localize_script( 'example', 'exampleData', [ 'url' => 'https://example.com/' ] );
} );

One thing I'm unsure of - The PR causes the enqueue to happen before the script register "in footer" call is made, but I'm not sure if that's actually a problem, as I don't know how the 'group' part is handled.. I think it's okay.

$registered = $wp_scripts->add( $handle, $src, $deps, $ver ); // it'll be enqueued at this point
if ( $in_footer ) {
	$wp_scripts->add_data( $handle, 'group', 1 ); // This might run after the enqueue.
}

https://github.com/WordPress/wordpress-develop/blob/2648a5f984b8abf06872151898e3a61d3458a628/src/wp-includes/functions.wp-scripts.php#L178-L181

(note: Can someone else who is more familiar with scripts/styles please test and commit, I'm way too rusty on these things)

Version 0, edited 3 years ago by dd32 (next)

#8 in reply to: ↑ 7 @azaozz
3 years ago

Replying to dd32:

When a plugin registers styles/scripts on wp_enqueue_scripts (as we encourage plugins to do),

This is actually wrong. Scripts should be registered on init, and enqueued on wp_enqueue_scripts.

As do I, it's not perfect, but it's not horrible.

Right. This is a "double" edge case :) When doing script registering and enqueueing separately scripts should be registered on init, and enqueued on wp_enqueue_scripts.

The case we're trying to accommodate is registering on wp_enqueue_scripts (that's late, this action runs after init), and then enqueueing on the wp_content filter by using it as an action (that can be late too, this filter usually runs in the middle of the HTML <body> but may run much earlier).

So generally there are two "doing it wrong" here, the first worse than the second. Wondering if there should be notices for them.

Also note that scripts can be registered and enqueued at the same time by using the wp_enqueue_script() function, see https://developer.wordpress.org/reference/functions/wp_enqueue_script/. Then it is the proper/expected way to use the wp_enqueue_scripts action. This will also help to mitigate the problem we're trying to solve here.

This is an example of what wouldn't work though:

Yeah, probably better to add that doing-it-wrong when registering a script from the wp_enqueue_scripts action.

One thing I'm unsure of - The PR causes the enqueue to happen before the script register "in footer" call is made, but I'm not sure if that's actually a problem

Seems this should work fine. This patch "holds" the actual enqueueing until the script is registered. Then the registering and enqueueing happens in the proper order.

The possible problems (as mentioned above) are from the fact that wp_enqueue_script() was called with a script handle only but the script is not yet registered until the registering happens. That means there cannot be any extras like localization, before/after, etc. However these extras are usually added after registering a script, not after enqueueing it. Frankly I'm not sure this rare edge case can be fixed easily.

#9 @audrasjb
3 years ago

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

Self assigning for test then commit as it looks like it's ready to go before Beta 2 today.

#10 @audrasjb
3 years ago

  • Keywords needs-dev-note added

Tested using the above example:

// Classic Theme: This will be called first
// FSE: This will be called second.
add_action( 'wp_enqueue_scripts', function() {
   wp_register_script( 'example', 'https://example.com/example.js' );
} );

// Classic Theme: This function will be called second, and enqueue the above script
// FSE: This function will be called first, and fail to enqueue
add_filter( 'the_content', function( $content ) {
   wp_enqueue_script( 'example' );

   return $content;
} );

## Test results

Before patch:

  • On a block theme, the example script is not printed on front-end.
  • On a Classic Theme, the example is printed on front-end.

After patch:

  • On a block theme, the example script is printed on front-end.
  • On a Classic Theme, the example is still printed on front-end.

Tests look good.

#11 @audrasjb
3 years ago

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

In 52338:

Script Loader: Allow for wp_register_script() to be called after wp_enqueue_script().

When a plugin registers styles/scripts on wp_enqueue_scripts (as plugin authors are encouraged to do), and conditionally enqueues their script/style on the_content filter, things "just work". In block themes, the_content is run prior to the header being processed, which results in the above scenario failing.

This change makes a wp_enqueue_script( 'example' ); wp_register_script( 'example' ); work, where as currently the enqueue silently fails (no "doing it wrong" message) and the following register has no impact. Scripts can therefore be enqueued and dequeued (by "handle") before they are registered.

Fixes #54529.

#13 @audrasjb
3 years ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.