Password strength meter's JS re-uses theme compat. asset handle

[DJPaul]

Per r8686, the locate_asset_in_stack method is used to fine a non-"core" asset. A shortcoming of this implementation is how the method generates the asset name; it's always one of "bp-child","bp-parent", "bp-legacy"; and the type of media (JS/CSS) is prepended to the end.

This works fine to load, for example, the main JS file from the bp-legacy pack. However, when a second JS file is loaded, it uses the exact same asset name, making the original unenqueue-able.

$filename = "password-verify{$min}.js"; 

// Locate the Register Page JS file 
$asset = $this->locate_asset_in_stack( $filename, 'js' );

$asset ends up looking like this:

array (size=2)
  'location' => string '../bp-legacy/js/password-verify.js' (length=97)
  'handle' => string 'bp-legacy-js' (length=12)

This is blocking #5797 which is to make the asset name somehow discoverable so that other developers can reliably un-enqueue these scripts.

[DJPaul]

How about adding a third parameter onto locate_asset_in_stack? I think this would provide a straightforward fix for #5797 as well.

[boonebgorges]

I'm definitely fine with adding another param to make the asset handle customizable.

I'm wondering whether $script_name should completely override the existing logic, or whether we should maintain the $location_type prefix. Even in the case of custom scripts, it seems as if it would be useful to maintain information about where in the stack the asset is found. Thus:

$retval['handle']   = ( $script_name ) ? "{$location_type}-{$script_name} : "{$location_type}-{$type}"; 

I suppose an argument against this is that our "main" assets - buddypress.css and buddypress.js don't have a natural "name" - we just call them 'bp-legacy-css' etc.

What do you think, DJPaul? Am I overthinking it?

[DJPaul]

Is there a use case for these "main" scripts being named in a location-specific way? I don't think there is one. If you want to dequeue, I think you should just be able to say dequeue( 'bp-classic-js' ) etc.

I think if we can identify a plausible use case, we should store the location type as a meta attribute via wp_style_add_data.

[boonebgorges]

Is there a use case for these "main" scripts being named in a location-specific way?

Only if we want to support their being loaded from anywhere in the template stack. It's possible that someone might not like our password meter JS, and want to override it in a child theme. The current locate_asset_in_stack() call means that this use case is technically supported.

[DJPaul]

Sorry Boone, I don't understand. I am only suggesting we override the $handle for the wp_enqueue_script() call. I am not saying we should change in any way the existing process of locating where the file is actually on the hard drive, only its $handle.

For example:
BP finds password-script.js in the child theme, its handle becomes password-script.
BP finds password-script.js in the theme compat. folder, its handle is still password-script.

If I wanted to de-queue that file on a custom site, I'd wp_dequeue_script( 'password-script' ) and I know it would always work, regardless of where the actual file is located in the site's files.

[boonebgorges]

Sorry Boone, I don't understand. I am only suggesting we override the $handle for the wp_enqueue_script() call. I am not saying we should change in any way the existing process of locating where the file is actually on the hard drive, only its $handle.

I'm not sure I totally understand either, but I think it's partially because the logic used for script naming here is a bit wonky. Check out ​https://buddypress.trac.wordpress.org/browser/tags/2.2.0/src/bp-templates/bp-legacy/buddypress-functions.php#L208. Our CSS and JS are enqueued according to the 'handle' returned by locate_asset_in_stack(). In each case, the $location_type of the located file is included in that handle ​https://buddypress.trac.wordpress.org/browser/tags/2.2.0/src/bp-templates/bp-legacy/buddypress-functions.php#L342. So we have wp_enqueue_script( 'bp-legacy-js' ), wp_enqueue_script( 'bp-parent-js' ), or wp_enqueue_script( 'bp-child-js' ), based on where the JS file is found.

I don't remember why I originally made these handles dynamic, but it's the root cause of what's happening in #5797. So maybe it is, in fact, better to have non-dynamic handles like you suggest in your patch for 'password-verify'. My only point is that it would be out of sync with the way enqueuing currently works in bp-legacy.

[DJPaul]

I think non-dynamic handles are the way to go because it makes it easier to (document how to) unhook them. I don't think including location metadata in the $handle is useful; better to have that as metadata somewhere else. Let me iterate on the patch and see if I can come up with something wrt this.

We could leave the existing bp-legacy naming as a quirk, and use non-dynamic names for other scripts like "password-verify".

[boonebgorges]

That sounds good to me. Thanks, DJPaul.

[DJPaul]

...including location metadata in the $handle is useful; better to have that as metadata somewhere else. Let me iterate on the patch and see if I can come up with something wrt this.

I didn't get very far; there's wp_style_add_data but not anything similar that I can find for wp_script_add_data.

[djpaul]

In 9524:

BP-Legacy: add a new param to locate_asset_in_stack to allow the enqueued asset's $handleto be explicitly set.

Originally designed to load a template pack's main CSS and JS files in a theme compatibility manner, in r8686 locate_asset_in_stack was used to enqueue a JS file for password strength meters (registration and settings screens). This worked, but the $handle, dynamically constructed and passed to wp_enqueue_script, for the password strength JS file is likely to be the exact same handle as that used for a template pack's main JS file. This has the consequence of making the file(s) registered first unenqueue-able.

The new parameter introduced by this change allows us to explicitly set the name of an enqueued asset's handle, which avoids this problem. Technically the script handle for the password strength meter JS was never intentionally set, so we avoid most of the concerns of a backwards compatibility break, and now allow developers to actually unenqueue a template pack's CSS and JS.

Fixes #5972.
See also #5797.