NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

Remove uneeded docblocks#25859

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

fabpot wants to merge1 commit intosymfony:2.7fromfabpot:phpdocs-removal

Closed

Remove uneeded docblocks#25859

fabpot wants to merge1 commit intosymfony:2.7fromfabpot:phpdocs-removal

Conversation

Copy link

Member

fabpot commentedJan 20, 2018

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.

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

Copy link

Contributor

TomasVotruba commentedJan 20, 2018•
edited
Loading

@fabpot I just pushed fix forintertag blank lines inSymplify v3.2.4

Let me know if any more use cases fail

fabpot force-pushed thephpdocs-removal branch from7f455f4 to53784eeCompare

January 20, 2018 19:58

fabpot commented

Jan 20, 2018

View reviewed changes

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

		* @param array $objectManager A configured object manager
		*
		* @throws\InvalidArgumentException
		* @throws InvalidArgumentException

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@TomasVotruba Not sure why it removed the blackshash here.

Copy link

Contributor

TomasVotrubaJan 20, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Fixed:deprecated-packages/symplify#602

Please tryv3.2.5

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@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

Copy link

Contributor

TomasVotrubaJan 21, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.neoncheckers:    -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

Copy link

Contributor

TomasVotrubaJan 21, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 :)

Copy link

Contributor

ostrolucky commentedJan 20, 2018

it also broke phdoc alignments

Copy link

Contributor

TomasVotruba commentedJan 20, 2018

@ostrolucky That's responsibility of another fixer.

Copy link

Contributor

ostrolucky commentedJan 20, 2018

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

Copy link

Contributor

TomasVotruba commentedJan 20, 2018•
edited
Loading

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 ThePhpdocIndentFixer fixer is run first. I'll fix the priorities to makeRemoveUselessDocBlockFixer run before it. Thanks 👍

fabpot force-pushed thephpdocs-removal branch from53784ee to3f375afCompare

January 21, 2018 14:31

fabpot commented

Jan 21, 2018

View reviewed changes

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

		*
		* @param DumperInterface $dumper
		* @param Cursor $parentCursor The cursor of the parent hash
		* @param array &$refs A map of all references discovered while dumping

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@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?

Copy link

Contributor

TomasVotrubaJan 21, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@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.

Copy link

MemberAuthor

fabpot commentedJan 21, 2018

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).

Copy link

Contributor

TomasVotruba commentedJan 21, 2018•
edited
Loading

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

nicolas-grekas added this to the2.7 milestone

Jan 21, 2018

TomasVotruba mentioned this pull request

Jan 21, 2018

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

Merged

chalasr reviewed

Jan 22, 2018

View reviewed changes

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

		*
		* @param Definition\|Alias\|object $service
		* @param array $options
		* @param Definition\|\Alias\|object $service

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

prepending\ toAlias is wrong

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

		* @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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

wrong\ onProcess, class is correctly imported

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

		*
		* @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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

same

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Fixed

src/Symfony/Component/Form/Button.php Outdated

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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@TomasVotruba Can you have a look at this issue?

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Fixed allong with&$value references.

I discovered one more error to resolve

xabbuh added Status: Needs Work and removed Status: Needs Review labels

Jan 22, 2018

Copy link

Contributor

TomasVotruba commentedJan 23, 2018•
edited
Loading

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-standardcheckers:    -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

Copy link

Member

chalasr commentedJan 23, 2018•
edited
Loading

@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

Copy link

Contributor

TomasVotruba commentedJan 23, 2018•
edited
Loading

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

Copy link

MemberAuthor

fabpot commentedJan 23, 2018

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

removed uneeded docblocks

abc13dc

fabpot force-pushed thephpdocs-removal branch from3f375af toabc13dcCompare

January 23, 2018 19:15

fabpot commented

Jan 23, 2018

View reviewed changes

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"

Copy link

MemberAuthor

fabpotJan 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

fabpot commented

Jan 23, 2018

View reviewed changes

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

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

Copy link

MemberAuthor

fabpotJan 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

TomasVotrubaFeb 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

fabpot commented

Jan 23, 2018

View reviewed changes

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

Copy link

MemberAuthor

fabpotJan 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

TomasVotrubaFeb 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

fabpot commented

Jan 23, 2018

View reviewed changes

src/Symfony/Component/HttpFoundation/Request.php

		*
		* @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.

Copy link

MemberAuthor

fabpotJan 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This one should not be removed.

Copy link

Contributor

TomasVotrubaFeb 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'll look on this one

Copy link

Contributor

TomasVotrubaFeb 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 commented

Jan 23, 2018

View reviewed changes

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

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

Copy link

MemberAuthor

fabpotJan 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

TomasVotrubaFeb 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

fabpot commentedFeb 7, 2018

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

Copy link

Contributor

TomasVotruba commentedFeb 8, 2018•
edited
Loading

@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.