Add rule to prevent deregistering scripts. #26

Pross · 2016-07-12T22:15:09Z

Added wp_deregister_script with a unit test.

Not sure if this is the way to do this, please let me know if it isnt!

jrfnl · 2016-07-12T22:32:41Z

Hi @Pross Thanks for this! Great to see the enthousiasm & willingness to contribute to this project.

In this case, your PR does need some work. Are you open for some coaching from me on this ?

Pross · 2016-07-12T22:33:55Z

Of course!

PM me on Slack.

jrfnl · 2016-07-12T23:35:19Z

As discussed on Slack - PR reviews will take place here on GH as that way it can also be used as a reference for future contributors.

As you're very quick to the party (quicker than I expected others to start contributing code), there will be a few start up hickups due to some changes we've initiated in the WPCS project.

There is a PR open in WPCS at the moment to start checking for code style. As we very much would like the theme review branch to comply, I intended to rebase the theme review branch once that was merged and then to rebase my own open PRs on the rebased theme review branch. Yeah, I know, a bit of a hassle, but this would mean we'd have a clean code base to start from and the means to keep it that way.

Once all the rebasing is done, we'll start merging PRs for items which have either been approved or are already well documented in the handbook.

I hope you don't mind, but that does imply that I will ask you to rebase your PR some time over the next few days as well (hoping the open PR will be merged soon). So please bear with me on that.

Having said all that, I'll now start leaving some feedback on the code ;-)

Oh and one last note: you may want to consider using named feature branches forked from the feature/theme-review-sniffs branch. That way it'll be easier to contribute to different issues and keeping the code changes for each issue isolated.

jrfnl · 2016-07-12T23:47:06Z

WordPress-Theme/ruleset.xml

@@ -3,7 +3,7 @@
 	<!-- For more information: https://make.wordpress.org/themes/handbook/review/ -->
 	<description>Standards any Theme to be published on wordpress.org should comply with.</description>

-
+	<rule ref="WordPress.Functions.FunctionRestrictions"/>


We'll be adding a new folder to the WordPress/Sniffs and WordPress/Tests directories names Theme. That folder will contain all sniffs (and it's sister the unit tests) which are very specific to the theme review process.

If you have a look at PR #10 where I've done something similar to what you're doing here, you can see what I mean and what that will look like.

Once the first custom sniff which will live in the Theme folder has been merged, the rule <rule ref="WordPress-Theme"/> will be added to the WordPress-Theme/ruleset.xml. From then on, all sniffs contained within the Theme folder will automatically be registered and run when running PHPCS against the new WordPress-Theme standard.

For sniffs to be re-usable, it is also important to keep them modular and name them for their specific function.
In this case, may I suggest naming the sniff DeregisterScript ?

Looks like I can simply add to that PR anyway.

I'd rather you don't.
It'll be so much easier to manage the project if every rule has their own sniff. If a rule changes, we change the sniff without potentially impacting other sniffs.

Also, having each rule in a separate sniff will also allow for more modular disabling of sniffs.
While this won't be allowed for wp.org (of course!), I can imagine, commercial theme hosting parties which also have some form of theme review in place creating their own rule set based on the wp.org Theme ruleset and just selectively excluding some rules.

jrfnl · 2016-07-12T23:59:38Z

Ok, review notes are inline. As a side-note: as mentioned above, WPCS will start checking for code style. The current files do not comply.
All the same, I wouldn't worry about it for now. Once the whole rebasing is done, you can check it yourself against the WPCS internal ruleset or wait for travis to tell you once the PR has been updated.

Pross · 2016-07-13T16:50:42Z

Updated the PR.

Moved sniff to the Theme folder as per PR10 Also renamed the classes/files and made the examples a bit more realistic.

jrfnl · 2016-07-14T07:18:44Z

WordPress-Theme/ruleset.xml

@@ -3,7 +3,7 @@
 	<!-- For more information: https://make.wordpress.org/themes/handbook/review/ -->
 	<description>Standards any Theme to be published on wordpress.org should comply with.</description>

-
+	<rule ref="WordPress.Theme.DeregisterScript"/>


No need to specify the specific rule. All sniffs in the Theme folder will be automatically added once the below has gone into the ruleset (and it will as soon as we merge the first custom sniff).

<rule ref="WordPress.Theme/>

jrfnl · 2016-07-14T08:08:04Z

P.S.: I've changed the issue title to better cover the PR. Hope you don't mind.

Pross · 2016-07-14T15:24:24Z

Closing for rewrite.

grappler · 2016-07-14T15:25:46Z

@Pross You don't need to close the PR. You can just reset the branch and start again.

Pross · 2016-07-14T15:59:55Z

Any idea how? Do I have to revert my commits?

grappler · 2016-07-14T17:30:12Z

WordPress/Sniffs/Theme/NoDeregisterCoreScriptSniff.php

+ * @author   Simon Prosser <pross@pross.org.uk>
+ */
+class WordPress_Sniffs_Theme_NoDeregisterCoreScriptSniff extends WordPress_Sniffs_Functions_FunctionRestrictionsSniff
+{


Please bring the bracket on the same line as the class name.

grappler · 2016-07-14T17:33:42Z

The check failed because of these styling issues.

FILE: ...Standards/WordPress/Sniffs/Theme/NoDeregisterCoreScriptSniff.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 50 | ERROR | [x] Functions must not contain multiple empty lines in a
    |       |     row; found 2 empty lines
    |       |     (Squiz.WhiteSpace.SuperfluousWhitespace.EmptyLines)
 58 | ERROR | [x] Expected 1 space after IF keyword; 0 found
    |       |     (Squiz.ControlStructures.ControlSignature.SpaceAfterKeyword)
 58 | ERROR | [x] Space after opening control structure is required
    |       |     (WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceAfterStructureOpen)
 58 | ERROR | [x] No space before opening parenthesis is prohibited
    |       |     (WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceBeforeOpenParenthesis)

jrfnl · 2016-07-15T04:09:04Z

WordPress/Sniffs/Theme/NoDeregisterCoreScriptSniff.php

+
+		$content = trim( $token['content'], '"\'' );
+
+		$functions = array(


I would suggest making both the $functions array as well as the $scripts array class properties.

…ionParam sniff

PR has been updated

jrfnl · 2018-06-10T04:23:54Z

I've rebased & updated the sniff.

This should now be ready for a final review & merge.

Update notes:

Updated for PHPCS 3.x and WPCS 0.14.x compatibility
Updated based on all the feedback found in the complete thread:
- Update link to handbook.
- Make the sniff slightly less easy to bypass by doing a quick check on the raw value of the parameter first and doing a more rigorous check if that yielded no results.
- Abstracted the throwing of the actual error out to a separate method to make this class more easily extendable. This allows for adding a separate sniff to detect calls to wp_deregister_style() to unregister WP Core external stylesheets which basically needs the same logic.
- If a variable was detected in $handle, throw a warning. Similar to the error, the warning is thrown from a separate method for easier extending of the class.
- Updated and added more unit tests.
Simplified the list of core scripts
As the array is only checked with isset(), there is no need for the value to have any meaning. This makes maintenance of the array simpler.

To do at a later stage:

Review the list of core scripts to make sure it's still up-to-date.
Document when the list was last reviewed and against what WP core file the review was done.
If there is no pertinent reason for the current list order, re-order the list to alphabetical.

jrfnl · 2018-06-10T09:03:44Z

FYI: I've added one more commit to make the "dynamic handle" check yet more rigorous. This last commit can be squashed into the "update based on feedback" commit before merge.

jrfnl · 2018-06-10T09:09:10Z

FYI: And another commit updating the docblock for the script list. This one should probably be squashed into either or the last two commits before merge.

grappler · 2018-06-10T10:39:49Z

WordPress/Tests/Theme/NoDeregisterCoreScriptUnitTest.inc

@@ -22,6 +22,9 @@ wp_deregister_script( 'themename-' . 'common' );
 */
 wp_deregister_script( $prefix . '-common' ); // Warning.
 wp_deregister_script( $something, 'jquery' ); // Warning for variable. Having the handle as second param will not work anyway, so no error.
+wp_deregister_script( self::$script_handle, 'jquery' ); // Warning for variable/dynamic param.
+wp_deregister_script( parent::SCRIPT_HANDLE, 'jquery' ); // Warning for variable/dynamic param.
+wp_deregister_script( get_script_handle(), 'jquery' ); // Warning for variable/dynamic param.


The second argument is not valid. Is there a reason to add it?

Ah, that was only meant to be in the test on line 24, will remove them from the others. And yes, it is just testing that the correct param is being checked.

grappler

All the mentioned items could be fixed in a future PR if you think that is best.

grappler · 2018-06-10T18:51:56Z

WordPress/Sniffs/Theme/NoDeregisterCoreScriptSniff.php

+			}
+
+			$text .= $this->strip_quotes( $this->tokens[ $i ]['content'] );
+		}


We were discussing in WordPress/WordPress-Coding-Standards#1364 about a utility function to return a string of all of the text string tokens.

I was wondering if it would be better to split the $found_variable_token from the $text code. This change would be my suggestion. https://gist.github.com/grappler/10dc57b4b3ab71a35e2fe925ef4da1ec The Unit Tests pass with this change.

When I wrote that code, I chose not to do so for efficiency reasons. The findNext() function walks the token array as well, so with your example code, there would be two foreach()-es walking the same tokens, while the current code does the same while only walking the tokens once.

grappler · 2018-06-10T18:53:41Z

WordPress/Sniffs/Theme/NoDeregisterCoreScriptSniff.php

+		$matched_parameter = $this->strip_quotes( $parameters[1]['raw'] );
+
+		if ( isset( $this->target_functions[ $matched_content ][ $matched_parameter ] ) ) {
+			$this->throw_prohibited_error( $stackPtr, array( $matched_parameter ) );


It may be better to show the error at the parameter instead of the function. This is not a blocker.

A case can be made for either. Is the deregistering the most important bit or is the fact that its a core script being deregistered that's important ?

For me the important part of this is that it's deregistering a core script. Sometimes scripts could be deregistered (such as child theme removing parent scripts) but core scripts almost always should not be removed.

Thanks for the feedback. I've added an additional commit which changes the line on which the error/warning is reported to the line containing the script handle.

grappler · 2018-06-10T18:55:07Z

WordPress/Sniffs/Theme/NoDeregisterCoreScriptSniff.php

+	 */
+	public function throw_variable_handle_warning( $stackPtr, $data = array() ) {
+		$this->phpcsFile->addWarning(
+			'Deregistering core scripts is prohibited. A variable script handle was found. Inspection of the %s() call needed.',


It may be good to output the variable in the error message so that a preliminary check could be made from the error message.

Good point. Done.

* Update link to handbook. * Make the sniff slightly less easy to bypass by doing a quick check on the `raw` value of the parameter first and doing a more rigorous check if that yielded no results. * Abstracted the throwing of the actual error out to a separate method to make this class more easily extendable. This allows for adding a separate sniff to detect calls to `wp_deregister_style()` to unregister WP Core external stylesheets which basically needs the same logic. * If a variable was detected in `$handle`, throw a warning. Similar to the error, the warning is thrown from a separate method for easier extending of the class. * Updated and added more unit tests.

As the array is only checked with `isset()`, there is no need for the value to have any meaning. This makes maintenance of the array simpler.

…arning

jrfnl · 2018-07-29T14:20:43Z

I've updated the PR based on @grappler's feedback (last commit) and squashed earlier commits which needed squashing. Just one open question left and this can be merged: #26 (comment)

dingo-d · 2018-08-14T17:49:15Z

In the meeting an interesting point was raised by @joyously:

I think it's the combination that makes it unwanted because parent theme scripts can be deregistered.
Wouldn't it be okay to deregister a parent script? But not a plugin script? Or a core script?

So we should only forbid deregistering core scripts? Because we know which script is a core script (https://developer.wordpress.org/reference/functions/wp_enqueue_script/#default-scripts-included-and-registered-by-wordpress).

justintadlock · 2018-08-14T19:22:59Z

The only core scripts I could really see deregistering are the audio/video/playlist shortcode scripts. Theme might very well want to do something custom there, and this would be directly related to presentation on the front end. Then again, they could simply dequeue in that case.

That's the only use case I've come across where it makes sense for themes to remove a core script in any way.

…ntains the script handle

jrfnl · 2018-08-14T21:02:09Z

The sniff only looks at deregistering and only for core scripts, so the last two comments are - as far as I'm concerned - already addressed.

jrfnl · 2018-08-19T15:12:16Z

Actions needed

Someone needs to review to sniff and if so inclined, approve & merge it.

Action for later

Review and update the script list. @dingo-d has a good list available and will take on this action.

dingo-d · 2018-08-23T06:46:59Z

Ok, since nobody gave the advice about the core scripts, I'll assume that the list is ok. Also I ran the unit test with the wp_deregister_script( 'j' . 'query' ); example and it threw an error, so this seems to be covered/fixed.

Not really sure if something else needs to be done on it, so I'll approve it.

grappler · 2018-09-01T15:24:15Z

@dingo-d will update the list of core scripts slugs.

Pross mentioned this pull request Jul 12, 2016

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

Closed

3 tasks

jrfnl reviewed Jul 12, 2016
View reviewed changes

jrfnl reviewed Jul 14, 2016
View reviewed changes

jrfnl changed the title ~~FunctionRestrictions~~ Add rule to prevent deregistering scripts. Jul 14, 2016

grappler force-pushed the feature/theme-review-sniffs branch from b8a5e09 to 5dd4758 Compare July 14, 2016 12:51

Pross closed this Jul 14, 2016

Pross reopened this Jul 14, 2016

grappler reviewed Jul 14, 2016
View reviewed changes

jrfnl reviewed Jul 15, 2016
View reviewed changes

grappler and others added 3 commits June 10, 2018 05:56

NoDeregisterCoreScript: Update inline docs & improve code from review

ec7d036

NoDeregisterCoreScript: Add all core scripts and extend AbstractFunct…

a635bfc

…ionParam sniff

NoDeregisterCoreScript: Update for PHPCS 3.x compatibility

fb7baf4

jrfnl added pr: needs review and removed pr: needs update labels Jun 10, 2018

grappler self-requested a review June 10, 2018 05:21

grappler mentioned this pull request Jun 10, 2018

User capabilities should be used not roles nor deprecated capabilities WordPress/WordPress-Coding-Standards#1364

Closed

grappler reviewed Jun 10, 2018

View reviewed changes

This was referenced Jul 28, 2018

De-fork from WPCS #169

Open

Meeting Notes 2018-07-28 #172

Open

jrfnl added 3 commits July 29, 2018 16:08

NoDeregisterCoreScript: Simplified the list of core scripts

a25f4cd

As the array is only checked with `isset()`, there is no need for the value to have any meaning. This makes maintenance of the array simpler.

NoDeregisterCoreScript: Show parameter with the "visual inspection" w…

ee6a413

…arning

NoDeregisterCoreScript: Report the error/warning on the line which co…

d6a9257

…ntains the script handle

dingo-d approved these changes Aug 23, 2018

View reviewed changes

grappler merged commit 68a5d93 into WPTT:feature/theme-review-sniffs Sep 1, 2018

jrfnl removed the pr: needs review label Oct 14, 2018

jrfnl added this to the 0.1.0 milestone Jun 12, 2019


		$content = trim( $token['content'], '"\'' );

		$functions = array(

Add rule to prevent deregistering scripts. #26

Add rule to prevent deregistering scripts. #26

Conversation

Pross commented Jul 12, 2016

jrfnl commented Jul 12, 2016

Pross commented Jul 12, 2016

jrfnl commented Jul 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl commented Jul 12, 2016

Pross commented Jul 13, 2016

Choose a reason for hiding this comment

jrfnl commented Jul 14, 2016

Pross commented Jul 14, 2016

grappler commented Jul 14, 2016

Pross commented Jul 14, 2016

Choose a reason for hiding this comment

grappler commented Jul 14, 2016

Choose a reason for hiding this comment

jrfnl commented Jun 10, 2018

Update notes:

To do at a later stage:

jrfnl commented Jun 10, 2018

jrfnl commented Jun 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grappler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl commented Jul 29, 2018

dingo-d commented Aug 14, 2018

justintadlock commented Aug 14, 2018

jrfnl commented Aug 14, 2018

jrfnl commented Aug 19, 2018

Actions needed

Action for later

dingo-d commented Aug 23, 2018

grappler commented Sep 1, 2018

jrfnl commented Jul 12, 2016 •

edited

Loading