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 File: add new File::findExtendedInterfaceNames() utility method #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 Core tests: set up abstract class and remove code duplication #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 File: add new File::findExtendedInterfaceNames() utility method #2128)
  • New findExtendedInterfaceNames() - see PR File: add new File::findExtendedInterfaceNames() utility method #2128
  • findImplementedInterfaceNames() (moved from File and adjusted based on File: add new File::findExtendedInterfaceNames() utility method #2128)
  • New (private) examineObjectDeclarationSignature() - see PR File: add new File::findExtendedInterfaceNames() utility method #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 :)

😀

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.