Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[New sniff] Verify that wp_deregister_script() isn't called #21

Closed
3 tasks
carolinan opened this issue Jul 12, 2016 · 11 comments
Closed
3 tasks

[New sniff] Verify that wp_deregister_script() isn't called #21

carolinan opened this issue Jul 12, 2016 · 11 comments
Assignees
Milestone

Comments

@carolinan
Copy link

carolinan commented Jul 12, 2016

Rule:

WARNING : Themes must not deregister core scripts.

Ref: https://make.wordpress.org/themes/handbook/review/required/#stylesheets-and-scripts

"Required to use core-bundled scripts rather than including their own version of that script. For example jQuery."

This is basically meant to only check that core scripts aren't being deregistered, however maintaining a list of core scripts for that purpose would be a maintenance nightmare, so returning a warning when any such call is encountered is the current solution.

Theme check file covering this rule:

https://github.com/Otto42/theme-check/blob/master/checks/deregister.php

To do:

  • Add / clarify the rule in the Theme Review handbook to the Requirements page.
  • Create unit tests
  • Create new sniff
@Pross
Copy link

Pross commented Jul 12, 2016

See #26

@grappler
Copy link
Member

@WPTRT/core Can you think of any reason why a them should be able able to deregister a script? I can' think of any. If we can't think of any we could change the sniff from a WARNING to an ERROR.

@justintadlock
Copy link

justintadlock commented Jul 14, 2016

There's definitely at least one reason. For example, a theme might want to run a custom media player on the front end. It could deregister or dequeue the core scripts and roll its own. I do this with styles and don't think it's a stretch to think that a theme could do it with the media player scripts.

That's all I got at the moment.

@Pross
Copy link

Pross commented Jul 14, 2016

So is the answer to check for deregistering certain scripts?

@justintadlock
Copy link

justintadlock commented Jul 14, 2016

Definitely. We can stop themes from deregistering core WP scripts (can always dequeue in my example above). Themes must have the ability to deregister parent theme and/or plugin scripts.

@Pross
Copy link

Pross commented Jul 14, 2016

Should we include ALL scripts from https://developer.wordpress.org/reference/functions/wp_enqueue_script/#defaults or should a list be drawn up?

@justintadlock
Copy link

I'd just include the most obvious problems in an error, which are probably jquery (and maybe masonry). I don't think I've seen a theme attempt to deregister anything else.

The entire reason behind this came about because we had themes trying to deregister the core jQuery and register their own. So, the core of the problem is this:

wp_deregister_script( 'jquery'

wp_register_script( 'jquery'

Catch those two things and you've caught 99% of the problem.

@Pross
Copy link

Pross commented Jul 14, 2016

Updated PR does exactly this ^

@jrfnl
Copy link

jrfnl commented Jul 18, 2016

From: #26 (comment)

Just a thought: what about keeping this list in line with the scripts WP core protects itself against from being deregistered on the admin side ?
The list can be found in the code of this function: https://developer.wordpress.org/reference/functions/wp_deregister_script/

The current PR #26 checks against jquery and is about ready for merge consideration.

Is there a decision that that's enough or are what we have currently opinions and does this still have to go to the Theme Review Board for definite approval ?

/cc @grappler

@justintadlock
Copy link

I think we're probably OK with this. If we need to add more, the code already looks like it's set up for just plugging in new script handles.

@dingo-d
Copy link
Member

dingo-d commented May 18, 2019

The sniff for this exists: NoDeregisterCoreScript. If this is correct @jrfnl can you close this issue?

@jrfnl jrfnl closed this as completed May 19, 2019
@jrfnl jrfnl added this to the 0.1.0 milestone Jun 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants