Remove uneeded docblocks #25859

fabpot · 2018-01-20T11:06:08Z

Q	A
Branch?	2.7
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	n/a

Big cleanup (replaces #24931).

Thanks @TomasVotruba for doing all the hard work :)

Before merging, blanck lines between @param blocks and @throws/@return/ ... should be kept.

TomasVotruba · 2018-01-20T16:48:54Z

@fabpot I just pushed fix for intertag blank lines in Symplify v3.2.4

Let me know if any more use cases fail

fabpot · 2018-01-20T19:59:17Z

src/Symfony/Bridge/Doctrine/DependencyInjection/AbstractDoctrineExtension.php

     *
-     * @throws \InvalidArgumentException
+     * @throws InvalidArgumentException


@TomasVotruba Not sure why it removed the blackshash here.

Unfortunately, this is also default behavior of ReflectionDocBlock. I'll look on that.

Fixed: deprecated-packages/symplify#602

Please try v3.2.5

@TomasVotruba Running the new version give me this error:

Fixing of "src/Symfony/Component/HttpFoundation/Request.php" file by "Symplify\CodingStandard\Fixer\Commenting\RemoveUselessDocBlockFixer" failed: Unknown modifier 'c' in pattern: #@see[\s]+(?<has_ slash>\\)?https\://tools\.ietf\.org/html/rfc7231#section\-4\.2\.1# in file /Users/fabien/Code/github/Symplify/EasyCodingStandard/vendor/nette/utils/src/Utils/Strings.php on line 604

Fixed in v3.2.6 and run on symfony/symfony 2.7 myself with success

Here is preview PR: 2.7...TomasVotruba:phpdocs-removal

with this ECS setup:

# ecs.neon checkers: - PhpCsFixer\Fixer\Phpdoc\PhpdocAlignFixer - Symplify\CodingStandard\Fixer\Commenting\RemoveUselessDocBlockFixer # remove extra blank lines caused by previous fixer - Symplify\CodingStandard\Fixer\Commenting\RemoveEmptyDocBlockFixer - Symplify\CodingStandard\Fixer\Commenting\RemoveSuperfluousDocBlockWhitespaceFixer

I see there are few edge case. I'll try to fix some, but some are very difficult to catch with provided ReflectionDocBlock api. I'd have to write my own package to be able to resolve it correctly :)

ostrolucky · 2018-01-20T20:04:57Z

it also broke phdoc alignments

TomasVotruba · 2018-01-20T20:46:47Z

@ostrolucky That's responsibility of another fixer.

ostrolucky · 2018-01-20T20:50:56Z

Right. But there might be issue with set priorities. Anyway I'm not blaming your fixer, but pointing to an issue in this PR.

TomasVotruba · 2018-01-20T20:56:16Z

I see. I think @fabpot didn't run it with the other fixer.

@fabpot I recommend running this first, than "@symfony" set from PHP CS Fixer, that's how I use it in practise just in one ECS run. That would prevent any other fails that are covered by fixers.

@ostrolucky The PhpdocIndentFixer fixer is run first. I'll fix the priorities to make RemoveUselessDocBlockFixer run before it. Thanks 👍

fabpot · 2018-01-21T14:39:08Z

src/Symfony/Component/VarDumper/Cloner/Data.php

@@ -210,7 +209,6 @@ private function dumpItem($dumper, $cursor, &$refs, $item)
     *
     * @param DumperInterface $dumper
     * @param Cursor          $parentCursor The cursor of the parent hash
-     * @param array           &$refs        A map of all references discovered while dumping


@TomasVotruba Checking the diff, I think the current strategy is too aggressive. Here is one such example where the comment seems interesting and should probably be kept. WDYT?

@fabpot Thanks for comment. I've noticed this already. I think it's caused by & which probably leads parsing error and to empty comment.

I'm adding it to the cases to be fixed.

fabpot · 2018-01-21T14:39:40Z

This PR is now ready to be reviewed. I've added a comment on one example where the comment should probably be kept instead of removed (as the description adds some value).

TomasVotruba · 2018-01-21T14:49:51Z

There is 1 last issue reported in fabpot.io: https://fabbot.io/report/symfony/symfony/25859/3f375af281292adbdffb83ae5a6860bc45b393ea

What fixer takes care of that? I'll add it to my comment to complete the setup

chalasr · 2018-01-22T11:54:44Z

src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/Descriptor.php

@@ -144,8 +143,7 @@ protected function renderTable(Table $table, $decorated = false)
     * Common options are:
     * * name: name of described service
     *
-     * @param Definition|Alias|object $service
-     * @param array                   $options
+     * @param Definition|\Alias|object $service


prepending \ to Alias is wrong

Fixed: master...TomasVotruba:phpdos-removal-master#diff-23068eac61c835540fd5aaf1edbd5cd7L129

chalasr · 2018-01-22T12:02:39Z

src/Symfony/Component/Console/Helper/ProcessHelper.php

-     * @param callable|null        $callback  A PHP callback to run whenever there is some
-     *                                        output available on STDOUT or STDERR
-     * @param int                  $verbosity The threshold for verbosity
+     * @param string|array|\Process $cmd       An instance of Process or an array of arguments to escape and run or a command to run


wrong \ on Process, class is correctly imported

Fixed: master...TomasVotruba:phpdos-removal-master#diff-425e56e3b866cb257f587805f7b2ee54R30

chalasr · 2018-01-22T12:02:47Z

src/Symfony/Component/Console/Helper/ProcessHelper.php

@@ -80,8 +79,7 @@ public function run(OutputInterface $output, $cmd, $error = null, $callback = nu
     * This is identical to run() except that an exception is thrown if the process
     * exits with a non-zero exit code.
     *
-     * @param OutputInterface $output   An OutputInterface instance
-     * @param string|Process  $cmd      An instance of Process or a command to run
+     * @param string|\Process $cmd      An instance of Process or a command to run


chalasr · 2018-01-22T12:03:50Z

src/Symfony/Component/Form/Button.php

-     * @param int|string|FormInterface $child
-     * @param null                     $type
-     * @param array                    $options
+     * @param int|string|\FormInterface $child


wrong \, I stop here :) the fixer seems to consider that classes are all coming from the global namespace

@TomasVotruba Can you have a look at this issue?

Fixed allong with &$value references.

I discovered one more error to resolve

TomasVotruba · 2018-01-23T13:51:28Z

Last version to check: master...TomasVotruba:phpdos-removal-master

If you comment there, I'll try to fix that asap. Don't mind the indent, just docs removal.

Used set:

# easy-coding-standard
checkers:
    - Symplify\CodingStandard\Fixer\Commenting\RemoveUselessDocBlockFixer
    - PhpCsFixer\Fixer\Phpdoc\PhpdocIndentFixer
    - PhpCsFixer\Fixer\Phpdoc\PhpdocAlignFixer

    # works best with these checkers, to remove empty docblock
    - Symplify\CodingStandard\Fixer\Commenting\RemoveSuperfluousDocBlockWhitespaceFixer
    - Symplify\CodingStandard\Fixer\Commenting\RemoveEmptyDocBlockFixer

chalasr · 2018-01-23T13:57:52Z

@TomasVotruba Some annotations are replaced by an extra blank line, sometimes an extra blank line is just added between the phpdoc description and the first @param annotation:
b2d0a5b#diff-a10c9afd8feaccd1ff66ffd54e2835bcR925

TomasVotruba · 2018-01-23T17:47:45Z

@chalasr Oh, I only used the remover fixer. But I've added space cleaner as well and updated the code. Ready for review 👍

fabpot · 2018-01-23T19:13:14Z

I've just updated this PR with the new code (thanks @TomasVotruba)

fabpot · 2018-01-23T19:19:13Z

src/Symfony/Component/Config/Util/XmlUtils.php

-     *  * Each tag is converted to a key value or an array
-     *    if there is more than one "value"
+     * * Each tag is converted to a key value or an array
+     *   if there is more than one "value"


Not sure about this change. The space is needed to render proper Markdown.

fabpot · 2018-01-23T19:21:03Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

-     *             Use \Symfony\Component\DependencyInjection\ContainerBuilder::addExpressionLanguageProvider instead.
-     *
-     * @param ExpressionFunctionProviderInterface $provider
+     * Use \Symfony\Component\DependencyInjection\ContainerBuilder::addExpressionLanguageProvider instead.


I think the previous version is more readable and as it renders the same, I would not change this.

This is something I can't cover easily. I recommend reverting those manually.

fabpot · 2018-01-23T19:22:27Z

src/Symfony/Component/Finder/Tests/Iterator/IteratorTestCase.php

-     *  @param \Traversable $iterator
+     * $a and $b such that $a goes before $b in $expected, the method
+     * asserts that any element of $a goes before any element of $b
+     * in the sequence generated by $iterator


And here, I'm pretty sure that the indentation is wrong as this describes the $expected param.

This is something I can't cover easily. I recommend reverting those manually.

fabpot · 2018-01-23T19:23:25Z

src/Symfony/Component/HttpFoundation/Request.php

@@ -1479,8 +1477,6 @@ public function isMethod($method)
     *
     * @see https://tools.ietf.org/html/rfc7231#section-4.2.1
     *
-     * @param bool $andCacheable Adds the additional condition that the method should be cacheable. True by default.


This one should not be removed.

I'll look on this one

Oh, I see it's commented bellow. This Fixer is not able to differentiate between commented argument and extra useless docs for param.

Just revert this manually

fabpot · 2018-01-23T19:25:23Z

src/Symfony/Component/Routing/Matcher/Dumper/DumperCollection.php

@@ -47,8 +47,6 @@ public function all()

    /**
     * Adds a route or collection.
-     *
-     * @param DumperRoute|DumperCollection The route or collection


Cannot be removed, I think it was removed as it's not valid. $child is missing here.

Yes, this is better to fix manually, since this Fixer relies on valid code.

fabpot · 2018-02-07T05:25:46Z

@TomasVotruba Have you had time to have a look at the issues I've noticed here?

TomasVotruba · 2018-02-08T14:30:31Z

@fabpot I missed your comments here, sorry. I'm looking into it.
Those are edge cases, that Fixer is unable to handle propperly without failing somewhere else.
Just revert manually.

nicolas-grekas · 2018-03-19T17:25:10Z

@TomasVotruba would you like to take over doing the manual tweaks? Otherwise I'd suggest to close.

TomasVotruba · 2018-03-19T19:06:06Z

@nicolas-grekas Sure. I can get to in till Friday.

TomasVotruba · 2018-12-02T19:47:11Z

@nicolas-grekas I'll try to re-send this. Which branch should I target now?

nicolas-grekas · 2018-12-02T20:13:07Z

3.4 is the oldest maintained branch now.

TomasVotruba · 2018-12-02T20:53:59Z

@nicolas-grekas Ok

carsonbot added the Status: Needs Review label Jan 20, 2018

fabpot mentioned this pull request Jan 20, 2018

[cs] remove unused doc blocks #24931

Closed

fabpot force-pushed the phpdocs-removal branch from 7f455f4 to 53784ee Compare January 20, 2018 19:58

fabpot commented Jan 20, 2018

View reviewed changes

fabpot force-pushed the phpdocs-removal branch from 53784ee to 3f375af Compare January 21, 2018 14:31

fabpot commented Jan 21, 2018

View reviewed changes

nicolas-grekas added this to the 2.7 milestone Jan 21, 2018

TomasVotruba mentioned this pull request Jan 21, 2018

[CodingStandard] fixing few edge cases based on Symfony and RemoveUselessDocBlockFixer deprecated-packages/symplify#603

Merged

chalasr reviewed Jan 22, 2018

View reviewed changes

xabbuh added Status: Needs Work and removed Status: Needs Review labels Jan 22, 2018

removed uneeded docblocks

abc13dc

fabpot force-pushed the phpdocs-removal branch from 3f375af to abc13dc Compare January 23, 2018 19:15

fabpot commented Jan 23, 2018

View reviewed changes

chalasr mentioned this pull request Feb 23, 2018

[FrameworkBundle] keep query in redirect #26281

Merged

fabpot closed this Mar 29, 2018

ogizanagi mentioned this pull request Dec 8, 2018

[HackDay][Messanger]Improve PHP DocBlock for Messanger Stamps #29529

Closed

Remove uneeded docblocks #25859

Remove uneeded docblocks #25859

Conversation

fabpot commented Jan 20, 2018

TomasVotruba commented Jan 20, 2018 • edited

Choose a reason for hiding this comment

TomasVotruba Jan 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasVotruba Jan 21, 2018 • edited

Choose a reason for hiding this comment

TomasVotruba Jan 21, 2018 • edited

Choose a reason for hiding this comment

ostrolucky commented Jan 20, 2018

TomasVotruba commented Jan 20, 2018

ostrolucky commented Jan 20, 2018

TomasVotruba commented Jan 20, 2018 • edited

Choose a reason for hiding this comment

TomasVotruba Jan 21, 2018 • edited

Choose a reason for hiding this comment

fabpot commented Jan 21, 2018

TomasVotruba commented Jan 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasVotruba commented Jan 23, 2018 • edited

chalasr commented Jan 23, 2018 • edited

TomasVotruba commented Jan 23, 2018 • edited

fabpot commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Feb 7, 2018

TomasVotruba commented Feb 8, 2018 • edited

nicolas-grekas commented Mar 19, 2018

TomasVotruba commented Mar 19, 2018

TomasVotruba commented Dec 2, 2018

nicolas-grekas commented Dec 2, 2018

TomasVotruba commented Dec 2, 2018 • edited

TomasVotruba commented Jan 20, 2018 •

edited

TomasVotruba Jan 20, 2018 •

edited

TomasVotruba Jan 21, 2018 •

edited

TomasVotruba Jan 21, 2018 •

edited

TomasVotruba commented Jan 20, 2018 •

edited

TomasVotruba Jan 21, 2018 •

edited

TomasVotruba commented Jan 21, 2018 •

edited

TomasVotruba commented Jan 23, 2018 •

edited

chalasr commented Jan 23, 2018 •

edited

TomasVotruba commented Jan 23, 2018 •

edited

TomasVotruba commented Feb 8, 2018 •

edited

TomasVotruba commented Dec 2, 2018 •

edited