Dont allow empty $src parameter in wp_register_script

[oskarhane]

In Codex the $src parameter in wp_register_script( $handle, $src, $deps, $ver, $in_footer ) is required but is allowed to be empty.

This makes it easy to make a plugin/theme that breaks other plugins.

Some themes have this code loaded in init-hook:

if (!is_admin()) {
	wp_deregister_script('jquery');                                   // De-Register jQuery
	wp_register_script('jquery', '', '', '', true);                   // It's already in the Header
}

If a plugin that uses jQuery is used on the login page, it'll be broken.
And you can't check if the script was correctly registered or not:

    //This will return true even though $src was empty.
    if(wp_script_is('jquery', 'registered'))

[SergeyBiryukov]

Related: #22470

[jeremyfelt]

In a case such as this, there's not much of a difference between an empty string and a URL that provides a broken version of jQuery or another library entirely. I can't see comprehensive validation being possible in this case. Suggest wontfix/close.

[oskarhane]

Replying to jeremyfelt:

In a case such as this, there's not much of a difference between an empty string and a URL that provides a broken version of jQuery or another library entirely. I can't see comprehensive validation being possible in this case. Suggest wontfix/close.

The difference is the intent of the developer. Leaving it empty vs. giving a broken url is a big difference in my book.

Maybe the $src shouldn't be required since it has no function then?
An alternative solution could be to add something like wp_script_src to get the $src for registered scripts.

[nacin]

Technically speaking, you can create an alias or group via an empty 'src'. As in, specify a handle and then say it has dependencies of X, Y, and Z. Enqueueing this one script then gives you X, Y, and Z. We use this a tiny bit in core.

Perhaps the bug is printing an empty "src" tag (if that's what we're doing) in lieu of nothing at all.

Additionally, perhaps [23378] also needs to check || $pagenow === 'wp-login.php' && current_filter() !== 'login_enqueue_scripts'.

[oskarhane]

Replying to nacin:

Technically speaking, you can create an alias or group via an empty 'src'. As in, specify a handle and then say it has dependencies of X, Y, and Z. Enqueueing this one script then gives you X, Y, and Z. We use this a tiny bit in core.

Perhaps the bug is printing an empty "src" tag (if that's what we're doing) in lieu of nothing at all.

Additionally, perhaps [23378] also needs to check || $pagenow === 'wp-login.php' && current_filter() !== 'login_enqueue_scripts'.

Good point with the grouping, I've never thought of that.

The situation is this: I've written a plugin that needs jQuery on the login page. Some themes reset (deregister and register with empty $src) jquery in the init-hook and register their own jquery in wp_enqueue_scripts. This leaves login_enqueue_scripts with an empty jquery.
Not printing an empty "src" tag won't help.

I'd rather not see all plugins that use login_enqueue_scripts deregister and register their own jquery just for the login page.

Adding your suggested check to [23378] should not help since the deregister/register empty is done in the init-hook?
Or am I reading it wrong?

[nacin]

[23378] prevents blind unregistration of jQuery (and a number of other critical scripts) on init. It allows it in the admin only if the person knows what they are doing (which we infer by their use of the admin_enqueue_scripts hook).

There is also a login_enqueue_scripts hook that we can use to infer if someone on wp-login.php knows what they are doing and should be allowed to unregister jQuery. Otherwise we can just block the unregistration itself.

[oskarhane]

Replying to nacin:

[23378] prevents blind unregistration of jQuery (and a number of other critical scripts) on init. It allows it in the admin only if the person knows what they are doing (which we infer by their use of the admin_enqueue_scripts hook).

There is also a login_enqueue_scripts hook that we can use to infer if someone on wp-login.php knows what they are doing and should be allowed to unregister jQuery. Otherwise we can just block the unregistration itself.

Yes, of course, I read it too fast and read "!==" as "==".
Adding that check would fix this issue!

[SergeyBiryukov]

Replying to nacin:

Additionally, perhaps [23378] also needs to check || $pagenow === 'wp-login.php' && current_filter() !== 'login_enqueue_scripts'.

[atimmer]

​25017.1.diff adds a check to make sure you cannot deregister jquery on the login page.

You can ignore ​25017.1-alt.diff

[nacin]

Looks good. I am going to simplify 25017.1.diff​ a bit.

[nacin]

In 25443:

Don't allow critical scripts to be deregistered on wp-login.php, just as we do in the rest of the admin.

props atimmer.
fixes #25017.

[benfreke]

This fix works, so I've left this closed. However, if a theme runs a check of is_admin() before deregistering the jquery, you get a message about not using wp_deregister_script in the administration screen when you attempt to login (with WP_DEBUG set to true, obviously).

Could I make a suggestion that the language or the error message be updated to reflect the fact that this check is no longer in just the administration? Ideally, pull the check completely out of the function and create a new function. This should also allow for quicker future updates if other areas of WordPress need to use this check.

Pseudo code:

if (is_admin()) {
  _keep_core_scripts_registered($handle, 'administration area');
} elseif (is_login()) {
  _keep_core_scripts_registered($handle, 'login screen');
}

protected function _keep_core_scripts_registered($handle, $area)
{
  // array of protected script handles
  // filter to add/remove scripts from above array
  if ( not allowed to be deregistered) {
    // as per existing function with second param passed to sprintf
  }
}

[SergeyBiryukov]

Replying to benfreke:

However, if a theme runs a check of is_admin() before deregistering the jquery, you get a message about not using wp_deregister_script in the administration screen when you attempt to login (with WP_DEBUG set to true, obviously).

I can't reproduce the warning as long as the correct hook (login_enqueue_scripts or admin_enqueue_scripts) is used, with or without is_admin() check.

Could I make a suggestion that the language or the error message be updated to reflect the fact that this check is no longer in just the administration?

Feel free to open a new ticket for that, but the current text seems accurate enough to me, even though the login screen is not mentioned there. The point is to prevent developers from using init (as in the ticket description) or other incorrect hooks for wp_deregister_script(), and point them to the correct hook.

Ideally, pull the check completely out of the function and create a new function.

That might be an unnecessary abstraction.

[nacin]

benfreke, you make a great point. Thanks for the comment. A change would mean another string for translators. Not a big deal, but while 25017.1.diff​ originally had a wp-login.php-specific block, I just didn't find it to be necessary at the time.

While is_admin() is false on wp-login.php, it's intrinsically part of "the administration area". It needs to be clear that their is_admin() check remains wrong and insufficient, and that they should be using the correct hook. If the message said "don't do this on login" the next thought will likely be "oh, I'm already excluding the admin, so I should also exclude login". I'd rather be a little vague upfront (especially since we don't actually know what they are checking) and unambiguous in how to correct it.

Alas, I'm most likely reading way too far into the psychology of error messages.

[benfreke]

Hi @SergeyBirukov and @nacin. Thanks for taking the time to reply. I think I got too close to the code, and I now agree with both of you that the existing message makes sense and points developers to the right solution.