Make WordPress Core

Opened 11 years ago

Closed 8 years ago

#23816 closed enhancement (maybelater)

wp_enqueue_script|style() dependency failures are silent

Reported by: johnbillion's profile johnbillion Owned by: rmccue's profile rmccue
Milestone: Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: needs-docs needs-patch
Focuses: Cc:

Description (last modified by markjaquith)

If I call wp_enqueue_script() or wp_enqueue_style() with a dependency that doesn't exist then I don't get told about it.

Example:

wp_enqueue_script(
	'my-script',
	get_stylesheet_directory_uri() . '/my-script.js',
	array( 'script-that-does-not-exist' )
);

Actual result: nothing is enqueued, and no notices are thrown.

Expected result: When WP_DEBUG is true, I should get a PHP notice warning me that the dependency script doesn't exist.

Attachments (3)

23816.diff (1.5 KB) - added by rmccue 9 years ago.
Fire an action if dependencies fail
23816.2.diff (3.5 KB) - added by rmccue 9 years ago.
Add two filters (one per dependency type)
23816.3.diff (4.0 KB) - added by rmccue 9 years ago.
Trigger errors on development by default

Download all attachments as: .zip

Change History (16)

#1 follow-up: @SergeyBiryukov
11 years ago

Actual result: 'my-script' gets enqueued

Could not reproduce that. My steps:

  1. Created an empty js/my-script.js file in Twenty Thirteen child theme.
  2. Added this to child theme's functions.php:
    function twentythirteen_my_scripts() {
    	wp_enqueue_script(
    		'my-script',
    		get_stylesheet_directory_uri() . '/js/my-script.js',
    		array( 'script-that-does-not-exist' )
    	);
    }
    add_action( 'wp_enqueue_scripts', 'twentythirteen_my_scripts' );
    
  3. The script was not enqueued.
  4. Once I replaced 'script-that-does-not-exist' with 'jquery', the script was enqueued.

#2 in reply to: ↑ 1 @johnbillion
11 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

Replying to SergeyBiryukov:

Could not reproduce that. The script was not enqueued.

Confirmed. Enqueing a file with a missing dependency does not enqueue the file. I'm not sure what I was doing.

#3 @markjaquith
10 years ago

  • Component changed from General to Script Loader
  • Description modified (diff)
  • Milestone set to Awaiting Review
  • Priority changed from low to normal
  • Resolution invalid deleted
  • Status changed from closed to reopened

Opening this back up. The original report is mostly valid. If you register/enqueue something with a bad dependency, you get silent failure. Nothing is output, and no notices are thrown. I typo'd 'wp-utils' instead of 'wp-util' and it was maddening trying to figure out why no scripts were printing.

@rmccue
9 years ago

Fire an action if dependencies fail

#4 @rmccue
9 years ago

Constantly running into this. Added a patch that fires an action here, allowing tooling to report a broken dependency chain, but this doesn't report anything in core. We might want to move this into the subclasses instead and simply call it from the parent to allow more specific hooks, plus differentiation between scripts and styles.

(Note: this file is technically part of BackPress, so I'm not sure if we should be doing something with actions. There aren't any in the file currently. That said, BackPress hasn't been synched in a long time, so I don't think it's an issue.)

We could also add a default handler into core that outputs these as HTML comments if you have WP_DEBUG on and we're doing_action( 'wp_head' )

@rmccue
9 years ago

Add two filters (one per dependency type)

#5 @rmccue
9 years ago

  • Keywords has-patch added

Updated patch; now adds wp_scripts_missing_dependency and wp_styles_missing_dependency as separate filters.

#6 follow-up: @johnbillion
9 years ago

  • Keywords needs-docs added

We might as well go the whole hog and add a default hook onto those actions which triggers a PHP notice when WP_DEBUG is on.

The third parameter needs adding to the hook docs too.

@rmccue
9 years ago

Trigger errors on development by default

#7 in reply to: ↑ 6 @rmccue
9 years ago

Replying to johnbillion:

We might as well go the whole hog and add a default hook onto those actions which triggers a PHP notice when WP_DEBUG is on.

The third parameter needs adding to the hook docs too.

Fixed on both by doing it inline, ala _doing_it_wrong/_deprecated_{file,function}. Can switch to a default filter if you'd prefer, but it's not really reusable code since it has different messages for each.

#8 @rmccue
9 years ago

  • Keywords needs-patch added; has-patch removed

Ran into an issue with this one. Dependency resolution takes place for all scripts in the header, and again in the footer. If you register a script to output in the footer, but it depends on something registered after wp_head, you get an error despite the script being output correctly.

We need to take this into account when passing in $handles to all_deps; an array_filter should suffice.

#9 @rmccue
9 years ago

  • Owner set to rmccue
  • Status changed from reopened to assigned

#10 @chriscct7
8 years ago

  • Keywords has-patch needs-refresh added; needs-patch removed

#11 @johnbillion
8 years ago

  • Keywords needs-patch added; has-patch needs-refresh removed

Determining whether an asset actually has a missing dependency is not that straight forward. Query Monitor has a component which detects broken dependencies and handles the header/footer situation that Ryan mentioned above.

#12 @rmccue
8 years ago

@johnbillion Do you have a suggested path forward on this? Should we punt from core in favour of plugin territory?

#13 @johnbillion
8 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from assigned to closed

If someone wants to work on this, feel free to re-open with a patch.

Note: See TracTickets for help on using tickets.