[POC] Add PHP 7.1 typehints + remove unused doc for @param and @return, #24930
Conversation
|
Not sure how error on PHP 7.2 is related to this: https://travis-ci.org/symfony/symfony/jobs/300794715#L2491 Other 2 builds are passing Any ideas? |
|
Last few commits are mostly experimental to show what can be automated by coding standard tools when set up propperly. Let me know what you prefer. |
| @@ -167,7 +167,7 @@ protected function getMappingDriverBundleConfigDefaults(array $bundleConfig, \Re | ||
| /** | ||
| * Register all the collected mapping information with the object manager by registering the appropriate mapping drivers. | ||
| * | ||
| * @param ContainerBuilder $container A ContainerBuilder instance | ||
| * @param ContainerBuilder $container A ContainerBuilder instance |
There was a problem hiding this comment.
this one can be completely removed in fact :)
There was a problem hiding this comment.
Let's automate it :)
Remove all messages in format:
A "type" instance
What do you think?
| @@ -79,7 +79,6 @@ public function getListeners(string $event = null): array | ||
| /** | ||
| * Checks whether an event has any registered listeners. | ||
| * | ||
| * @param string $event |
There was a problem hiding this comment.
this commit is interesting and related rule could be always enabled
but of course it should not build on the previous commits, which are BC breaking changes
There was a problem hiding this comment.
Sure, just add Symplify\CodingStandard\Fixer\Commenting\RemoveUselessDocBlockFixer
| @@ -46,7 +46,7 @@ public function __construct(ContainerInterface $container) | ||
| * | ||
| * @return bool | ||
| */ | ||
| public function dispatchEvent($eventName, EventArgs $eventArgs = null) | ||
| public function dispatchEvent(string $eventName, EventArgs $eventArgs = null) |
There was a problem hiding this comment.
this is interesting, unfortunately we won't be able to enable this rule, as this we must keep BC and we'll still maintain 5.5 code for some years to come (3.4 being merged into master)
There was a problem hiding this comment.
I see, this is resolved by external Sniff, but I can make it ignore this file.
| @@ -44,7 +44,7 @@ public function isOptional() | ||
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function warmUp($cacheDir) | ||
| public function warmUp($cacheDir): void |
There was a problem hiding this comment.
I personally don't like "void": very low value (we don't add @return void for the same reason IMHO)
BC breaking change of course
and only my POV, so up for discussion of course :)
There was a problem hiding this comment.
in general 👍 for void; we imply mixed otherwise. So in terms of closing the gates, and to follow @stof 's comment, i agree with;
we should indeed add types whenever we can
so we can safely assume no type means mixed. But im not sure https://wiki.php.net/rfc/mixed-typehint changes our game.
Also <phhp7.2 we should keep @param/return object, as those can be replaced with typehints in 7.2 and we dont wanna imply mixed either.
There was a problem hiding this comment.
The "void" return type will help PHP to execute code faster. PHP 7.2 will have better code optimizer. Any type hint will reduce the amount of time to execute code since no need to detect type.
There was a problem hiding this comment.
Source? As far as I know, type hinting in PHP has negative performance impact. The aspect I personally don't like about void return type hint is that it will prevent subclasses from making fluent interfaces. Other than that, sure, it makes code more clear.
There was a problem hiding this comment.
| @@ -44,7 +44,7 @@ public function __construct(ManagerRegistry $registry) | ||
| /** | ||
| * Adds the stack logger for a connection. | ||
| * | ||
| * @param string $name | ||
| * @param string $name |
There was a problem hiding this comment.
php-cs-fixer already has a similar rule, isn't it?
| @@ -45,7 +45,6 @@ public function __construct(ManagerRegistry $registry) | ||
| * Adds the stack logger for a connection. | ||
| * | ||
| * @param string $name | ||
| * @param DebugStack $logger |
|
Some random comments added on a commit per commit basis. |
Thank you. I'll integrate them to the tool if possible.
@nicolas-grekas Well, anyone can port these two classes. But it requires PHP_CodeSniffer and like 20 related classes, so it makes sense me to not reinvent the wheel and use directly EasyCodingStandard.
I'll prepare smaller PRs, maybe per package? |
|
@TomasVotruba not per package, we always prefer per-topic and repo-wide; |
|
Closing because this was a poc. |
Follow up to #24722
@nicolas-grekas Let me know how this works for you.