Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[POC] Add PHP 7.1 typehints + remove unused doc for @param and @return,#24930
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
TomasVotruba commentedNov 12, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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? |
TomasVotruba commentedNov 12, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. |
| * Register all the collected mapping information with the object manager by registering the appropriate mapping drivers. | ||
| * | ||
| * @param ContainerBuilder $containerA ContainerBuilder instance | ||
| * @param ContainerBuilder $container A ContainerBuilder instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this one can be completely removed in fact :)
TomasVotrubaNov 12, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's automate it :)
Remove all messages in format:
A "type" instance
What do you think?
| /** | ||
| * Checks whether an event has any registered listeners. | ||
| * | ||
| * @param string $event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure, just addSymplify\CodingStandard\Fixer\Commenting\RemoveUselessDocBlockFixer
| * @return bool | ||
| */ | ||
| publicfunctiondispatchEvent($eventName,EventArgs$eventArgs =null) | ||
| publicfunctiondispatchEvent(string$eventName,EventArgs$eventArgs =null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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)
TomasVotrubaNov 12, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I see, this is resolved by externalSniff, but I can make it ignore this file.
| * {@inheritdoc} | ||
| */ | ||
| publicfunctionwarmUp($cacheDir) | ||
| publicfunctionwarmUp($cacheDir):void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
in general 👍 for void; we imply mixed otherwise. So in terms of closing the gates, and to follow@stof 'scomment, i agree with;
we should indeed add types whenever we can
so we can safely assume no type means mixed. But im not surehttps://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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| * Adds the stack logger for a connection. | ||
| * | ||
| * @param string$name | ||
| * @param string $name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
php-cs-fixer already has a similar rule, isn't it?
| * Adds the stack logger for a connection. | ||
| * | ||
| * @param string $name | ||
| * @param DebugStack $logger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I like this rule :)
nicolas-grekas commentedNov 12, 2017
Some random comments added on a commit per commit basis. |
TomasVotruba commentedNov 12, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thank you. I'll integrate them to the tool if possible.
@nicolas-grekas Well, anyone can port thesetwoclasses. But it requires PHP_CodeSniffer and like 20 related classes, so it makes sense me to not reinvent the wheel and use directlyEasyCodingStandard.
I'll prepare smaller PRs, maybe per package? |
nicolas-grekas commentedNov 12, 2017
@TomasVotruba not per package, we always prefer per-topic and repo-wide; |
nicolas-grekas commentedNov 19, 2017
Closing because this was a poc. |
Follow up to#24722
@nicolas-grekas Let me know how this works for you.