Php_codesniffer: Refactor suggestion / move utility functions from File to static classes

[jrfnl]

Since PHPCS 3.x now has a minimum PHP requirement of PHP 5.4, I was wondering whether it might be an idea to refactor a number of groups of methods in the File class to traits.

The reasoning behind this is two-fold:

1. Only a limited number of sniffs use the methods I have in mind for this, so having them in traits rather than in the File class makes the re-useable code more modular and easier to maintain.
2. Over the years I have written a number of additional token walking utility methods which I think belong in PHPCS rather than in the WPCS/PHPCompatibility standards in which they now live.
   Traits would allow for these methods to be added to PHPCS more easily without making the File class even larger than it already is.

Timing

The transition could be made in PHPCS 3.x.
In PHPCS 3.x, the File class would in that case use the new traits and for the transitioned methods just call the method in the trait under the hood.
The methods in the File class would be deprecated and could be removed in PHPCS 4.x.
This would allow external standards some time to implement the new way of doing things and make the BC break in PHPCS 4.x smaller as it can already be mitigated beforehand.

Proposal details

To be concrete, I'm currently thinking of the following Traits:

• DeclarationName, containing:
  
  
  
  • getName(), previously called getDeclarationName()
  
  
• FunctionDeclaration, containing:
  
  
  
  • getParameters(), previously called getMethodParameters()
  
  
  • getProperties(), previously called getMethodProperties()
  
  
  • and using DeclarationName for the getName() method
  
  
• PropertyDeclaration, containing:
  
  
  
  • getMemberProperties()
  
  
• ObjectDeclaration containing:
  
  
  
  • getClassProperties()
  
  
  • findExtendedClassName()
  
  
  • findImplementedInterfaceName()
  
  
  • findExtendedInterfaceNames() - open PR #2128
  
  
  • and using DeclarationName for the getName() method
  
  
• Generic containing:
  
  
  
  • isReference()
  
  
• ScopeConditions, containing:
  
  
  
  • hasCondition()
  
  
  • getCondition()
  
  

Additionally, I'd be happy to pull the following based on battle-tested code I've written for WPCS/PHPCompatibility:

• FunctionCall, containing:
  
  
  
  • hasParameters()
  
  
  • getParameterCount()
  
  
  • getParameters()
  
  
  • getParameter() (to get the details for the parameter at a specific position)
    
    These methods all work on both function calls as well as for splitting an array into the individual items and could be extended to cover isset(), unset() and list() as well.
  
  
• And if there would be any interest in it - FQClassNames, containing:
  
  
  
  • getNameFromNewToken()
  
  
  • getNameFromDoubleColonToken()
  
  
  • getExtendedName()
  
  
  • getFQName()
  
  
  • isNamespaced()
  
  
  • determineNamespace()
  
  
  • getDeclaredNamespaceName()
  
  

For the Generic trait, I could also offer an additional isShortList() method.

I'm also still working on the logic for a TStringAnalyser which should reliably determine whether, for instance, a T_STRING token represents a function call, the use of a global constant or for instance a use statement alias.

Open questions

What I'd currently like to know, is:

• Does the principle of this sound like a good idea for PHPCS ?
• If so, does the implementation proposal for PHPCS 3.x/4.x sound like a good idea ?
• If so, where should the traits live ? I.e. , should they be placed in a dedicated Traits directory or, for instance, in Util/Traits or should the directory be called something completely different ?
• If so, feedback on the proposed initial Traits as outline above (trait names, function groupings etc)
• And would there be interest in the additional traits/methods as outlined above ?

[gsherwood]

• Does the principle of this sound like a good idea for PHPCS ?

Yeah, I think I like the idea. The File class has always been bloated but I've never put thought into refactoring it due to the large BC break it would cause.

• If so, does the implementation proposal for PHPCS 3.x/4.x sound like a good idea ?

My initial thought was v4 only, but your point about developers being able to migrate sniffs before v4 is a good one, so I think you are right. This feels like something that can go into v3 with the transitional methods and in File deprecated immediately and removed in v4.

• If so, where should the traits live ? I.e. , should they be placed in a dedicated Traits directory or, for instance, in Util/Traits or should the directory be called something completely different ?

I would normally throw them under a src/Traits folder. I do prefer trait class to include the word "Trait" at the end of the file and trait name, but having Traits in the FQ name makes this redundant. So I don't know. Maybe they should live in src/Files or src/Sniffs somewhere?

• If so, feedback on the proposed initial Traits as outline above (trait names, function groupings etc)

They look ok to me. I don't have a strong opinion on this yet, but I imagine I will once I see some code being used.

• And would there be interest in the additional traits/methods as outlined above ?
  Yep.

This one's a big job and something I'd probably want to solely focus on for a single PHPCS release as it is also going to be a big information dump for sniff developers. So maybe this is the change in v3.5 or v3.6.

[jrfnl]

@gsherwood Thanks for your response and feedback. I'd happily work on this, but as you say you'd prefer to have this in a dedicated release, I'll hold off for now until 3.4.0 has come out. That would also allow for PR #2128 to be merged before this refactor.

[jrfnl]

I've been thinking about this some more and while traits would be a good solution for some of the external standards I work on, which have a base sniff class from which all sniffs extend, for PHPCS itself, the implementation will be simpler and cleaner as classes with static properties and static methods.

This will prevent naming conflicts over imported traits when both an abstract sniff as well as a child sniff would use the same trait, or when one trait would use another.

With that mind, putting these static classes in the Util or in a Helper directory seems logical.

---

For anyone wondering about the difference/choice for static classes:
Nearly all of these methods will need to be passed the $phpcsFile object and will need the tokens array.

In some of the external standards, a base sniff class is used which makes those available via $this to save having to pass these around on every function call to utility functions, like so:

abstract class Sniff implements PHPCS_Sniff {
    public $phpcsFile;
    public $tokens;

    public function process(File $phpcsFile, $stackPtr) {
        $this->phpcsFile = $phpcsFile;
        $this->tokens    = $phpcsFile->getTokens();
        return processToken($stackPtr);
    }

    abstract public function processToken($stackPtr);
}

However, as this is not standard in PHPCS itself, traits would not have access to $phpcsFile or $tokens via $this, which removes the reason for using traits instead of static classes.

[jrfnl]

Just FYI: I've started work on this as, as soon as I started work on the AbstractDocblockSniff - #2222 -, I realized some things would be easier if some of the new utility functions would already be available.

I hope to slowly but surely get a complete WIP branch ready by the time 3.4.0/3.4.1 is released, so I can start pulling the individual bits from it as soon as 3.5.0 opens for commits.

Once I've cleaned up what I've done so far, I'll put a WIP branch online somewhere so anyone interested can have a look at the direction this is going in.

[jrfnl]

Little status update for those interested:

I'm working on this on and off, but making steady progress. I estimate that about 70% is done at this moment.

Notes:

• All existing, as well as all new, utility methods mentioned below have dedicated unit tests based on PR #2295.
• I've adjusted all existing calls to the "old" methods in the current code base.
  Depending on what gets merged before this refactor gets merged, some additional adjustments may still be needed, but I'm keeping a close eye on that.
• All "old" methods (and the below mentioned properties) are deprecated, but will still work, calling the new methods in the utility classes under the hood.
• All properties and methods mentioned are public static and can be called from anywhere (unless otherwise indicated).

Current structure as things are shaping up:

• Class Util\ConditionUtils
  
  
  
  • getCondition() (moved from File) - this method has been enhanced a lot to be more flexible and remove code duplication.
  
  
  • hasCondition() (moved from File)
  
  
  • New: _getFirstCondition()_
  
  
  • New: _getLastCondition()_
  
  
  • New: _isOOProperty()_
  
  
  • New: _isOOConstant()_
  
  
  • New: _isOOMethod()_
  
  
  • New: _validDirectScope()_
  
  
• Class Util\FunctionDeclarationUtils
  
  
  
  • $magicMethods (moved from Generic.NamingConventions.CamelCapsFunctionName + PEAR.NamingConventions.ValidFunctionName)
  
  
  • $methodsDoubleUnderscore (moved from Generic.NamingConventions.CamelCapsFunctionName)
  
  
  • $magicFunctions (moved from Generic.NamingConventions.CamelCapsFunctionName + PEAR.NamingConventions.ValidFunctionName)
  
  
  • getMethodParameters() (moved from File)
  
  
  • getMethodProperties() (moved from File)
  
  
  • New _isMagicFunction()_
  
  
  • New _isMagicFunctionName()_
  
  
  • New _isMagicMethod()_
  
  
  • New _isMagicMethodName()_
  
  
  • New _isPHPDoubleUnderscoreMethod()_
  
  
  • New _isPHPDoubleUnderscoreMethodName()_
  
  
  • New _isSpecialMethod()_
  
  
  • New _isSpecialMethodName()_
  
  
• Class Util\ObjectDeclarationUtils
  
  
  
  • getClassProperties() (moved from File)
  
  
  • findExtendedClassName() (moved from File and adjusted based on #2128)
  
  
  • New _findExtendedInterfaceNames()_ - see PR #2128
  
  
  • findImplementedInterfaceNames() (moved from File and adjusted based on #2128)
  
  
  • New (private) _examineObjectDeclarationSignature()_ - see PR #2128
  
  
• Class Util\PassedParametersUtils
  
  
  
  • New _hasParameters()_
  
  
  • New _getParameters()_
  
  
  • New _getParameter()_
  
  
  • New _getParameterCount()_
  
  

Currently working on:

• Class Util\VariableUtils
  
  
  
  • $phpReservedVars (moved from AbstractVariableSniff + enhanced and made complete)
  
  
  • getMemberProperties() (moved from File)
  
  
  • New _isPHPReservedVarName()_
  
  
  • New _isSuperglobal()_
  
  
  • New _isSuperglobalName()_
  
  
  • New _isForeachAs()_
  
  
  • WIP: New _isAssignment()_
  
  
  • .... (to be determined)
  
  
• Class Util\NameUtils
  
  
  
  • getDeclarationName() (moved from File)
  
  
  • isCamelCaps() (moved from Util\Common)
  
  
  • WIP: New _toCamelCaps()_
  
  
  • WIP: New _isPascalCase()_
  
  
  • WIP: New _toPascalCase()_
  
  
  • WIP: New _isUpperCaseUnderscores()_
  
  
  • WIP: New _toUpperCaseUnderscores()_
  
  
  • WIP: New _isSnakeCase()_
  
  
  • WIP: New _toSnakeCase()_
  
  
  • _Suggestions for more/other common conversions welcome._ Please include a link to the specs with your suggestion.
  
  
• Class Util\TextStringUtils
  
  
  
  • REGEX_COMPLEX_VARS
  
  
  • New _stripQuotes()_
  
  
  • New _stripInterpolatedVariables()_
  
  
  • New _getInterpolatedVariables()_
  
  
  • .... (to be determined)
  
  
• Class Util\... (name to be determined)
  
  
  
  • isReference() (moved from File)
  
  
  • New _isShortList()_
  
  
  • New _getUseType()_
  
  
  • New _getAsType()_
  
  
  • .... (to be determined)
  
  

On the back-burner for later in combination with the AbstractDocblockSniff (see #2222):

• Class Util\CommentUtils
  
  
  
  • $allowedTypes (moved from Common)
  
  
  • suggestType() (moved from Common)
  
  
  • New _findCommentAboveFunction()_
  
  
  • New _findCommentAboveOOStructure()_
  
  
  • New _findCommentAboveOOProperty()_
  
  
  • New _findCommentAboveOOConstant()_
  
  
  • New _findCommentAbove()_
  
  
  • .... (to be determined)
  
  

Feedback welcome.

[gsherwood]

First thought... wow :)

Second is just about naming. I don't think every class under Util also needs to end with Utils. Seems redundant to me in this case. Is there a specific reason (code looks better, it's a convention im not aware of, etc)?

I'd also change FunctionsDeclarationUtils to FunctionDeclaration to match ObjectDeclaration (unless that was a typo).

[jrfnl]

First thought... wow :)

:grinning:

Second is just about naming. I don't think every class under Util also needs to end with Utils. Seems redundant to me in this case. Is there a specific reason (code looks better, it's a convention im not aware of, etc)?

Not existing convention or anything, just felt like a way to differentiate these classes from the other/existing classes in the Util directory which don't typically offer methods for use in sniffs (other than the Common class, but the utility methods in that class will be moved out to one of the new classes anyway).

Alternatively, another directory for these type of classes would also be an option. Just what would that directory be called in that case ?

I'd also change FunctionsDeclarationUtils to FunctionDeclaration to match ObjectDeclaration (unless that was a typo).

That was a typo ;-)

[gsherwood]

Alternatively, another directory for these type of classes would also be an option. Just what would that directory be called in that case ?

In my head, these were under File/Util, but I think I just read your comments incorrectly.

Maybe Util/Sniffs ?? I hate naming things.

[jrfnl]

In my head, these were under File/Util, but I think I just read your comments incorrectly.

To prevent further confusion, they are currently in the src/Util directory.

Maybe Util/Sniffs ?? I hate naming things.

I hear you. Util/Sniffs doesn't feel right either though, though I haven't got a good alternative.
I'll sleep on it and welcome more suggestions. Someone will have a good name in mind ;-)

[jrfnl]

Ok, let's figure this name thing out as I will have to redo every single commit with the new names, so I can't really publish a WIP branch until we have a decision on this.

Some suggestions of where to place these classes (in alphabetic order, not indicating any preference):

• src/Helpers
• src/SniffHelpers
• src/SniffUtils
• src/Utils/SniffHelpers

The previously suggested src/Sniff/Utils doesn't really feel right as it would lump these classes together with the abstracts, while they are not abstracts and are useful for a lot more sniffs than just the abstracts.

The previously suggested src/File/Utils doesn't really feel right either as these classes don't provide a File object with tokens, like the classes in File.

My preference would be a toplevel directory as hiding these away in a deeper level makes them less easily discoverable and they are kind of useful ;-)

Watchers of this repo, please feel free to chime in with ideas or preferences as well.

[jrfnl]

@gsherwood In practical terms, how would you like this to be pulled ?

PR #2295 is a prerequisite for this PR so would need to be merged first and I've worked on the presumption that PR #2353 will be merged beforehand as well.

The way I've set things up is with atomic commits with most of the time quite detailed commit messages telling "the story" (see screenshot).

Screenshot of the first part of the WIP branch commits

---

Some parts could be pulled simultaneously, but pulling things in one go or in blocks in the order I've set them up will probably be easiest as for implementation commits, the use statements in sniffs would conflict if not pulled in this order and for commits adding files (the helper classes and/or unit tests), the package.xml file would quickly start to conflict.

[GaryJones]

src/Helpers is too generic.

Of those choices, src/SniffUtils seems the most descriptive and accurate. These are utility classes / methods / _things_ to help write Sniffs efficiently.

[jrfnl]

For the naming convention methods, I'm still trying to find unambiguous specifications. I've created a little comparison table of my current understanding of them.

Input welcome! You can comment in the comparison table.
https://docs.google.com/spreadsheets/d/1WQo7G3BRIk0dT9eMxcPJw48DShrlenQRfvlD--rFAgA/edit?usp=sharing

[jrfnl]

@gsherwood Just checking in: have you had a chance to think about the naming and how to pull this ?

[gsherwood]

@jrfnl Sorry, no. I'm swamped at work at the moment.

[jrfnl]

@gsherwood Understood. When you do have some headspace to think it over, the naming/directory structure question is my first priority.
Once that's decided upon, I can fix up all the commits and publish what I have so far so people can have a look and review/give feedback.

All in all, things are shaping up nicely and, though I keep thinking of more things, I'd say I'm about 90% done.

[jrfnl]

@gsherwood Quick question about the Common::suggestType() method: this method is currently quite limited and doesn't allow for short form types, union/intersection types or the PSR-5 array types.

Now I have seen numerous issues related to this, so I'd like to account for this, which leaves me with a choice for which I need input:

1. Refactor the method to allow for these additional types and add a new parameter to determine whether long/short form of types should be used.
   This would change the behaviour of the current sniffs using the method - Squiz/FunctionComment and Squiz/VariableComment - as more types would be recognized and therefore more corrections might be suggested.
   (Currently if the type is not recognized by the method because it uses an unknown format, the original value will be returned and no error will be thrown).
2. Don't refactor the method, but add a secondary method which builds onto the first and allows for the additional types.

As Squiz is a proprietary standard, I'd like know what you'd prefer.

If :two:, a $psr5 property could be added to the existing sniffs which could then be toggled to true in a ruleset to use the new secondary method and short form.

What would you prefer ?

[gsherwood]

As Squiz is a proprietary standard, I'd like know what you'd prefer.

I've never liked the fact that there are sniffs relying on the PHPCS core code to figure out short/long forms of types given preferences can differ between standards. I'd prefer to not have a method at all and let the sniffs decide themselves, but I realise that will result in a lot of repeated code.

So I think the best thing to do is change the core to understand the standard formats from phpdoc and psr-5 and use those in the Squiz sniffs as well. It's a BC break for those sniffs, but I want the Squiz standard to change here anyway. I was waiting for PSR-5 to be completed, but I've been waiting a while now and I don't think it's going to happen.

[jrfnl]

So I think the best thing to do is change the core to understand the standard formats from phpdoc and psr-5 and use those in the Squiz sniffs as well.

:+1: Done ;-)
And aside from the new long/short form parameter, I've also added an optional $allowedTypes parameter, which can overrule the default list which has now been extended to include all the types listed in the draft PSR-5.

Only question I still have would be whether the "long" form for true and false would be true/false or boolean ?

[jrfnl]

Oh and I've also added a wrapper method suggestTypeReturnTag() suggestReturnType() which adds void to the $allowedTypes list, while for param/var tags, it will not be allowed (by default, though changeable by passing a custom $allowedTypes parameter).

Edit: Hmm... just thinking, that may not even be needed as void would otherwise be seen as custom type name, like a class name and would be returned unchanged anyway.

[gsherwood]

Only question I still have would be whether the "long" form for true and false would be true/false or boolean ?

Given true/false are valid return types, the long form probably remains as true/false, unless I've missed some complexity here.

[gsherwood]

Just checking in: have you had a chance to think about the naming and how to pull this ?

I think pulling in the other two commits at once makes sense. I'll target them for 3.5.0 as well.

As for naming, I really dislike multi-word directories, so my vote is for src/Util/Sniffs or src/Util/Sniff - can't decide if I like the plural form or not. Every other directory besides Util uses plurals, so I'd probably lean that way.

[jrfnl]

@gsherwood Thanks for the input. That would hide these utility classes away in a deeper level, making them a little less easily discoverable, and - aside from the Standards directory - none of the others directories in src have deeper levels.

[gsherwood]

That would hide these utility classes away in a deeper level, making them a little less easily discoverable

I don't think they are hidden in there. I suspect people would look in Utils for utility functions and see another directory dedicated to sniff utility classes. It also allows for future util categories to have a place.

aside from the Standards directory - none of the others directories in src have deeper levels

True, but I don't think those other dirs are related to each other. This is a new collection of utility classes that will be used just as often as Util/Tokens. I'd prefer to keep them together in a Util folder and namespace.

[jrfnl]

Okidokie, src/Util/Sniffs it is.

Give me a few days to adjust all the individual commits. After that I will set up a draft PR.

[jrfnl]

Oh and just to be sure: we were going to drop the Utils from the class names too, weren't we ?

So, for just a sampling of the new classes:

Old file name | New file name | New class name
--- | --- | ---
src\Util\ConditionUtils | src\Util\Sniffs\Conditions | PHP_CodeSniffer\Util\Sniffs\Conditions
src\Util\FunctionDeclarationUtils | src\Util\Sniffs\FunctionDeclarations | PHP_CodeSniffer\Util\Sniffs\FunctionDeclarations
src\Util\ObjectDeclarationUtils | src\Util\Sniffs\ObjectDeclarations | PHP_CodeSniffer\Util\Sniffs\ObjectDeclarations
src\Util\NamespaceUtils | src\Util\Sniffs\Namespaces | PHP_CodeSniffer\Util\Sniffs\Namespaces

[gsherwood]

Oh and just to be sure: we were going to drop the Utils from the class names too, weren't we ?

Yes please

[jrfnl]

Ok, done all the renames and recommits. WIP is up as a draft PR #2456

• Draft changelog
• Draft developers upgrade guide for the wiki
• Draft issue for PHPCS 4.0.0 removal of deprecated features

[mwgamble]

Note: you can not add use statements for the new classes as they won't be available in older PHPCS versions. Note: if you choose this direction, you can not (yet) use the new methods available in the new utility classes.

I don't think this is accurate. This snippet works just fine in PHP 5.5 and up:

<?php
use NonExistentNamespace\NonExistentClass;
var_dump(class_exists(NonExistentClass::class));

https://3v4l.org/S5KTi

[jrfnl]

@mwgamble Interesting, I'll do some testing with one of the external standards, but I seem to remember this was problematic when I was making some of those cross-version compatible with PHPCS 2 and 3.

Providing the tests are successful, I'll remove that phrase from the Upgrade Guide. Thanks for your input!

[jrfnl]

@mwgamble Tested & found working. I've updated the Upgrade Guide to reflect this and expanded the code sample as well. Thanks!

[jrfnl]

Closing as this refactor is not going into PHPCS anymore.

It has been published separately as PHPCSUtils. More details in this comment: https://github.com/squizlabs/PHP_CodeSniffer/pull/2456#issuecomment-580492465