Fix ForbiddenNames sniff for use statements.
#271
Conversation
|
This looks like a much more complete version of my original fix. 👍 👍 |
|
@ryanneufeld Glad you approve. Thanks for taking the time to have a look. |
jrfnl
force-pushed
the
feature/issue-117-124-use-statements
branch
2 times, most recently
from
0df0222
to
19564ed
Compare
|
Ok, I think I've solved this for PHPCS 1.x as well now. I've rebased & re-done the commits to have them nice & tidy. I've added one additional utility function which handles the difference between PHPCS 1.x and 2.x with regards to use statements and added unit tests for the function as well. |
| * PHPCS cross-version compatibility method. | ||
| * | ||
| * In PHPCS 1.x no conditions are set for a scoped use statement. | ||
| * This method works round that limitation. |
| { | ||
| static $isLowPHPCS, $ignoreTokens; | ||
|
|
||
| if (isset($isLowPHPCS) === false) { |
There was a problem hiding this comment.
imho $isLowPHPCS === null looks cleaner and has (possibly) less instructions.
but then again i'm not familiar with this project's codestyle guide :)
There was a problem hiding this comment.
i'm not familiar with this project's codestyle guide
At this moment, there is none - see #137
I've tried to stay close to the PHPCS code style as that's what's used predominantly.
There was a problem hiding this comment.
Wondering about something completely different : shouldn't we define the visibility of the static variables ?
There was a problem hiding this comment.
Only if we can - i.e. class variables.
In this case, they are static variables within a method, so visibility is not an option/does not apply.
…ate/function/const within `use` statements. The test cases for function/const are in a separate file as they need a specific testVersion to test against.
Added new test case files to generate to the `generate-forbidden-names-test-files` script: * `class-use-const` * `class-use-function` * `class-use-trait-alias-public-method` * `class-use-trait-alias-protected-method` * `class-use-trait-alias-private-method` * `interface` (file had previously been manually added) * `interface-extends` (file had previously been manually added) Adjusted the anonymous function for `class-use-trait-alias-method` to exclude `private/protected/public` from the generated test file as those are valid. Added the new test case files to the data provider in the sniff test file.
This method handles checking whether a token is in a T_USE scope and works around limitations of the PHPCS 1.x implementation where `use {}` did not yet create a scope condition.
This method should be favoured over directly calling `tokenHasScope()` for checking whether a token is in a T_USE scope.
Includes unit tests for the new utility method.
…/const in use statements. Visibility: Added logic to check for visibility keywords and handle them correctly. Function/const: The previous fix would bow out as soon as one of these was encountered. That is incorrect as the keyword without an alias is still invalid. The current fix will move on to the alias to evaluate that instead or error on the function/const keyword if no alias is found. Also: Removed the extra code which was added in PHPCompatibility#126 from the test file as it's no longer needed.
- Improve some variable names for clarity. - Add/improve some documentation. - Prevent skipping over tokens by returning after a function call instead of returning the result. - Make sure that the `tokenHasScope()` method is tested for both array as well as integer input for the `$validScopes` parameter. - Remove unnecessary test `setUp()` method.
jrfnl
force-pushed
the
feature/issue-117-124-use-statements
branch
from
19564ed
to
cce68b3
Compare
|
@wimg Any questions or remarks ? |
|
Thanks for merging, three more "small" ones and then three really big ones coming up. |
Fix visibility in
usestatements and improve earlier fix forfunction/constinusestatements.Visibility: Added logic to check for visibility keywords and handle them correctly.
Function/const: The previous fix would bow out as soon as one of these was encountered. That is AFAICS incorrect as the keyword without an alias is still invalid.
The current fix will move on to the alias to evaluate that instead or error on the
function/constkeyword if no alias is found.Also: Removed the extra code which was added in #126 from the test file as it's no longer needed.
Fixes #117
Fixes #124
Fixes #267
/cc @ryanneufeld - I'd appreciate it if you would review as you created the original fix for the
function/constkeyword withinusestatements.Added unit test and test cases for valid usage of
public/protected/private/function/constwithinusestatements.The test case for
function/constare in a separate file as they need a specifictestVersionto test against.Regenerated the test case files for the ForbiddenNames sniff.
Added new test case files to generate to the
generate-forbidden-names-test-filesscript:class-use-constclass-use-functionclass-use-trait-alias-public-methodclass-use-trait-alias-protected-methodclass-use-trait-alias-private-methodinterface(file had previously been manually added)interface-extends(file had previously been manually added)Adjusted the anonymous function for
class-use-trait-alias-methodto excludeprivate/protected/publicfrom the generated test file as those are valid.Added the new test case files to the data provider in the sniff test file.
N.B.: The unit tests are failing on PHPCS 1.x as PHPCS 1.x does not create a scope for ausestatement with curly brackets.I'm tempted to say: sod PHPCS 1.x and skip the failing tests for PHPCS 1.x..... Fixing this to also work on PHPCS 1.x just seems not worth the time.@wimg what do you say ?