feature: Order of PHPDoc @param annotations#3909
feature: Order of PHPDoc @param annotations#3909Wirone merged 14 commits intoPHP-CS-Fixer:masterfrom grubersjoe:feature-phpdoc-param-order
Conversation
|
Should I squash all commits into a single one? |
| $docAnnotation->remove(); | ||
| $doc | ||
| ->getLine($docAnnotation->getStart()) | ||
| ->setContent($orderedAnnotations[$i]); |
There was a problem hiding this comment.
The fixer crashes on this line if I test it with a nested doc block.
I added this test case
public function testNestedPhpdoc()
{
$expected = <<<'EOT'
<?php
/**
* @param string[] $array
* @param callable $callback {
* @param string $value
* @param int $key
* @param mixed $userdata
* }
* @param mixed $userdata
*
* @return bool
*/
function string_array_walk(array &$array, callable $callback, $userdata = null) {}
EOT;
$input = <<<'EOT'
<?php
/**
* @param callable $callback {
* @param string $value
* @param int $key
* @param mixed $userdata
* }
* @param string[] $array
* @param mixed $userdata
*
* @return bool
*/
function string_array_walk(array &$array, callable $callback, $userdata = null) {}
EOT;
$this->doTest($expected, $input);
}And the result is this:
Undefined offset: 2
php-cs-fixer/vendor/symfony/phpunit-bridge/DeprecationErrorHandler.php:108
php-cs-fixer/src/Fixer/Phpdoc/PhpdocParamOrderFixer.php:150
php-cs-fixer/src/Fixer/Phpdoc/PhpdocParamOrderFixer.php:79
php-cs-fixer/src/AbstractFixer.php:75
php-cs-fixer/tests/Test/AbstractFixerTestCase.php:127
php-cs-fixer/tests/Fixer/Phpdoc/PhpdocParamOrderFixerTest.php:572
php-cs-fixer/vendor/phpunitgoodpractices/traits/src/ExpectationViaCodeOverAnnotationTrait7.php:39
There was a problem hiding this comment.
I suggest you fix only "first level" annotations, the same way as can be seen in PR 3911.
There was a problem hiding this comment.
Can you add a test case with @param Foo|Bar $x, @param ?Foo $x and @param Foo[]|\Bar\Baz $x?
There was a problem hiding this comment.
@dmvdbrugge I'll look into it.
@kubawerlos Do you mean a testcase like this (I'm a bit confused about the three $x)?
<?php
class C {
/**
* @param Foo|Bar $y
* @param ?Foo $z
* @param Foo[]|\Bar\Baz $x
*/
public function m($x, $y, $z) {}
}There was a problem hiding this comment.
Yeah, some mix of really "creative" types.
| */ | ||
| private function findParamAnnotationByIdentifier(array $docParams, $identifier) | ||
| { | ||
| $regex = sprintf('/\*\s*@param\s*\w*\s*\%s/', $identifier); |
There was a problem hiding this comment.
It would be cleaner to use preg_quote instead of escaping %s.
There was a problem hiding this comment.
Good call. The regex has also changed to support more complex types like Foo[]|\Bar\Baz. It doesn't check the order of operators, though. I'm not sure if it's necessary to take this into account. Invalid @param annotations would be possible right now, but I don't know if it should be the fixer's task to check validity thoroughly.
|
Another case (obfuscated from an existing project, I wouldn't came with such case by myself, I'm not that creative) that will crash the fixer: <?php
/**
* @param Foo $fooBar
* @param $fooSomethingNotMatchingTheName
* @param OtherClassLorem $x
*/
function f(Foo $fooBar, Payment $foo, OtherClassLoremIpsum $y)
{
} |
|
I've fixed the open issues and added (successfully running) unit tests for them. Nested Docblocks are now supported and the fixer should no longer crash. |
| $blockEnd = Preg::match('/\s*}\s*$/', $param->getContent()); | ||
|
|
||
| if ($blockStart) { | ||
| $inBlock = true; |
There was a problem hiding this comment.
The fact that you only do true/false for $inBlock suggests that this still breaks for a double-nested (or more) doc block. Nesting could happen infinitely deep, and params in a nested block could have the same name as a block higher in the chain. I don't know if this is a case we want to support, but the spec allows it.
There was a problem hiding this comment.
You are right. I've adapted the code to support deeply nested Docblocks and added tests.
|
What about adding the fixer to our But not yet, @grubersjoe, when you give it a go and you will find - among good changes - that PHPDoc for |
|
That's right @kubawerlos. I forgot to also check for types like From my perspective the fixer seems to work correctly now and might be merged. I guess I should squash all my commits to a single one, right? diff --git a/src/Fixer/Alias/SetTypeToCastFixer.php b/src/Fixer/Alias/SetTypeToCastFixer.php
index a02f78a7..40327314 100644
--- a/src/Fixer/Alias/SetTypeToCastFixer.php
+++ b/src/Fixer/Alias/SetTypeToCastFixer.php
@@ -196,15 +196,15 @@ private function removeSettypeCall(
$tokens->clearAt($functionNameIndex); // we'll be inserting here so no need to merge the space tokens
$tokens->clearEmptyTokens();
}
/**
* @param Tokens $tokens
- * @param Token $castToken
* @param int $functionNameIndex
* @param Token $argumentToken
+ * @param Token $castToken
*/
private function fixSettypeCall(
Tokens $tokens,
$functionNameIndex,
Token $argumentToken,
Token $castToken
diff --git a/tests/Doctrine/Annotation/TokenTest.php b/tests/Doctrine/Annotation/TokenTest.php
index 0b94857c..0f544cb2 100644
--- a/tests/Doctrine/Annotation/TokenTest.php
+++ b/tests/Doctrine/Annotation/TokenTest.php
@@ -93,14 +93,14 @@ public function provideIsTypeCases()
];
}
/**
* @dataProvider provideIsNotTypeCases
*
- * @param int $type
* @param int|int[] $types
+ * @param int $type
*/
public function testIsTypeReturnsFalse($types, $type)
{
$token = new Token();
$token->setType($type);
diff --git a/tests/Fixer/ClassNotation/OrderedClassElementsFixerTest.php b/tests/Fixer/ClassNotation/OrderedClassElementsFixerTest.php
index a99d49ce..3652e649 100644
--- a/tests/Fixer/ClassNotation/OrderedClassElementsFixerTest.php
+++ b/tests/Fixer/ClassNotation/OrderedClassElementsFixerTest.php
@@ -327,15 +327,15 @@ protected function abc() {
EOT
],
];
}
/**
+ * @param array $configuration
* @param string $expected
* @param null|string $input
- * @param array $configuration
*
* @dataProvider provideFix71Cases
* @requires PHP 7.1
*/
public function testFix71(array $configuration, $expected, $input = null)
{
diff --git a/tests/Fixer/ControlStructure/NoUselessElseFixerTest.php b/tests/Fixer/ControlStructure/NoUselessElseFixerTest.php
index 0c817133..b9ef9a22 100644
--- a/tests/Fixer/ControlStructure/NoUselessElseFixerTest.php
+++ b/tests/Fixer/ControlStructure/NoUselessElseFixerTest.php
@@ -773,14 +773,14 @@ public function provideConditionsWithoutBracesCases()
];
return $cases;
}
/**
- * @param string $input
* @param string<int, bool> $indexes
+ * @param string $input
*
* @dataProvider provideIsInConditionWithoutBracesCases
*/
public function testIsInConditionWithoutBraces($indexes, $input)
{
$reflection = new \ReflectionObject($this->fixer);
diff --git a/tests/RuleSetTest.php b/tests/RuleSetTest.php
index eb357dbd..42e02269 100644
--- a/tests/RuleSetTest.php
+++ b/tests/RuleSetTest.php
@@ -33,14 +33,14 @@ public function testCreate()
$ruleSet = RuleSet::create();
$this->assertInstanceOf(\PhpCsFixer\RuleSet::class, $ruleSet);
}
/**
- * @param string $ruleName
* @param string $setName
+ * @param string $ruleName
* @param array|bool $ruleConfig
*
* @dataProvider provideAllRulesFromSetsCases
*/
public function testIfAllRulesInSetsExists($setName, $ruleName, $ruleConfig)
{
@@ -67,14 +67,14 @@ public function testIfAllRulesInSetsExists($setName, $ruleName, $ruleConfig)
} catch (InvalidForEnvFixerConfigurationException $exception) {
// ignore
}
}
/**
- * @param string $ruleName
* @param string $setName
+ * @param string $ruleName
*
* @dataProvider provideAllRulesFromSetsCases
*/
public function testThatThereIsNoDeprecatedFixerInRuleSet($setName, $ruleName)
{
$factory = new FixerFactory();
diff --git a/tests/Tokenizer/Analyzer/CommentsAnalyzerTest.php b/tests/Tokenizer/Analyzer/CommentsAnalyzerTest.php
index c19f5ead..aee24976 100644
--- a/tests/Tokenizer/Analyzer/CommentsAnalyzerTest.php
+++ b/tests/Tokenizer/Analyzer/CommentsAnalyzerTest.php
@@ -181,14 +181,14 @@ public function testPhpdocCandidateAcceptsOnlyComments()
$this->expectException(\InvalidArgumentException::class);
$analyzer->isBeforeStructuralElement($tokens, 2);
}
/**
- * @param bool $isPhpdocCandidate
* @param string $code
+ * @param bool $isPhpdocCandidate
*
* @dataProvider providePhpdocCandidateCases
*/
public function testPhpdocCandidate($code)
{
$tokens = Tokens::fromCode($code);
@@ -233,14 +233,14 @@ public function providePhpdocCandidateCases()
['<?php /* @var string $s */ print($s);'],
['<?php /* @var string $s */ echo($s);'],
];
}
/**
- * @param bool $isPhpdocCandidate
* @param string $code
+ * @param bool $isPhpdocCandidate
*
* @dataProvider provideNotPhpdocCandidateCases
*/
public function testNotPhpdocCandidate($code)
{
$tokens = Tokens::fromCode($code);
diff --git a/tests/Tokenizer/TokensTest.php b/tests/Tokenizer/TokensTest.php
index 3456c918..85ca4256 100644
--- a/tests/Tokenizer/TokensTest.php
+++ b/tests/Tokenizer/TokensTest.php
@@ -511,14 +511,14 @@ public function bar()
$this->assertCount(1, $found[T_FUNCTION]);
$this->assertArrayHasKey(9, $found[T_FUNCTION]);
}
/**
* @param string $source
- * @param Token[] $expected tokens
* @param int[] $indexes to clear
+ * @param Token[] $expected tokens
*
* @dataProvider provideGetClearTokenAndMergeSurroundingWhitespaceCases
*/
public function testClearTokenAndMergeSurroundingWhitespace($source, array $indexes, array $expected)
{
$this->doTestClearTokens($source, $indexes, $expected); |
kubawerlos
left a comment
There was a problem hiding this comment.
I'd vote for adding the fixer to our .php_cs.dist, but above that the fixers looks fine now.
|
Okay I added the fixer to |
|
In the opening of this PR a reference is made to another PR that has staled.
I'm not stating this fixer should add or remove I really like to have this fixer in :) PS. |
|
@SpacePossum I already added test cases for these two scenarios: public function testOnlyParamsUndocumented()
{
$expected = <<<'EOT'
<?php
class C {
/**
* @param $a
* @param $b
* @param $c
* @param $d
*/
public function m($a, $b, $c, $d, $e, $f) {}
}
EOT;
$input = <<<'EOT'
<?php
class C {
/**
* @param $a
* @param $c
* @param $d
* @param $b
*/
public function m($a, $b, $c, $d, $e, $f) {}
}
EOT;
$this->doTest($expected, $input);
}
public function testOnlyParamsSuperfluousAnnotations()
{
$expected = <<<'EOT'
<?php
class C {
/**
* @param $a
* @param $b
* @param $c
* @param $superfluous2
* @param $superfluous1
* @param $superfluous3
*/
public function m($a, $b, $c) {}
}
EOT;
$input = <<<'EOT'
<?php
class C {
/**
* @param $a
* @param $superfluous2
* @param $b
* @param $superfluous1
* @param $c
* @param $superfluous3
*/
public function m($a, $b, $c) {}
}
EOT;
$this->doTest($expected, $input);
}Superfluous parameters are listed in previous order after the valid @param annotations. As you said I also think that it's not the concern of this fixer to add or remove annotations. Other fixers should handle that if need be. |
| * | ||
| * @return array | ||
| */ | ||
| private function getFuncParameterTokens(Tokens $tokens, array $functionSequence) |
There was a problem hiding this comment.
maybe PhpCsFixer\Tokenizer\Analyzer\ArgumentsAnalyzer::getArguments can be used for this?
There was a problem hiding this comment.
I checked this out. Unfortunately this function works differently. It "returns start and end token indexes of arguments". So $c = null (note the leading space) leads to a single space as start and null as end. So in the following case you get null instead of $c:
function m(array &$a, callable $b, $c = null, $d) {}
// -> $paramNames =
Array
(
[8] => $a
[13] => $b
[18] => null
[25] => $d
)Therefore I would keep my function, since it seems to work fairly well.
| private function sortDocBlockParamAnnotations(Token $docBlockToken, array $funcParamTokens) | ||
| { | ||
| $doc = new DocBlock($docBlockToken->getContent()); | ||
| $paramAnotations = $doc->getAnnotationsOfType('param'); |
There was a problem hiding this comment.
it might be faster to first check if the PHPDoc has @params to start with and if not scanning the function signature for arguments as no fixing will be done anyway
(i.e. to do this check in the mean fix-loop above)
There was a problem hiding this comment.
Yes, that's better. Done.
|
I added all reqested changes @SpacePossum. What's next :)? |
|
Can you do |
|
Done |
|
@grubersjoe thanks for all the work :D
What is next is for some of the maintainers to find some time to test and review a bit more, |
|
Here's some test cases I'd like to see: Other annotations mixed in with paramsNon-public methods
(and a mixture of the above, in any valid order) Interfaces
Closures |
|
I've added tests for all above cases @ntzm. The check for a function signature has become less strict to support closures, but everything works as expected still. Also I had to adapt my previous approach of sorting the params to support the first category (other annotations mixed in between): Before, the parameter lines (and only those) were overwritten with the parameters in order. Therefore annotations in between would simply stay, where they had been. Now all annotations between the first and last param annotation line are sorted. Consequently, everything in between is listed in previous order immediately after the param block. The corresponding test case: $expected = <<<'EOT'
<?php
/**
* [c1] Method description
* [c2] over multiple lines
*
* @see Baz
*
* @param int $a Long param
* description
* @param mixed $b
* @param mixed $superflous1 With text
* @param int $superflous2
* @return array Long return
* description
* @throws Exception
* @throws FooException
*/
function foo($a, $b) {}
EOT;
$input = <<<'EOT'
<?php
/**
* [c1] Method description
* [c2] over multiple lines
*
* @see Baz
*
* @param mixed $b
* @param mixed $superflous1 With text
* @return array Long return
* description
* @param int $superflous2
* @throws Exception
* @param int $a Long param
* description
* @throws FooException
*/
function foo($a, $b) {}
EOT; |
|
I am happy with the current behaviour of this fixer, but I think some integration tests are needed to make sure it doesn't conflict with other fixers that add, remove or otherwise modify |
|
@grubersjoe @ntzm What should we do to merge this PR? |
|
Hey. Sorry for the super long delay. I'm going to rebase this onto the current master and add the desired integration tests. I think afterwards this might be mergable again... |
|
thanks @grubersjoe : D |
|
So, sorry for the delay - again. Real life came in between 🙈. I've rebased my branch and added the fixer to the integration test in Please let me know if more integration tests are needed or something else needs to be handled. Edit: Pipelines failing What's the meaning of this? It seems to me this is happening independently of my changes (the incomplete tests are not mine). |
|
Thanks for the updates :) Currently the CI has some troubles with the tooling itself and not some much with the code. I tested you new rule on the symfony master code base and it made one change (pretty clean code base it seems ;) ) interface CacheInterface
{
/**
- * @param callable(CacheItem):mixed $callback Should return the computed value for the given key/item
* @param float|null $beta A float that controls the likeliness of triggering early expiration.
* 0 disables it, INF forces immediate expiration.
* The default (or providing null) is implementation dependent but should
* typically be 1.0, which should provide optimal stampede protection.
+ * @param callable(CacheItem):mixed $callback Should return the computed value for the given key/item
*
* @return mixed The value corresponding to the provided key
*/
public function get(string $key, callable $callback, float $beta = null);(the doc is pretty odd though... maybe that should be changed in the symfony project itself? WDYT?) |
|
I've tried it myself with Symfony's master branch and I also got one change only, but it's correct... However, I enabled this fixer only. What ruleset did you use? /**
* Test the returned data when AccessDecisionManager is a TraceableAccessDecisionManager.
*
* @param string $strategy strategy returned by the AccessDecisionManager
- * @param array $voters voters returned by AccessDecisionManager
* @param array $decisionLog log of the votes and final decisions from AccessDecisionManager
+ * @param array $voters voters returned by AccessDecisionManager
* @param array $expectedVoterClasses expected voter classes returned by the collector
* @param array $expectedDecisionLog expected decision log returned by the collector
*
* @dataProvider providerCollectDecisionLog
*/
public function testCollectDecisionLog(string $strategy, array $decisionLog, array $voters, array $expectedVoterClasses, array $expectedDecisionLog): void |
on:
and symfony: |
|
Thank you very much @grubersjoe 🍻 |
|
Oh wow. I didn't think this would end up being merged after all this time but thank you for picking it up 😄👍. |
|
I try to clean the backlog and ensure that contributors' work is not wasted, I believe people can take advantage of many things that are waiting here 🙂. 60 left 😅. |
This PR addresses #3161. Another quite old PR is still open to fix the same issue, but seems inactive: #2385.
The fixer reorders
@paramannotations in the DocBlock according to the formal parameters of the function / method.Example:
to
Superfluous
@paramannotations are ignored and listed in previous order after the valid ones.Thoughts?