-
-
Notifications
You must be signed in to change notification settings - Fork 483
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
Issue 1157/Proposal 2: rename sniffs #1242
Conversation
The original sniff class still exists, but extends the renamed sniff now. In addition, a deprecation warning will be thrown if this sniff is included directly by a custom ruleset. A deprecation warning will also be thrown if the sniff is included directly just to set custom properties. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.
The messages thrown by the sniff have been downgraded to warnings and the message text adjusted to make the sniff more generically usable. For the VIP ruleset, this change is undone via custom ruleset properties.
The original sniff class still exists, but extends the renamed sniff now. In addition, a deprecation warning will be thrown if this sniff is included directly by a custom ruleset. A deprecation warning will also be thrown if the sniff is included directly just to set custom properties. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.
The original sniff class still exists, but extends the renamed sniff now. In addition, a deprecation warning will be thrown if this sniff is included directly by a custom ruleset. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.
The original sniff class still exists, but extends the renamed sniff now. In addition, a deprecation warning will be thrown if this sniff is included directly by a custom ruleset. A deprecation warning will also be thrown if the sniff is included directly just to set custom properties. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.
The original sniff class still exists, but extends the renamed sniff now. In addition, a deprecation warning will be thrown if this sniff is included directly by a custom ruleset. A deprecation warning will also be thrown if the sniff is included directly just to set custom properties. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.
The original sniff class still exists, but extends the renamed sniff now. In addition, a deprecation warning will be thrown if this sniff is included directly by a custom ruleset. A deprecation warning will also be thrown if the sniff is included directly just to set custom properties. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.
The original sniff class still exists, but extends the renamed sniff now. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.
The message thrown by the sniff has been downgraded to a warning and the message text adjusted to make the sniff more generically usable. For the VIP ruleset, this change is undone via a custom ruleset property.
…egory The original sniff class still exists, but extends the renamed sniff now. In addition, a deprecation warning will be thrown if this sniff is included directly by a custom ruleset. A deprecation warning will also be thrown if the sniff is included directly just to set custom properties. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.
The original sniff class still exists, but extends the renamed sniff now. In addition, a deprecation warning will be thrown if this sniff is included directly by a custom ruleset. A deprecation warning will also be thrown if the sniff is included directly just to set custom properties. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.
The message thrown by the sniff has been downgraded to a warning and the message text adjusted to make the sniff more generically usable. For the VIP ruleset, this change is undone via a custom ruleset property.
The original sniff class still exists, but extends the renamed sniff now. In addition, a deprecation warning will be thrown if this sniff is included directly by a custom ruleset. A deprecation warning will also be thrown if the sniff is included directly just to set custom properties. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.
Reminder: Check wiki for old references too. |
This looks good to me, but I wonder if we should do some additional renames while we are at it. For example, I'm unclear on the difference between Going down through the list of our different sniff groupings:
So basically, I realize that this initial proposal was mainly intended to address ease of including/excluding DB and Security sniffs in particular, and preparing VIP for deprecation. So maybe any other changes should be dealt with separately? |
@JDGrimes As a general rule for both the proposal as well as most sniffs where I've added new categories, I've looked towards PHPCS upstream to see where similar sniffs upstream were placed and tried to use categories which were consistent with upstream.
Upstream We mainly use upstream sniffs for these things at the moment, except for the
To me, the difference between the
With that in mind, Side note: both of these sniffs would be good candidates to pull upstream in the foreseeable future. The only "odd one out" I see in the PHP category would be
AFAICS, the difference between
With that in mind, the WP sniffs follow the exact same logic.
Upstream, this category is used similar to There are a couple of open issues - #1138 comes to mind - related to function call signatures, for which I'm considering writing a WPCS specific sniff which would then be placed in this category. Basically,
See my comments about this above.
If I remember correctly, we do in some of the whitespace and file related sniffs.
Upstream there is no |
@jrfnl All of that makes a lot of sense.
I'd be in favor of this as well. |
…d rename The sniff has been renamed from `GlobalVariables` to `GlobalVariablesOverride` to make it easier to understand at one glance what the sniff is about. The original sniff class still exists, but extends the renamed sniff now. In addition, a deprecation warning will be thrown if this sniff is included directly by a custom ruleset. A deprecation warning will also be thrown if the sniff is included directly just to set custom properties. Note: if the sniff was explicitly **excluded** via a custom ruleset, no deprecation warning will be thrown and the exclusion will have no effect. The custom ruleset will need to be updated to use the new sniff name for the exclusion to be reinstated.
I've added an additional commit to handle the rename of the Using |
@jrfnl Good to merge from me. |
As this has now been merged, I'd like to change the wiki as well. I will annotate that the change in sniff names happened in version |
I'm getting the following when using the
The second one seems sensible (though I'm not sure why it's appearing since my |
@GaryJones Good catch! The message text should be fixed via #1263. As for why the messages are showing... not sure... Could you point me to the ruleset so I can run some tests ? |
@jrfnl My PR has just been merged, so see https://github.com/NicktheGeek/genesis-featured-widget-amplified/blob/hotfix/0.9.1/.phpcs.xml.dist With the corrected messages: |
@jrfnl So, it seems to be this:
A quick solution would be to add tl;dr: A vs B check is fine in itself, but B hasn't been initialised at the time the first check is done. |
@GaryJones I'm been looking into this as well. The defaults for the
That would also prevent the deprecation warnings from being thrown when the custom properties have been set using the old sniff names, which is exactly the situation for which we want to throw the messages, so that would not work. |
I also spotted that - and that solution would work for me. |
FYI: I've updated the wiki pages to use the new sniff names. |
While there currently isn't consensus yet about proposal 1 of issue #1157, proposal 2 seems widely supported.
To that end, I have prepared the necessary changes.
This is PR 1 in a series of three PRs to address this and by far the largest one.
This PR basically takes care of all the sniff renames, sniff deprecations and
error
towarning
downgrades as proposed, with the exception of the splitting of thePostsPerPage
sniff which I will address in a separate PR.I've elected to do all the renames in one go as they all require changes to the
ruleset.xml
file for the top-levelWordPress
ruleset and would otherwise all conflict or have to be pulled one by one, each waiting on the previous one.All changes have been made in separate commits though for easier review and better traceability in the git history.