Proposal: new sniff categorization

[jrfnl]

As this repo was (is) a fork from WPCS, when this project started, it was decided to put all Theme specific sniffs in a Theme sniff category.

However, things change. And one of the things which is about to change is that the repo will be de-forked from WPCS.

This means that it can, from that point onwards, create its own logical categories.

In this issue, I'd like to propose the following initial categorization for the sniffs currently in the Theme category:

HTML

Sniffs which examine the HTML used in a theme.

• ForbiddenIframe
• NoTitleTag - alternatively, this sniff could go into the WP category as it suggests using add_theme_support() instead.

Integrations

Sniffs which are related to external libraries/dependencies/services etc.

• CorrectTGMPAVersion
• NoAutoGenerate

PluginTerritory

Sniffs which check that themes do not add functionality (which should be done via plugins).

• NoAddAdminPages
• PluginTerritoryFunctions

WP

Sniff which check that native WP functionality is used instead of re-inventing the wheel

• FileInclude
• NoFavicon

More categories

... can be added if and when needed in the future.

Impact Analysis

• All open PRs adding new sniffs which will not be merged before the de-forking will need to be adjusted, i.e. for each sniff it will need to be decided what category they belong in and the sniff and tests will need to be moved to a different (non-Theme) category.
• If any themes were already relying on the existing sniff names, they will be impacted by this. However, as this repo is an initial setup for the TRTCS repo without any releases so far, a breaking change like this is only to be expected and should be fine.
• The issue template should be adjusted to add a proposed category section.

👉 Opinions welcome. If there are no objections, the above categorization will be used when the de-forking PR is created.

[jrfnl]

Just thinking, there are three more sniffs we need to take into account and add to categories as these will be removed from WPCS in the near future, but are currently included in TRTCS:

• AdminBarRemoval => proposal: rename to NoAdminBarRemoval and place in PluginTerritory category
• SessionFunctionsUsage => proposal: PluginTerritory or WP
• SessionVariableUsage => proposal: PluginTerritory or WP

For the session related sniffs, we may need a new category, but I can't currently come up with a good name. Suggestions welcome.

[dingo-d]

I think that sessions would be good in plugin territory category.

[joyously]

I would prefer to see them organized like our Requirements document, which has these headings, not all of which are possible to be checked with sniffs.

• Accessibility
• Code
• Core Functionality and Features
• Presentation vs Functionality
• Documentation
• Language
• Licensing
• Naming
• Options and Settings
• Plugins
• Screenshot
• Privacy
• Selling, credits and links
• Stylesheets and Scripts
• Templates

I think Language and Options can be merged with Code.
Accessibility and Licensing and Documentation are likely out of scope.
Core Functionality instead of WP, and merge Templates in.
Perhaps Excess Functionality instead of Presentation vs.Functionality or PluginTerritory.
And maybe Resources instead of Stylesheets and Scripts.

That leaves a list of

• Code (includes Language and Options)
• Core Functionality (includes Templates)
• Excess Functionality
• Naming
• Plugins
• Screenshot
• Privacy
• Selling, credits and links
• Resources

[pattonwebz]

NOTES:

• Need to set the actual names here in valid php namespace format.
• All sniffs need to be set into one of the new categories including the open PRs.

[jrfnl]

Action needed (by @dingo-d and @pattonwebz )

• Take a definite decision on the category names - keep in mind that these will also be used as namespace names, so they should be "OneWordPossiblyWithCaps".
• Categorize all the existing sniffs currently in the Theme folder into the new categories.
• Categorize the three VIP sniffs which will remain from WPCS into the new categories.
• Categorize all sniffs in open PRs into the new categories and leave a comment in the PR what the category will become.
• Adjust the issue template to add Proposed Category.
• Possibly go through existing issues to add a category to these.

[joyously]

Categories, with sniffs:

• Code
  FileIncludeSniff
ForbiddenIframeSniff
NoAutoGenerateSniff
• CoreFunctionality
  NoFaviconSniff
NoTitleTagSniff
• ExcessFunctionality
  NoAddAdminPagesSniff
PluginTerritoryFunctionsSniff
• Naming
• Plugins
  CorrectTGMPAVersionSniff
• Screenshot
• Privacy
• SellingCreditsLinks
• Resources

[dingo-d]

Why isn'tPluginTerritoryFunctionsSniff in the Plugins category? The ExcessFunctionality seems unnecessary. This should all be in the plugins category imo.

We should also change this in the handbook.

It wouldn't be a bad idea to update the themes handbook a bit too.

[joyously]

Because the Plugins category is about including plugins or requiring plugins to work, whereas the ExcessFunctionality category is about doing things that themes should not be doing. They can both be considered Code, but they are two separate sections in the requirements (and very different).

[dingo-d]

So the categories, as proposed by @joyously look fine, we just need to add a few more. I propose a bit modified version:

• Code
  FileIncludeSniff
ForbiddenIframeSniff
NoAutoGenerateSniff
• CoreFunctionality
  NoFaviconSniff
NoTitleTagSniff
SessionFunctionsUsage
NoDeregisterCoreScriptSniff
• PluginTerritory
  NoAddAdminPagesSniff
PluginTerritoryFunctionsSniff
SessionVariableUsage
AdminBarRemoval
• Plugins
  CorrectTGMPAVersionSniff

To be added later on (maybe)

• Privacy - sniff looking for url shorteners like bitly, we would need a lot of examples of URL shorteners to work on this

Not needed (in my opinion)

• Naming - covered by WPCS (capital P sniff)

We need to have a concensus about this, and this should be addressed in the Team Review meeting.

[grappler]

I think using the requirements structure is a good idea. Once we have defined the categories in the ruleset we should not change them and that would apply the same for the requirements documentation.

I would prefer to use PluginTerritory instead of ExcessFunctionality.

The category code is not very clear and I can see that as a dumping ground for sniffs that don't fit anywhere. All of the sniffs run on code so it is not very clear. BestPractice would be an alternative suggestion.

[dingo-d]

Ok, so after much debate we came up with these initial categories for the existing sniffs:

• ThouShallNotUse
  ForbiddenIframeSniff
NoAutoGenerateSniff
• CoreFunctionality
  FileIncludeSniff
NoFaviconSniff
NoTitleTagSniff
SessionFunctionsUsage
NoDeregisterCoreScriptSniff
• PluginTerritory
  NoAddAdminPagesSniff
PluginTerritoryFunctionsSniff
SessionVariableUsage
AdminBarRemoval
• Plugins
  CorrectTGMPAVersionSniff

The sniffs in the PR can be moved to these categories or added in the new ones if need be.

[jrfnl]

@dingo-d Just one last question - are you sure that the Session related sniff shouldn't be in the same category ?

[jrfnl]

Oh and one more question - the PluginTerritoryFunctionsSniff file now lives in the PluginTerritory category, so there is duplication in the sniff name.
Should I change the sniff name ? And if so, to what ?
FunctionsSniff ? ForbiddenFunctionsSniff ?

[dingo-d]

@jrfnl You mean a separate category just for sessions? We could put them in ThouShallNotUse or in PluginTerritory, don't really know why I separated those tbh 😄

If it's not a problem it would be better to change the sniff name.
Maybe to UseInPluginsSniff? or PluginsFunctionsSniff, whichever you find better. I'm fine with ForbiddenFunctionsSniff also, I don't think that anyone from the team would object whichever is chosen.

[jrfnl]

You mean a separate category just for sessions?

No, I meant having both sniffs in the same category, whichever category that is. Having them in different categories feels counter-intuitive.

If it's not a problem it would be better to change the sniff name.

As what we are doing now is a complete and utter BC-break, at this moment, we have all the freedom in the world to change sniff names (and anything else) 😎

Maybe to UseInPluginsSniff? or PluginsFunctionsSniff, whichever you find better. I'm fine with ForbiddenFunctionsSniff also, I don't think that anyone from the team would object whichever is chosen.

Both names you suggest repeat the Plugins part of the category name again. If we look at other standards, both WPCS as well as PHPCS use sniff names like ForbiddenFunctions / DiscouragedFunctions / DeprecatedFunctions and may have several sniffs named the same in different categories.

[dingo-d]

OK, then put the sessions in the PluginTerritory since sessions are ok to be used in plugins.

Yeah, the plugins part is redundant, since the category is called PluginTerritory. The ForbiddenFunctionsSniff it is then 😄 (the Sniff part will be added to every sniff name, right?)

[jrfnl]

As the repo history has now been rewritten and the change is contained in the develop and master branch, I'm closing this issue.

The remaining actions from comment #166 (comment) should still be taken though:

• Categorize all sniffs in open PRs into the new categories and leave a comment in the PR what the category will become.
• Adjust the issue template to add Proposed Category => see PR Issue template: textual update #178.
• Possibly go through existing issues to add a category to these.