Extract re-usable code from Sniff.php into new package

[GaryJones]

Sniff.php contains a lot of helper methods: re-usable bits of code that could be used in one or more sniffs, e.g.:

• is_foreach_as() (part of GlobalVariablesOverride: detect variable overrides in global namespace #1457)
• is_token_in_test_method()
• is_test_class()
• is_assignment()
• is_in_isset_or_empty()
• get_array_access_key()
• is_comparison()
• etc.

All of the methods are non-WPCS / non-WP specific (or could be made to be). It would be even nicer if some of these could be moved into PHPCS itself (cc @gsherwood), but if not, then having a separate package, perhaps including the WordPress-specific methods as well, would allow the helper methods to be re-used by other WP-specific coding standards (Neutron, VIP CS, etc.)

Sniff.php is very long, and while that's not a bad thing on its own, it does make maintenance trickier, and re-using all of that non-WPCS goodness harder too.

[jrfnl]

@GaryJones I'd been thinking about this myself and my take on this is slightly different.

For a number of "groups" of methods which are in the Sniff class, I have been thinking of moving them to Traits once we drop PHPCS 2.x support.

• Traits were introduced in PHP 5.4, so once PHPCS 3.x is the minimum we can start using them.
• Traits would allow for smaller groupings of related methods in contrast to having everything in the Sniff class, making maintenance simpler.
• Traits would allow these methods to be used, even when sniffs do not extend the base Sniff class (if the traits are set up properly that is).

I'd need to look into how this would work with PHPCS autoloading, but with the <autoload> directive which was added in one of the early PHPCS 3.x versions, if PHPCS autoloading would not handle this correctly, this will be solvable anyway.

I don't think the other standards is a consideration as such. They can just declare WPCS as a dependency - which most already do anyway as they use WPCS sniffs - and either, at this moment, use the WPCS Sniff base class, or if we'd move things around to traits, just use the traits.

At this moment, there are only very few methods in the WPCS Sniff class which I think are good enough, code style independent enough + generic enough to be considered for pulling to PHPCS.

The group of "function call parameter" methods definitely come to mind. I actually wrote those with PHPCS in mind and have - not to put too fine a point to it - basically been testing them in WPCS and PHPCompatibility before pulling them to PHPCS.

However, the typical place to pull these in PHPCS would be to the File class, which is just as large as the WPCS Sniff class, so basically we'd be moving the maintenance problem around, not solving it.

Another thing to take into consideration before pulling anything to PHPCS, is copyright.
GPL does not mean copyright does not apply. So, if I remember correctly, that means that, for each and every method to be considered for this, we'd need to trace through the file history to find out exactly who have touched the method during its lifetime so far and get explicit permission of each person before any action like you are suggesting can be taken.

This is basically the reason why for sniffs which I've written for WPCS, but which were generically applicable, I've often pulled them to both WPCS as well as PHPCS around the same time as at that moment, I was the only one who had touched the code, and as I pulled the sniff to both, there is no copyright issue.

[GaryJones]

I have been thinking of moving them to Traits

That would work for me.

But equally, that doesn't preclude those traits from being in their own package.

I don't think the other standards is a consideration as such. They can just declare WPCS as a dependency - which most already do anyway as they use WPCS sniffs - and either, at this moment, use the WPCS Sniff base class, or if we'd move things around to traits, just use the traits.

It doesn't have to be a consideration, but in open source spirit, if there's some code that is in WPCS that could be of use to someone else, it benefits all the users to make it available and have that consistent comprehensive functionality handled in one place.

Neutron doesn't have WPCS as a dependency at all.

VIPCS does, but only because it builds upon the WPCS-specific logic, as opposed to the general PHPCS helper methods found in WPCS. The former may change, but it may still want to use the latter without needing to require the whole of WPCS.

However, the typical place to pull these in PHPCS would be to the File class, which is just as large as the WPCS Sniff class, so basically we'd be moving the maintenance problem around, not solving it.

Could they be pushed into PHPCS as traits?

So, if I remember correctly, that means that, for each and every method to be considered for this, we'd need to trace through the file history to find out exactly who have touched the method during its lifetime so far and get explicit permission of each person before any action like you are suggesting can be taken.

IANAL, but so long as attribution is maintained, we should be good.

[jrfnl]

Could they be pushed into PHPCS as traits?

That's not a question to ask or discuss here, but in the PHPCS repo.

But equally, that doesn't preclude those traits from being in their own package.

But copyright does.

IANAL, but so long as attribution is maintained, we should be good.

And attribution is never maintained, nor the history, if the code is moved out of this repo, so see my previous reply.

[jrfnl]

It doesn't have to be a consideration, but in open source spirit, if there's some code that is in WPCS that could be of use to someone else, it benefits all the users to make it available and have that consistent comprehensive functionality handled in one place.

Oh and just to be clear: that's what making WPCS a dependency does.
WPCS is available as a package and people can use the code within it by making WPCS a dependency for their project.

If we reorganize code to make that easier, all the better, but it's not an argument for a separate package.

[dingo-d]

Could they be, and would it be a good idea at all, to have a helper class where these helper methods are static? Something like SniffHelper class.

They would probably all have to be rewritten, since most (if not all) have $this->tokens and $this->phpcsFile, so they should be passed as a parameter of the static method.

[jrfnl]

FYI and to follow up on my previous comment where I said I'd been thinking about this anyway:

I've opened an issue about a refactor for PHPCS - squizlabs/PHP_CodeSniffer#2189

• With this refactor the code of most existing utility methods within PHPCS will be moved out of the File class and into static methods in separate Utility classes.
  The old methods will remain available until PHPCS 4, but will be deprecated once the refactor is done.
• The refactor opens the door to introducing new utility methods as mentioned above. In the issue I've indicated some which I have in mind for that.
• Greg has given me the green light for the refactor, though it will still be a while before the code is in PHPCS.

With all this in mind, I think we should hold off on the move to traits until it has crystallized what will go in to PHPCS and how, to prevent doing a lot of unnecessary (double) work.

After we've upgraded to the PHPCS version containing the refactor, we can evaluate what's left of the WPCS utility methods and whether moving some of these to traits and/or static classes would still make sense and how that should be organized.

Makes sense ?