Add rule to prevent deregistering scripts.

Pross commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

Of course!

PM me on Slack.

jrfnl commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

Pross commented 8 years ago

Closing for rewrite.

grappler commented 8 years ago

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

Pross commented 8 years ago

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

grappler commented 8 years ago

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 commented 8 years ago

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

@Pross Still need help with how to do this ? If so, feel free to ping me on Slack later today and I'll talk you through it.

Short answer would be: no to reverting. Just reset your branch to the original point where you split off from the theme-review-sniffs branch. Create a new commit which includes your previous changes and your new changes in one and then force push it to the branch on GH. As the branch is still the same, this PR will be updated with the fresh copy and previous commits will disappear as if they never existed ;-) While you're at it, you may want to rebase the branch against the current state of the theme-review-sniff branch as well. Warning: Only do this for a branch in which you are the only one working on it, otherwise things could get ugly ;-)

jrfnl commented 8 years ago

@Pross FYI - the FunctionsRestrictionsSniff has now been turned into a proper abstract WordPress_AbstractFunctionRestrictionsSniff to avoid the earlier confusion we had about this.

See: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/pull/603

Pross commented 8 years ago

My travis build failed because of another PR

`FILE: ...Coding-Standards/WordPress/Sniffs/Theme/NoAddAdminPagesSniff.php

62 | ERROR | [x] Blank line found after control structure | | (WordPress.WhiteSpace.ControlStructureSpacing.BlankLineAfterEnd)`

jrfnl commented 8 years ago

My travis build failed because of another PR

@Pross I've solved this in the branch, if you rebase, you won't get the error anymmore.

Pross commented 8 years ago

Rebased, All good.

jrfnl commented 8 years ago

@Pross Looks like something went wrong with the rebase - this branch now contains commits which are already in the main theme-review-sniffs branch. Could you please check this ? Let me know if you need help with straightening this out.

Pross commented 8 years ago

All I did was click rebase like you said, so it updated my branch with your branch.

Was I supposed to do any different?

jrfnl commented 8 years ago

No... but the effect should have been different. Hang on... I'll check out your fork and have a look what's going on.

jrfnl commented 8 years ago

@Pross Something really weird is going on there. It did not rebase nor merge. The split off is still at the old point (14/7), but the commits which were added to the main branch have been added on top - totally not how a rebase should work. Want me to help you sort it out ? If so, ping me on Slack or Skype, we'll take this off GH.

jrfnl commented 8 years ago

Hi @Pross Ok, so a lot has been happening in WPCS over the last few months and we're about ready to do a merge back into TRCS. We'd like to merge this PR before that, so I've had another look and have a couple more questions if you don't mind. Will add them inline.

grappler commented 8 years ago

@Pross Could you allow us maintainers to make changes to your PR. https://github.com/blog/2247-improving-collaboration-with-forks

I could then work on your PR to get it merged instead of needing to open a new PR.

jrfnl commented 6 years ago

@Pross Simon, just wondering if you are still interested in finishing this sniff. If not, are you ok with me taking over & bringing it to the finish-line ?

jrfnl commented 6 years ago

Notes on what is needed for update:

Update for PHPCS 3.x
Update based on last review

Pross commented 6 years ago

If not, are you ok with me taking over & bringing it to the finish-line

Be my guest.

jrfnl commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.

jrfnl commented 6 years ago

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: https://github.com/WPTRT/WordPress-Coding-Standards/pull/26#discussion_r205975121

dingo-d commented 6 years ago

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 commented 6 years ago

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.

jrfnl commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

WPTT / WPThemeReview