Remove "no_superfluous_phpdoc_tags" rule from Php Cs Fixer ruleset

[pavog]

Hallo everyone!

I would like to contribute to this repository by improving the code quality in order to increase the level of PHPStan validation.
To do this, I need to add many doc blocks to functions used in the src and in the tests.

I started by adding doc blocks to test functions and used the automatic generation of doc blocks in the PHPStorm IDE.
For example, I added the doc block to testGetRequired in PackageSelectionTest and changed it from:

/**
 * @dataProvider dataGetRequired
 */
public function testGetRequired(array $expected, Package $package, bool $requireDependencies, bool $requireDevDependencies): void

to

/**
 * @dataProvider dataGetRequired
 *
 * @param Link[] $expected
 * @param Package $package
 * @param bool $requireDependencies
 * @param bool $requireDevDependencies
 *
 * @throws \ReflectionException
 */
public function testGetRequired(array $expected, Package $package, bool $requireDependencies, bool $requireDevDependencies): void

After running the php cs fixer checks, it wanted me to remove three of the parameters because the no_superfluous_phpdoc_tags rule told me that they do not provide any useful information.

That's true because:

• @param Link[] $expected Provides the information that array $expected (typed parameter) is actually Link[] $expected
• @param Package $package Information is already typed in the parameters (so in the function signature)
• @param bool $requireDependencies Information is already typed in the parameters (so in the function signature)
• @param bool $requireDevDependencies Information is already typed in the parameters (so in the function signature)

But in my opinion, it is not the best idea to describe a function by mentioning only one of the four parameters in its doc block.

• It then looks like I forgot to mention the other three.
• And when I open the short doc (ctrl + q in PhpStorm), it only shows one parameter in the list of parameters. This could confuse other developers if they rely on the short documentation and don't look at the actual function signature.

---

This is how it looks in the short doc of PhpStorm:

---

Therefore, I would like to suggest that we remove the rule from the php cs fixer rulebook.
What do you think about this?

[alcohol]

Fine by me. I have no arguments against removing this rule. I merely wish for contributors to adhere to whichever standards the community prefers.

[pavog]

I opened #680

[SvenRtbg]

My argument against removing this rule: There should be exactly one source of authoritative type info, not two. If there are strict types in the method's signature, any additional annotations stating the same info are redundant, and may be at worst deviate from the code. So in essence if the signature states string and the annotation somehow states string|null, which info is correct? Did someone accidentally remove the null case from the signature, or leave in the null case that was removed?

I prefer the PHPDocs to be as short as possible, and the only remaining case for having @param or @return is when the code returns an array in the form of a makeshift collection - this is the only place where it adds benefit by explaining the contents of the array.

[SvenRtbg]

An additional argument would be that Symfony removes these annotations in their code style, and Satis is using Symfony components itself. It would make for a more similar style.

[pavog]

Hi @SvenRtbg,

I understand your arguments and can also understand your point of view.

However, I think that we should either:

• do not describe any parameters at all in the docblocks
• describe all parameters in the docblocks

And do not mix and then describe only some, but not necessarily all parameters.
I find this inconsistent and also a bad/weird style.

---

For the two cases you mentioned:

1. If I have @param string|null $outputDir in the doc block and string $outputDir in the function signature: Phpstan checks for this. This is the result: phpstan: PHPDoc tag @param for parameter $outputDir with type string|null is not a subtype of native type string.

2. If I have @param string $outputDir in the doc block and ?string $outputDir in the function signature.: Unfortunately, we don't have checks to prevent this scenario.

Maybe there is a rule for the php cs fixer that we don't have enabled, or there might be a check for it in phpstan soon. My IDE (phpstorm) checks this and warns me. But you are right: We need to prevent this, and we can't do without a check of phpstan or php cs fixer running in the ci tests.

---

To take the discussion a bit further:
What do you think is the best way to increase phpstan checking (to solve #658) but keep the no_superfluous_phpdoc_tags rule?

• A) Keep the doc block short and simple and only add the parameters that phpstan says must be added.
• B) Add as many parameters as possible to the documentation blocks and add descriptions so that they provide useful information.
• C) ?

[SvenRtbg]

Independently from any tools (because they may not be available to check for obvious or non-obvious errors) I prefer to keep the amount of PHPDoc as close to zero as possible. We are dealing with the PHP legacy here that had nothing better to offer until 7.0. And I have encountered my fair share of code that simply lies in the PHPDoc block without any consequences during execution time, but giving developers a hard time when actually relying on these info in their IDE when dealing with other code. And I think we agree that code should be as correct as possible, otherwise you wouldn't have started to care about PHPStan results. So thank you for that!

Addressing your questions: If we have the least amount of PHPDoc possible, there wouldn't be any problems in cases 1 and 2 above. And I would definitely opt for solution A.

My counterargument for solution B, misinterpreting it to the fullest extent maybe, is that it opens the door to overwhelm the docblock with information that could be considered useless at best, and misleading at worst. This is, from my POV the discussion about useless documentation in code: Comments seem useful at first, but by not getting updated during code changes they turn into being confusing, misleading or they simply lie about the current code. So regardless of any tools, the code should be using as few comments as possible anyways. So I oppose B.

As I mentioned before, sometimes PHPDocs are still necessary to explain the contents of arrays, because PHP is lacking any support for Generics. And that's fine, because the discussion about them has at least one suggestion to just allow annotating them in the code, but without any runtime checks, so it would be syntactic sugar only.

So assuming that you are referring to the "has no value type specified in iterable type array" results that I see when running PHPStan, there currently is no other way but to add this info into the docblock.

And if you feel the only way to add this is by using the PHPDoc creation feature of PHPStorm, go ahead and use that. You should be able to run the PHP-CS-Fixer afterwards, which will remove any superfluous PHPDoc tags - note that they are only considered if they are identical to what the type declaration in the code says. So if both agree to string, the PHPDoc annotation gets removed by that rule. If you have any kind of ObjectsInArray[] annotation with type array in the code, the annotation will be kept.

[alcohol]

using the PHPDoc creation feature of PHPStorm

In regards to PHPStorm, I think his main issue is that it renders the "help" or "api" information inadequately because it doesn't merge the docblock and native type hints.

Only docblock params are rendered as "parameters" (see second image).

Compare

vs

[SvenRtbg]

Interesting observation. If you'd have entered some description behind $payload, it would also show in the mouseover window. And that's possibly the only case where there is a difference, because you cannot explain a parameter in the code only.

And in fact the superfluous rule does not remove tags that have a description, it is only removing tags that are identical to the code without any additional information.

[alcohol]

Yes, I also noticed that it will gladly accepted param dockblocks for non existant arguments. E.g. for this same method I made phpstorm believe it accepted 3 parameters and it all rendered them as parameters in the popup. I reckon letting the code be leading and only using docblock params where the code cannot adequately convey the real content of the arguments/output is the safest choice.

[SvenRtbg]

func_get_args() still is a thing... but not in this code base.