feat(PhpdocToCommentFixer): Add option to handle return as valid docblock usage (#7401) #7402

f1amy · 2023-11-01T19:21:17Z

Fixes #7401
Supersedes #5109

coveralls · 2023-11-01T20:00:19Z

coverage: 94.785% (-0.004%) from 94.789%
when pulling 6f4ca1b on f1amy:fix-phpdoc_to_comment-before-return
into 07cfaab on PHP-CS-Fixer:master.

Wirone

@f1amy we had some conversation between maintainers, and here's the summary: we would like to introduce new config option for this fixer, e.g. allow_before_return_statement which should be set to false by default, to keep current behaviour intact, as we consider this as a feature request, not a bug fix. In Fixer v4 we will make it true by default (so you can leave such a comment there), because PHP's ecosystem supports it (like PHPStan, PHPStorm). Please introduce the config option, regenerate docs and provide test cases for both config values.

Wirone · 2023-11-11T23:10:06Z

@keradus @kubawerlos FYI it was already proposed in the past (#5109), so we need to close one, but which one? This is most recent and up-to-date, but @bendavies' contribution would be rejected unfairly IMHO 🤷.

f1amy · 2023-11-12T14:04:39Z

Added option allow_before_return_statement and converted this PR into a feature.
Added TODO to make option true by default in 4.0

keradus · 2023-11-13T13:53:05Z

@Wirone - We merge one that is ready, when it's ready, and we close the other one.

src/Fixer/Phpdoc/PhpdocToCommentFixer.php

tests/Tokenizer/Analyzer/CommentsAnalyzerTest.php

f1amy · 2023-11-20T20:49:01Z

Rebased latest changes, list.rst file removed as in master.

f1amy · 2023-12-01T06:42:28Z

I'm not sure if static code analysis fail is caused by this PR's code, should I just wait for new commits from master?

Wirone · 2023-12-01T10:49:17Z

@f1amy The error is not related to your changes, but I can't reproduce it locally, though 🤔. Created PR with newest PHPStan, but error remains. Will investigate it later.

mvorisek · 2023-12-09T19:38:35Z

I agree with #5109 (comment), such @var should be replaced with phpstan ignore. Once the ignore is no longer needed, phpstan will warn about it, and it can be removed. @var introduces only more LOC that cannot be checked.

f1amy · 2023-12-09T21:12:45Z

I agree with #5109 (comment), such @var should be replaced with phpstan ignore. Once the ignore is no longer needed, phpstan will warn about it, and it can be removed. @var introduces only more LOC that cannot be checked.

I agree that we should not need to correct types with @var, and phpstan ignore could be used instead. However in order to do that, we still need to add a phpdoc with @phpstan-ignore error.name above return for it to work.

f1amy · 2023-12-14T07:20:54Z

@Wirone could you please check the status of this PR?

It says that 1 change is requested, but I thought I fixed it already, is it just waiting for review?

Requires new review after changes.

kubawerlos · 2023-12-14T16:12:45Z

src/Tokenizer/Analyzer/CommentsAnalyzer.php

@@ -65,7 +65,7 @@ public function isHeaderComment(Tokens $tokens, int $index): bool
     *
     * @see https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc.md#3-definitions
     */
-    public function isBeforeStructuralElement(Tokens $tokens, int $index): bool
+    public function isBeforeStructuralElement(Tokens $tokens, int $index, bool $allowReturnStatement = false): bool


From here, the new parameter does not make sense - isBeforeStructuralElement is clear, parameter allowReturnStatement allows cheating. Let's not do that and limit allow_before_return_statement functionality to only PhpdocToCommentFixer.

Extracted into isBeforeReturn method

src/Fixer/Phpdoc/PhpdocToCommentFixer.php

- correct doc for method

- add tests - update docs

f1amy · 2023-12-14T17:55:56Z

@kubawerlos ready for review again!

kubawerlos · 2023-12-14T19:43:46Z

src/Fixer/Phpdoc/PhpdocToCommentFixer.php

@@ -102,6 +119,8 @@ public function configure(array $configuration = null): void
            static fn (string $tag): string => strtolower($tag),
            $this->configuration['ignored_tags']
        );
+
+        $this->allowBeforeReturnStatement = (bool) $this->configuration['allow_before_return_statement'];


Suggested change

$this->allowBeforeReturnStatement = (bool) $this->configuration['allow_before_return_statement'];

$this->allowBeforeReturnStatement = true === $this->configuration['allow_before_return_statement'];

We have no casting for configuration in other fixers, let's use comparing to true, or even true === $this->configuration['allow_before_return_statement'] directly in line 153.

kubawerlos · 2023-12-14T19:49:20Z

src/Tokenizer/Analyzer/CommentsAnalyzer.php

+            return false;
+        }
+
+        return $this->isValidReturnStatement($tokens, $nextIndex);


Suggested change

return $this->isValidReturnStatement($tokens, $nextIndex);

return $tokens[$nextIndex]->isGivenKind(T_RETURN);

No need for a separate function for such a simple check.

Agreed. fixed

kubawerlos · 2023-12-15T16:16:17Z

Thank you @f1amy 👍🏼

…lock usage (PHP-CS-Fixer#7401) (PHP-CS-Fixer#7402)

f1amy changed the title ~~Fix "Fixer breaks PhpDoc before return" #7401~~ fix(PhpdocToCommentFixer): Handle return as valid language construct (#7401) Nov 1, 2023

Wirone requested a review from kubawerlos November 1, 2023 20:19

kubawerlos requested review from keradus and removed request for kubawerlos November 9, 2023 15:16

Wirone previously requested changes Nov 11, 2023

View reviewed changes

f1amy changed the title ~~fix(PhpdocToCommentFixer): Handle return as valid language construct (#7401)~~ feature(PhpdocToCommentFixer): Add option to handle return as valid docblock usage (#7401) Nov 12, 2023

f1amy changed the title ~~feature(PhpdocToCommentFixer): Add option to handle return as valid docblock usage (#7401)~~ feat(PhpdocToCommentFixer): Add option to handle return as valid docblock usage (#7401) Nov 12, 2023

f1amy requested a review from Wirone November 12, 2023 13:54

keradus reviewed Nov 13, 2023

View reviewed changes

src/Fixer/Phpdoc/PhpdocToCommentFixer.php Outdated Show resolved Hide resolved

keradus reviewed Nov 13, 2023

View reviewed changes

tests/Tokenizer/Analyzer/CommentsAnalyzerTest.php Show resolved Hide resolved

f1amy requested a review from keradus November 13, 2023 17:55

Wirone mentioned this pull request Dec 1, 2023

DX: Update PHPStan to 1.10.46 #7486

Merged

kubawerlos requested changes Dec 14, 2023

View reviewed changes

f1amy added 6 commits December 14, 2023 21:44

Add failing test

de43607

Fix failed test

31af072

- correct doc for method

Add allow_before_return_statement option to rule phpdoc_to_comment

303d3d6

- add tests - update docs

Fix phpdoc_to_comment docs example

eba36ef

Code review fixes

eadfd6d

Extract checking for return statement into its own method

03d01d5

f1amy added 3 commits December 14, 2023 22:37

Fix PHPDoc casing

c517cc2

DRY

d3f114c

Update docs

06f0b10

f1amy requested a review from kubawerlos December 14, 2023 17:55

kubawerlos requested changes Dec 14, 2023

View reviewed changes

Code review fixes

5ef6b60

f1amy requested a review from kubawerlos December 15, 2023 15:40

kubawerlos approved these changes Dec 15, 2023

View reviewed changes

kubawerlos enabled auto-merge (squash) December 15, 2023 15:58

kubawerlos added 2 commits December 15, 2023 16:58

Merge branch 'master' into fix-phpdoc_to_comment-before-return

9b0b52b

Merge branch 'master' into fix-phpdoc_to_comment-before-return

6f4ca1b

kubawerlos merged commit dbb7816 into PHP-CS-Fixer:master Dec 15, 2023
26 checks passed

keradus mentioned this pull request Dec 24, 2023

allow @var phpdocs before return statements #5109

Closed

danog pushed a commit to zoonru/PHP-CS-Fixer that referenced this pull request Feb 2, 2024

feat(PhpdocToCommentFixer): Add option to handle return as valid docb…

4a9c28f

…lock usage (PHP-CS-Fixer#7401) (PHP-CS-Fixer#7402)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(PhpdocToCommentFixer): Add option to handle return as valid docblock usage (#7401) #7402

feat(PhpdocToCommentFixer): Add option to handle return as valid docblock usage (#7401) #7402

f1amy commented Nov 1, 2023 •

edited

coveralls commented Nov 1, 2023 •

edited

Wirone left a comment

Wirone commented Nov 11, 2023

f1amy commented Nov 12, 2023

keradus commented Nov 13, 2023

f1amy commented Nov 20, 2023

f1amy commented Dec 1, 2023

Wirone commented Dec 1, 2023

mvorisek commented Dec 9, 2023

f1amy commented Dec 9, 2023

f1amy commented Dec 14, 2023

kubawerlos Dec 14, 2023

f1amy Dec 14, 2023

f1amy commented Dec 14, 2023

kubawerlos Dec 14, 2023

f1amy Dec 15, 2023

kubawerlos Dec 14, 2023

f1amy Dec 15, 2023

kubawerlos commented Dec 15, 2023

	$this->allowBeforeReturnStatement = (bool) $this->configuration['allow_before_return_statement'];
	$this->allowBeforeReturnStatement = true === $this->configuration['allow_before_return_statement'];

	return $this->isValidReturnStatement($tokens, $nextIndex);
	return $tokens[$nextIndex]->isGivenKind(T_RETURN);

feat(PhpdocToCommentFixer): Add option to handle return as valid docblock usage (#7401) #7402

feat(PhpdocToCommentFixer): Add option to handle return as valid docblock usage (#7401) #7402

Conversation

f1amy commented Nov 1, 2023 • edited

coveralls commented Nov 1, 2023 • edited

Wirone left a comment

Choose a reason for hiding this comment

Wirone commented Nov 11, 2023

f1amy commented Nov 12, 2023

keradus commented Nov 13, 2023

f1amy commented Nov 20, 2023

f1amy commented Dec 1, 2023

Wirone commented Dec 1, 2023

mvorisek commented Dec 9, 2023

f1amy commented Dec 9, 2023

f1amy commented Dec 14, 2023

kubawerlos Dec 14, 2023

Choose a reason for hiding this comment

f1amy Dec 14, 2023

Choose a reason for hiding this comment

f1amy commented Dec 14, 2023

kubawerlos Dec 14, 2023

Choose a reason for hiding this comment

f1amy Dec 15, 2023

Choose a reason for hiding this comment

kubawerlos Dec 14, 2023

Choose a reason for hiding this comment

f1amy Dec 15, 2023

Choose a reason for hiding this comment

kubawerlos commented Dec 15, 2023

f1amy commented Nov 1, 2023 •

edited

coveralls commented Nov 1, 2023 •

edited