Leveraging the PHPCompatibility_Sniff

[jrfnl]

Something I've been wondering about for a while now and just want to put it out there to see if there was a specific reason for (not doing) it (yet) and/or to get a second opinion.

• Currently every class has a process() method which receives the $phpcsFile and the $stackPtr.
• Nearly every process() method then proceeds to call $phpcsFile->getTokens() and often also assigns the current token to a variable.
• Whenever secondary methods are called - either in the sniff class or in the parent PHPCompatibility_Sniff class -, the $phpcsFile and $stackPtr are passed on and $tokens is retrieved again.

What about:

• adding a process() method to the PHPCompatibility_Sniff parent class
• adding an abstract processToken() method to the parent class
• changing the existing process() methods in the child classes to processToken()
• the process() method in the parent class would assign $phpcsFile, $stackPtr, $tokens and $currentToken to protected properties in the same class which can then be accessed by all child classes and methods and would subsequently call the processToken() method

This way we would leverage the fact that they are parent-child classes, won't have to pass the parameters on all the time and save on some function calls.

Opinions ?

[MarkMaldaba]

What is recommended best-practice for this? What do the built-in PHPCS sniffs do?

I am always wary about code de-duplication that ends up obfuscating behaviour or adding an extra cognitive burden (i.e. doing something unexpected). I don't know if that is the case here, but that would be a reason not to do it if it is.

How many sniffs do we had where this behaviour wouldn't be wanted (i.e. which would need to implement process() instead of processToken())?

[MarkMaldaba]

We also need to consider the impact on unit tests - if these are class properties rather than function arguments then testing the sub-processes becomes harder.

Obviously, not an issue if these functions don't need to be called directly during testing, but a consideration.

[wimg]

The testing part shouldn't be impacted since in fact our tests are more integration tests than unit tests (we don't test the code directly but the result of the code processing through PHPCS). It's not following the standard way of working in PHPCS. That's not necessarily a bad thing, but I wonder if the gains would be big enough to compensate for the work that it requires ?

[jrfnl]

@MarkMaldaba Thanks for your feedback.

What is recommended best-practice for this?

There is no recommended best practice AFAIK, though I've recently implemented the same in the WordPress Coding Standards where the same issue came into play.

What do the built-in PHPCS sniffs do?

Most build-in sniffs have an extremely long process() method which definitely does not comply with modern best practices of using smaller re-usable blocks ;-)

For the utility methods within PHPCS itself, those are contained within the $phpcsFile / PHPCodeSniffer_File class which has $tokens etc as class properties and thereby does not require the passing of the $phpcsFile parameter or extra function call to $tokens.

How many sniffs do we had where this behaviour wouldn't be wanted (i.e. which would need to implement process() instead of processToken())?

AFAICS none, though there are a few (two or three) which don't necessarily need it, but wouldn't suffer for the change.

We also need to consider the impact on unit tests - if these are class properties rather than function arguments then testing the sub-processes becomes harder.

Obviously, not an issue if these functions don't need to be called directly during testing, but a consideration.

None of the sniffs currently test subprocesses, all test the complete sniff in the current unit test suite.

For most utility functions which I've added over the last few months, we do have separate unit tests and, while the change would mean that the logic in the MethodTestFrame used by those tests would need to change a little, it would not prevent us testing these methods.

[MarkMaldaba]

@jrfnl Given those responses, I don't personally have any objections to refactoring in this way. My only concern is whether it makes it harder for new users to contribute, particularly users already familiar with PHPCS, but I trust you and @wimg to make that call.

I undestand @wimg's concern about whether the gain is worth the effort expended to make the changes, but assuming we're only talking about the one-off cost of updating the existing codebase, that's only really a concern for the person doing the work.

[jrfnl]

My only concern is whether it makes it harder for new users to contribute, particularly users already familiar with PHPCS

We could add some additional information to the CONTRIBUTING.md file to help new contributors on their way.

assuming we're only talking about the one-off cost of updating the existing codebase, that's only really a concern for the person doing the work.

Making the change in the WPCS codebase did not take all that long, couple of hours, so considering the amount of time I've been spending on this library anyway, I'm not concerned about that.

The only real issue would be finding a moment where there is no backlog of PRs as the change would affect all sniffs and would be easiest if pulled in one go.