[WIP] PHPCS 3.5.0: refactor utility methods and add additional utility methods #2456
Conversation
Refactor to remove duplicate code. This new class abstracts out the `setUp()` and `tearDown()` methods which were included in nearly all the utility method test classes. The methods have been changed to static `setUpBeforeClass()` and `tearDownAfterClass()` with a static property as the initial processed `File` doesn't change between tests, so there is no need to tokenize and process the unit test case file for each test individually. This makes the utility method unit tests significantly faster as well as lower the memory usage. Additionally, the _standard_ against which the files are processed has been changed from `Generic` (75 sniffs) to `PSR1` (7 sniffs), which again makes the unit test suite for the Core utility functions significantly faster. The current implementation allows for only one test case file per utility method test file, but as the utility method tests are not sniff specific, adding additional tests files for a method if needed, should not be a problem. By default test case files are expected to be PHP `inc` files. Child classes can overload the static `$fileExtension` property when the test case file is a JS or CSS file.
This adds a new `getTargetToken()` method which can retrieve the target token for testing with higher precision than the (duplicate) code which was so far used in the individual test methods.
The improvements this method offers are:
* Avoid test leaking/contamination.
If/when the token to start the test from was retrieved by doing a `findNext()` from the delimiter comment, a typo could cause a token from the *next* test to be used for the testing instead of the target token.
If the expected results would be the same for both tests, this would go completely unnoticed.
This is now no longer possible.
The only requirement is that the delimiter comments start with `/* test...`.
* No more token counting when setting up the unit tests, just pass the target token type constant to this method and it will get you the correct token.
This also allows for not having to jump through hoops when deciding where to place the delimiter comment for the test.
* If the token a test looks for is a `T_STRING` or text based token, the optional `$tokenContent` allows for selecting the correct token, even when there are several of the same type in the test case line.
Implement use of the new AbstractMethodUnitTest::getTargetToken() method as well as abstract out the actual testing to a helper method as the logic was the same in all methods. I've chosen not to implement this with a data provider as the separate test methods creating the `$expected` arrays make the tests more readable and easier to understand.
Implement use of the new AbstractMethodUnitTest::getTargetToken() method as well as abstract out the actual testing to a helper method as the logic was the same in all methods. I've chosen not to implement this with a data provider as the separate test methods creating the `$expected` arrays make the tests more readable and easier to understand.
As the refactor will add a lot of new unit test files, let's automate the creation of the test suite some more by automatically adding all Test files within the `Test\Core` directory.
As far as I can see, Exceptions thrown by sniffs/utility methods when, for instance, a wrong token is passed, should be `RuntimeException`s, while Exceptions thrown by the `Tokenizer` classes should be `TokenizerException`s. In a number of places this was applied inconsistently. This has now been fixed. Aside from that, for quite a number of methods, the `Exception` being thrown has now been documented.
Move the methods related to condition checking out of the `File` class to a separate class with static methods. This deprecates the `File::hasCondition()` and the `File::getCondition()` methods in favour of the same in a new `Util\Sniffs\Conditions` class. This functionality is only used by a small percentage of sniffs, so is better placed in a dedicated class. This also allows for additional condition related functions to be added to this dedicated class. The deprecated methods should be removed in PHPCS 4.0.
The `File::getCondition()` and `File::hasCondition()` methods (now `Conditions::getCondition()` and `Conditions::hasCondition()`) did not have dedicated unit tests. These have now been added.
… more versatile This refactors the `Conditions::getCondition()` method to be more versatile with an eye on additional methods to be added later. It also removes some duplicate code and will allow the new methods to be added without code duplication. Includes additional unit tests covering the new functionality added to the `Conditions::getCondition()` method.
…ethods
This adds two additional utility methods:
* `getFirstCondition()` - to retrieve the first condition of a type passed in `$types` for a token or the first condition for a token if no types are specified.
This is effectively just an alias for the `getCondition()` method.
* `getLastCondition()` - to retrieve the last condition of a type passed in `$types` for a token or the last condition for a token if no types are specified.
Includes dedicated unit tests.
This commit is two-fold: On the one hand, it improves handling of anonymous classes which extend another class or implement an interface. This means that the error code for those errors will now correctly indicate that the _unused function parameter_ was found in an extended class or implemented interface. On the other hand, the sniff now recognizes that classes can be nested and will only look at the actual class in which the method is found, not at a higher level class. This fixes a bug were the error code would be incorrect for unused parameters in nested classes. Includes unit tests for both cases.
…fs\Conditions This adds four additional utility methods: * `isOOProperty()` - to check if a `T_VARIABLE` token is a class/trait property. Returns true/false. * `isOOConstant()` - to check if a `T_CONSTANT` token is a class/interface constant. Returns true/false. * `is_OOMethod()` - to check if a `T_FUNCTION` token is a class/interface/trait method. Returns true/false. * `validDirectScope()` - to check the direct condition of an arbitrary token against a set of valid scope conditions. Returns the integer stackPtr to the direct condition if valid or false otherwise. These methods have been battle-tested in the past year or two in the PHPCompatibility standard. Includes dedicated unit tests.
…::getMemberProperties() ... without changing the existing behaviour. It could be argued that the parse error warning can now be removed in favour of just throwing the Exception, but that would be a BC-break for sniffs using this method and expecting to receive an empty array instead of the Exception. I'd suggest making that change in PHPCS 4. Alternatively, the parse error warning could already be removed with just an empty array being returned for now.
…AbstractVariableSniff
1. Closures can not be declared as the value of a property.
This would be a parse error and one which I don't believe the `Conditions::isOOProperty()` should need to account for.
2. Interfaces can not declare properties.
The switch over to the `Conditions::isOOProperty()` method in the `AbstractVariableSniff` means that a variable declared as a property in an interface will no longer be seen as a property and therefore the warning which would be generated by the `File::getMemberProperties()` method will no longer be thrown.
The unit test files have been adjusted accordingly.
Interfaces cannot declared properties. Traits however can and were so far not tested.
Interfaces cannot declared properties. Traits however can and were so far not tested. The switch over to the `Conditions::isOOProperty()` method in the `AbstractVariableSniff` means that a variable declared as a property in an interface will no longer be seen as a property and therefore the warning which would be generated by the `File::getMemberProperties()` method will no longer be thrown. However, if the construct is changed to a `trait`, the normal logic will kick in and the `PrivateNoUnderscore` error will be thrown.
…ons utility methods
This adds ten new utility/convenience methods. The first two are targeted at open/close parentheses tokens: * `getOwner()` - to retrieve the stack pointer to the parentheses owner of an open/close parenthesis. Returns int or false if the parenthesis does not have an owner. * `isOwnerIn()` - to check whether the parenthesis owner of an open/close parenthesis is within a limited set of valid owners. Returns bool. The other eight methods are targeted at arbitrary tokens: * `hasOwner()` - to check whether the passed token is nested within parentheses owned by one of the valid owners. Returns bool. * `lastOwnerIn()` - to check whether the owner of a direct wrapping set of parentheses is within a limited set of acceptable tokens. Returns int/false. * `getFirstOpener()` - to retrieve the position of the opener to the first set of parentheses an arbitrary token is wrapped in where the parentheses owner is within the set of valid owners. Returns int/false. * `getFirstCloser()` - to retrieve the position of the closer to the first set of parentheses an arbitrary token is wrapped in where the parentheses owner is within the set of valid owners. Returns int/false. * `getFirstOwner()` - to retrieve the position of the parentheses owner to the first set of parentheses an arbitrary token is wrapped in where the parentheses owner is within the set of valid owners. Returns int/false. * `getLastOpener()` - to retrieve the position of the opener to the last set of parentheses an arbitrary token is wrapped in where the parentheses owner is within the set of valid owners. Returns int/false. * `getLastCloser()` - to retrieve the position of the closer to the last set of parentheses an arbitrary token is wrapped in where the parentheses owner is within the set of valid owners. Returns int/false. * `getLastOwner()` - to retrieve the position of the parentheses owner to the last set of parentheses an arbitrary token is wrapped in where the parentheses owner is within the set of valid owners. Returns int/false. These last eight, all use the private `nestedParensWalker()` helper method under the hood. Where the return value is of the type `int|false`, the integer stack pointer to the parentheses opener/closer/owner will be returned or `false` when: * the token is not wrapped in parentheses; * the parentheses owner is not within the set of `$validOwners` passed; * and in the case of `owner`: when the desired set of parenthesis does not have an owner. Includes dedicated unit tests.
…l\Sniffs\Conditions
…class Implement the use of the new `Parentheses::getOwner()` method in the `AbstractArraySniff` class.
Implement the use of the new `Parentheses::isOwnerIn()` method in the `Squiz.PHP.NonExecutableCode` sniff.
…eStatements Implement the use of the new `Parentheses::hasOwner()` method in the `Generic.Formatting.DisallowMultipleStatements` sniff.
…ntAlignment Implement the use of the new `Parentheses::hasOwner()` method in the `Generic.Formatting.MultipleStatementAlignment` sniff.
Foxtrot merges should typically be avoided: |
|
Thanks for reviewing all my comments. I've resolved all the conversations where we agree and left the others open for more discussion. For such a big PR, only having 6 disagreements seems like a pretty good result to me :) |
Yes, very happy with this. |
|
@antograssiot @webimpress @photodude @klausi @TomasVotruba @dingo-d @GaryJones @umherirrender @djoos @weierophinney @louisl @Majkl578 @dereuromark @mbabker @lenaorobei @VasekPurchart @sirbrillig @legoktm @kukulich Apologies for the group ping, but I would love to see your input in the still open discussion point regarding the naming of the utility classes. These are the open discussions which would benefit from your input:
As maintainers of some of the largest public external standards for PHPCS you will likely be working with these classes, so your input on what you would find logical names will be invaluable and can help us take the most-balanced decision. Thanks! P.S.: If you've handed over the baton to someone else in the mean time or feel I've missed someone in the above list, please feel free to ping them and invite them to this discussion. No offense meant to anyone I forgot to ping. |
|
@jrfnl Good work and thanks for the notification. |
|
@GaryJones @dingo-d @TomasVotruba @louisl @sirbrillig Thank you for your input so far! @gsherwood I propose we leave the decisions till beginning of next week to allow some more people to add their voice to the discussion. I realize it's a holiday period in parts of the world, so keeping my fingers crossed. |
|
Unfortunately we haven't had more input since my last comment. All the same, the input we did receive was useful. Thanks everyone who contributed to the discussions! So I think it's decision time. Here's the summary of my take-aways from the input received so far: General note on appending
|
I'd be happy to consider this again. It would make naming these classes easier. See below.
The 3 developers who commented on the class all seemed to think that the methods could move to TextStrings. I think TextStrings should be renamed to StringUtil and the methods merged. One place to go looking for string-based utility functions. I think the name fits nicely with the other util classes. I would set the class names as:
This is where my thinking is right now. |
|
Argh.... seriously ? That would mean not just rebasing, but re-doing every single commit for a second time to bring back the |
Happy to leave this for 3.6.0 given the work and the fact discussion are still ongoing. I really need to get 3.5.0 out ASAP, but then I could provide more help over the next few months. |
|
I definitely think I'd prefer having |
I'm not as this being up in the air is, and has been for months, a blocker for several standards I work on. I think we can all agree that this refactor is a good thing:
Asking me to rename a few more things still is fine. Asking me to rename every single thing a second time is not acceptable. That's just being callous with my time. I could have decided straight off the bat to create this as a separate PHPCS utility repo which standards could pull in as a dependency and use. Instead I've tried to be a good open source citizen and give back, discussed things first in issue #2189 and over time pulled this PR, but my patience is running out after ten months.
I've been trying to coach this through now for 10 months, waiting for responses for weeks, sometimes months on end. Sorry to say, but the history of this repo does not allow me to have much confidence in a statement like I could provide more help over the next few months, quite apart from it then still being a blocker for many more months. I suggest we all sleep on it over the weekend and reconvene on Monday. But to be fair, if the renaming all things stands, I will be pulling the PR and publishing this as a separate package. |
|
@jrfnl Sorry, I wasn't trying to push the PR back because I don't think it's amazing. I was just trying to take some pressure off you. I misread that - I'm sorry.
I was not aware of how long the process was to rename these classes. I'm not concerned about the commit history showing the rename, so I thought you were going to go in and commit a new change. If you're not bothered by the history, I can find some time this week to get that rename done, but wouldn't try and clean up the history. I'd probably do a (careful) string replace over the code base and fix things until it works.
It think would matter, especially if I made the name changes. They'd soon be incompatible, and would be even less compatible as time went on. I think it would be better for the PHP community if we can figure this out rather than forcing a split, but I understand if you want to create a new project instead. I'm not trying to waste your time. I appreciate everything you've done for this project more than you know. I talk about you at work and home often because your contributions have kept this project - one that I am passionate about - healthy and growing while my life has gotten increasingly more complicated and busy. I've had some really great contributors help me out in the past, but none like you, and I don't mind saying this in public because I'm sure they would agree. So whatever happens here, thank you. |
@gsherwood I think you misunderstood part of my post.
What I meant by that is that I would publish the new and improved utility functions as an PHPCS external standard without sniffs. I was not talking about creating and maintaining a different fork of this repo. The advantages of having the utility functions available as a separate package:
The disadvantages of it being a separate package:
So, in other words, advantages for everyone, except PHPCS itself. It may take end-users a little while to find the new standards and such, but I don't expect that will take too long, especially once PSR-12 is approved and published and they find the standard here to be far from complete.
Sorry, no. Taking pressure off me, would have been reviewing & merging this as soon as the
After all the effort which has gone into this PR to have everything as clean atomic commits, that, again, is just callous. I will step away from keyboard now and not take a final decision until after the weekend, but I stand by what I said earlier: If the request to rename everything stands, I will pull the PR. |
|
@jrfnl and I have spoken privately about this PR over the last couple of weeks and we've decided that the best way forward is to release these util classes as a separate package. This will immediately provide new tools for sniff developers to use without needing to consider the impact on the core of PHPCS itself as they evolve. |
This is a very disappointing outcome IMO. I thought this was also about cleaning up many sniffs that desperately needed refactoring, in addition to providing important utility functionality for general use. |
|
I'm also saddened by this decision. This is a PR that is doing so much more than just a simple cleanup, it helps us when working on external standards (while working on the standards there are couple of utility functions that I identified that would most likely be covered in this PR). Having a separate package to maintain can just cause harm in the long run imo. |
|
So is this PR just dead in the water now? Months of hard work left to waste because of a naming dispute? |
|
@mwgamble I'll be publishing the functions elsewhere soon. They will be available for external standards supporting PHPCS 2.6.0+ up to |
|
FYI: I've tagged the first alpha release for PHPCSUtils. You will find nearly all the new utilities from this PR there plus many more additional utility functions, token collections as static properties, abstract sniff classes and fixers for use with PHPCS. On top of that a lot of the PHPCS native utility functions are also available via PHPCSUtils in a BackCompat layer, allowing external standards to use the latest version of the PHPCS native utilities even while still supporting older PHPCS versions. All the utilities published are compatible with PHPCS 2.6.0 - There are two classes which I've held held back for now as I deem them to need further improvement ( I'm fully aware that the readme is very sparse at the moment. I'm working on that (and many more things) and it will be filled in before the https://github.com/PHPCSStandards/PHPCSUtils The two new sniffs which were included in this PR https://github.com/PHPCSStandards/PHPCSExtra Both more utility methods, as well as more new sniffs can be expected to be published in these packages in the (near) future. |
🚧 This is a draft PR to make what I've been working on for this refactor visible and accessible to interested parties. 🚧
masterwhenever relevant.Having said all that: feedback is very welcome.
Current WIP
Things I'm still working on, which may or may not be added to this PR (listed in order of the likelihood of them to make it into this PR):
TextStrings::stripQuotes()can be used/implemented.Comments::findFunctionComment()in theSquiz.WhiteSpace.FunctionSpacingsniff.Comments::findFileComment()AbstractDocblockSniffTextStrings::getInterpolatedVariables()andTextStrings::stripInterpolatedVariables().Mostly done, but some nasty edge cases are failing.
FQNamesVariables::isAssignment()T_AStoken is used for.Some of the above may be pulled separately at a later point in time once this PR has been merged.
Introduction
This refactor has been done with an eye to:
The basic constraints which this refactor tries to adhere to:
Where existing utility methods did not have dedicated unit tests yet, those have been added before making any significant changes to the methods.
public staticand can be called from anywhere (unless specifically designed for internal/under the hood use).Unit tests
In general, I've erred on the side of caution and have added abundant unit tests, sometimes up to the point of redundancy.
All of the newly introduced utility classes have a code coverage of > 98% based on their dedicated unit tests alone.
Where the test coverage is lower, it either involves conditions related to the availability of PHP extensions, exceptional circumstances which would only occur when there are rare parse errors/during live coding or artifacts of code coverage analysis weirdness related to function end comments being seen as code lines.
Notes:
Comments: The methods are 100% covered. The stray lines is PHPUnit getting confused over function end comments.ConstructNames: One of the "old" methods (ConstructNames::isUnderscoreName) is not unit tested yet. Unit tests for that will be added when the additional methods to determine whether construct names comply with various naming conventions, will be added.Orthography: This is due to conditions related to the availability of the MBString PHP extension.UseStatements: The methods are 100% covered. The stray line is PHPUnit getting confused over a function end comment.Performance
I've done a numbers of checks with Blackfire, but haven't been able to draw any real conclusions from these.
The slower part could be explained by the number of additional files which need to be scanned, so this is inconclusive.
phpunit ./tests/Standards/AllSniffs.phpshows no significant difference (< 2% difference in memory and run time which can be explained by the additional unit tests which have been added for bug fixes and enhancements to sniffs).phpunit ./tests/Core/AllTests.phpshows a whopping -90% run time difference and a + 27.7% memory diff.The run time going down significantly is due to the changes as previously pulled in Core tests: set up abstract class and remove code duplication #2295, which are included in this PR.
I expect the memory difference to be due to the enormous amount of extra unit tests for this run - previously 229, now 1433.
It may be warranted to do some additional performance tests, for instance running each of the PHPCS native rulesets against a large project such as WordPress using first
masterand then this refactor branch, to see if that provides us with conclusive results.Reviewing the commits
All commits have been set up as atomic commits and, safe for a few which highlight underlying issues which are subsequently fixed in the commits straight after, all individual commits will pass all CI checks.
As the commits tell a story, this PR will be easiest to review per commit in the order I've placed the commits.
I've implemented the use of the new utility methods wherever I deemed appropriate based on keyword searches.
There may however be more sniffs which could benefit from the new methods. Suggestions welcome, though this can/should probably be fixed in separate PRs after this PR has gone in.
Documentation
These documents will be kept up to date with additional changes made to this branch.
Discussion points
File::getMemberProperties()(nowFunctionDeclarations::getMemberProperties()) method throws a warning when a property is declared in an interface (= parse error). This behaviour is currently unchanged.It could be argued that the parse error warning can now be removed in favour of just throwing the
Exception, but that would be a BC-break for sniffs using this method and expecting to receive an empty array instead of theException.I'd suggest making that change in PHPCS 4. Also see issue Suggestion: remove warnings/errors for detected parse errors (PHPCS 4) #2455.
Alternatively, the parse error warning could already be removed with just an empty array being returned for now.
$magicMethods,$methodsDoubleUnderscoreand the$magicFunctionsproperties from theGeneric.NamingConventions.CamelCapsFunctionNameand thePEAR.NamingConventions.ValidFunctionNamesniffs to the newFunctionDeclarationsclass creates a minor BC-break.The properties are being recreated in their old form via the class constructors of those two sniffs, but any sniff which would extend one of those sniffs and overload the class constructor without calling the parent constructor will have to deal with a BC-break as in that case, the properties will be empty.
As an alternative, the old, deprecated properties could be left as they were in the sniff classes for the time being and the recreation code removed.
In that case, there would be no BC break, but the content of the properties would still be in three places in the code base.
$phpReservedVarsfrom thePHP_CodeSniffer\Sniffs\AbstractVariableSniffclass to thePHP_CodeSniffer\Util\Sniffs\Variablesclass creates a similar minor BC-break if a child class overloads the constructor without calling the parent constructor.Changes to existing sniff unit tests
This refactor has been done in such way to avoid backward-compatibility breaks.
Nearly all sniff unit tests still pass without any adjustments to the unit test files, other than adding additional test cases for bugs which have been fixed or enhancements a sniff has received through this refactor.
The only sniff unit test files which have been adjusted are:
Squiz.Scope.MemberVarScope- the unit test file contained a parse error (closure as class property value in the initial declaration) which theAbstractVariableSniffaccounted for.This particular parse error is no longer accounted for, so the sniff will now throw some additional errors in that specific situation (which shouldn't occur in real life anyway).
Squiz.WhiteSpace.MemberVarSpacingandZend.NamingConvention.ValidVariableName- the unit test files contained parse errors (properties declared in an interface). TheAbstractVariableSniffwill no longer pass those on as "member variables".Traits, however, can contain properties and were so far untested for both sniffs, so I've taken the liberty to change the test cases from
interfaces totraits.Squiz.Arrays.ArrayDeclaration- contained a unit test related to short lists which was originally added to check for aTokenizerissue and not to test this sniff specifically - see Short array list syntax not correctly tokenized if short array is the first content in a file #1971.Squiz.Commenting.FunctionComment- contained a PSR-5 style return type which is now fixable.For the most part, I've not removed unit tests from sniff test case files, even when the case being tested is now also tested through the dedicated utility method unit tests.
The only exception to this is for the magic method related test cases for the
Generic.NamingConventions.CamelCapsFunctionName,PSR1.Methods.CamelCapsMethodNameandPEAR.NamingConventions.ValidFunctionNamesniffs.For those three sniffs, I've removed most magic method related unit test cases, just leaving a few spot-checks in place.
Fixes #2189 (refactor)
Fixes #1984 (short list constructs)
Fixes #1864 (allow Squiz comment sniffs to suggest short form types)
(Maybe) Fixes #2222 (abstract docblock sniff)
Closes #2128 (extended interfaces)
Closes #2295 (core utility method testing refactor)
Closes #2353 (consistent exceptions)
Closes #2368 (MySource/AjaxComparison)
To do once this PR is accepted/merged:
Squiz.Commenting.FunctionCommentandSquiz.Commenting.VariableCommentsniffs.Other open action points
These can be sorted out after this PR has been merged, low prio.
Variables"superglobal" methods should possible be used by theMySource.PHP.GetRequestDatasniff, but as the specs of that sniff (or rather of the suggested method to use instead) are not clear, I have not actioned this at this time.PassedParameters::getParameters()method can be implemented in theGeneric.Functions.FunctionCallArgumentSpacingsniff.