Add MagicMethodCasingFixer #3739
Conversation
SpacePossum
commented
May 11, 2018
SpacePossum
commented
README.rst
Outdated
| @@ -743,6 +743,10 @@ Choose from the list of available rules: | |||
|
|
|||
| Magic constants should be referred to using the correct casing. | |||
|
|
|||
| * **magic_method_casing** | |||
| ], | ||
| '__callstatic' => [ | ||
| 'name' => '__callStatic', | ||
| 'argumentsRange' => [2, 2], |
There was a problem hiding this comment.
why is casing related to number of params ?
There was a problem hiding this comment.
If the number of arguments do not match the signature as defined as magic PHP method it does not get fixed as it assumed as overridden, to be a safe-ish side, you think we could do without? happy to remove
There was a problem hiding this comment.
all magic methods are in fact overriden.
I assume you are talking about overloading methods, but that doesn't work in PHP and PHP will still treat them as magic methods. IMO, casing shall be respected even if one provides wrong number of params.
There was a problem hiding this comment.
that simplifies things :D
will update soon :)
|
idea is good, can you fix travis before we would review PR ? |
|
the failures do not seem related to this PR, I'm looking into those nevertheless in another PR (not sure about a time frame though) |
looks pretty much related to this PR |
doesn't look much related though |
|
you are talking about nightly build which is allowed to failed and that we have meta 7.3 ticket that mentions this issue already I'm talking about 5.6 build that must not fail errors on those builds are different |
|
just listing all failures to look into about this PR so I can work on those, not stating these have the same weight or priority |
|
5.6 build. all |
|
rebased, I need Travis to see the 5.6 failures because only on that version the linter checks to much |
| private function isMagicMethodCallInWrongCasing(Tokens $tokens, $index) | ||
| { | ||
| $prevIndex = $tokens->getPrevMeaningfulToken($index); | ||
| if (!$tokens[$prevIndex]->equals([T_OBJECT_OPERATOR, '->'])) { |
There was a problem hiding this comment.
What about static calls? e.g. for __callStatic
There was a problem hiding this comment.
one will not call Foo::__callStatic() but Foo::undefinedMethod(), so nothing to detect here
There was a problem hiding this comment.
fixed for this case nevertheless
|
@SpacePossum , is there anything missing in this PR ? |
|
there is this very theoretical missed opportunity case, but I'm happy with this PR as it is now and therefore I think it is ready to go :) |
| * | ||
| * @return bool | ||
| */ | ||
| private function isMagicMethodSignatureInWrongCasing(Tokens $tokens, $index) |
There was a problem hiding this comment.
The method name is misleading IMO: it doesn't check if the tokens are a magic method with wrong casing, it only checks if they are a function definition.
| } | ||
|
|
||
| $nextIndex = $tokens->getNextMeaningfulToken($index); | ||
| if (!$tokens[$nextIndex]->equals('(')) { |
There was a problem hiding this comment.
Could be simplified:
return $tokens[$nextIndex]->equals('(');| } | ||
|
|
||
| $nextIndex = $tokens->getNextMeaningfulToken($index); | ||
| if (!$tokens[$nextIndex]->equals('(')) { |
There was a problem hiding this comment.
Could be simplified:
return $tokens[$nextIndex]->equals('(');| * | ||
| * @return bool | ||
| */ | ||
| private function isMagicMethodCallInWrongCasing(Tokens $tokens, $index) |
There was a problem hiding this comment.
The method name is misleading IMO: it doesn't check if the tokens are a magic method call with wrong casing, it only checks if they are a method call.
| { | ||
| return [ | ||
| [ | ||
| '<?php |
There was a problem hiding this comment.
I would split this case in several smaller ones to ease debugging if needed.
|
|
||
| echo $a->__sleep; | ||
| ', | ||
| ], |
There was a problem hiding this comment.
Please add the following cases:
<?php
function __Tostring() {}<?php
__Tostring();There was a problem hiding this comment.
<?php
function __Tostring() {}
can we note this as a false-fixing and move on, like anyone who creates functions with magic method names is not likely to use this tool anyway ;)
There was a problem hiding this comment.
the one who will inherit codebase after him would ;P
agree, edge case...
|
I fixed both edge cases and all tests are green. @julienfalque can I bother you with asking for another review? |
| continue; | ||
| } | ||
|
|
||
| if ('__callstatic' === $name && $this->isStaticMethodCall($tokens, $index)) { |
There was a problem hiding this comment.
Is it handled by this fixer? e.g. Foo::__sEt_State([]);
There was a problem hiding this comment.
it isn't I think... hmmm
|
|
||
| for ($index = 1; $index < $tokenCount - 2; ++$index) { | ||
| if (0 === $inClass && $tokens[$index]->isClassy()) { | ||
| $inClass = 1; |
There was a problem hiding this comment.
Would it be easier to make this variable be null by default and then store the result of $tokens->findBlockEnd() so that you just compare it to the current index instead of counting { and } tokens? The variable name could become e.g. $inClassUntil.
There was a problem hiding this comment.
That would require scanning from the classy open { till the end and than go back scanning all the tokens again for candidates, which is not very efficient.
| } | ||
|
|
||
| if ($this->isMethodSignature($tokens, $index)) { | ||
| if (0 !== $inClass) { |
There was a problem hiding this comment.
This is misleading: the current index is a method signature, as just checked before, so the current index is in a class. The method name is actually wrong and should be isFunctionSignature IMO.
There was a problem hiding this comment.
I think this works well ATM
There was a problem hiding this comment.
Yes, but this can be misleading when you don't know what the code does yet. The more the code is clear about that, the easier to understand it and maintain it. The first thing I asked myself was why is the second "are we in a class" check required? Then I remembered the previous condition actually doesn't check that.
There was a problem hiding this comment.
The more the code is clear about that, the easier to understand it and maintain it.
I understand that concept
why is the second "are we in a class" check required?
It isn't, however if we don't do that global functions named the same a magic methods get fixed as well ( even though it would change code behavior) and you asked me to update the fixer not to do that.
There was a problem hiding this comment.
I've renamed the method and added a comment in the code, lemme know if this is sufficient.
There was a problem hiding this comment.
It is, thanks :) Just a small typo: an method => a method.
There was a problem hiding this comment.
fixed, thanks :)
| $fixerReflection = new \ReflectionClass(MagicMethodCasingFixer::class); | ||
| $property = $fixerReflection->getProperty('magicNames'); | ||
| $property->setAccessible(true); | ||
| $allMethodNames = $property->getValue(); |
There was a problem hiding this comment.
I think we should avoid relying on such internal properties to generate test cases: if one removes values from it, tests won't fail as they won't run at all.
There was a problem hiding this comment.
so you prefer listing all in test as well, so if one is added to the fixer but not to tests, the test won't fail?
There was a problem hiding this comment.
Exactly. When removing code, if tests don't fail, one might think the removed code was not used or some other change replaced it, which is not true. A missing test when adding code is easily spotted during review.
There was a problem hiding this comment.
for me adding or removing is the same type of operation, so;
current: if one removes an entry from the fixer the test still passes
suggested: if one adds an entry to the fixer the test still passes
the only diff is the list was copyed to the test, but the coverage remains the same.
A missing test when adding code is easily spotted during review.
I don't see why this would be different for when removing code over adding code?
There was a problem hiding this comment.
In both cases this will have to be spotted during review. But it is different when nobody spots it. When removing code and test, you probably break something for users. When adding new code without test, you just add something that (probably) works as expected, or at least doesn't break existing tests.
In general, tests should be as much decoupled from the code itself as possible. The point is to be defensive. This is similar to Symfony doc's advice regarding hardcoded URLs in tests.
There was a problem hiding this comment.
Well this is one pattern against another; decoupled test or DRY: maintaining the same list of static values at two spots in the code.
I understand the arguments; I just out weight the value of not having two list + generating tests cases so I don't forget one, over the more theoretical decoupling of test argument.
| */ | ||
| public function isCandidate(Tokens $tokens) | ||
| { | ||
| return $tokens->isTokenKindFound(T_STRING); |
There was a problem hiding this comment.
Can we add && $tokens->isAnyTokenKindsFound([T_FUNCTION, T_OBJECT_OPERATOR, T_PAAMAYIM_NEKUDOTAYIM]);?
There was a problem hiding this comment.
I think T_PAAMAYIM_NEKUDOTAYIM should be T_DOUBLE_COLON here? sounds like a good idea to me :)
|
anything left here? I would like to move on ;) |
| */ | ||
| final class MagicMethodCasingFixer extends AbstractFixer | ||
| { | ||
| private static $magicNames = [ |
There was a problem hiding this comment.
Constant? Or is the private more important?
There was a problem hiding this comment.
I pick scoping over constant, when we bump PHP requirement we can use scoped const, till than I prefer this one
keradus
changed the title
|
Thank you @SpacePossum. |
This PR was merged into the 2.13-dev branch. Discussion ---------- Add MagicMethodCasingFixer ``` $ php php-cs-fixer describe magic_method_casing Description of magic_method_casing rule. Magic method definitions and calls must be using the correct casing. Fixing examples: * Example #1. ---------- begin diff ---------- --- Original +++ New @@ -1,7 +1,7 @@ <?php class Foo { - public function __Sleep() + public function __sleep() { } } ----------- end diff ----------- * Example #2. ---------- begin diff ---------- --- Original +++ New @@ -1,2 +1,2 @@ <?php -$foo->__INVOKE(1); +$foo->__invoke(1); ----------- end diff ----------- ``` Commits ------- e45919f Add MagicMethodCasingFixer
