Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add @return PHPdoc to {@inheritdoc} blocks from vendors #2453

Merged
merged 1 commit into from
Sep 18, 2021

Conversation

wouterj
Copy link
Contributor

@wouterj wouterj commented Sep 14, 2021

This package included almost all @return PHPdoc except from a few. Some of these have comments that were {@inheritdoc}, but given the "inheritdoc" are from PHP's DateTimeInterface, one could argue that they are "unknown".

Besides fixing the PHPdoc, these return annotations will also suppress type deprecations when Carbon is used in a Symfony application. Since Symfony 5.4, developers are warned when a child class does not yet have a return type (PHP native or PHPdoc) while the parent has. See also the Symfony docs for more info on this topic.

I'm not sure about the plans of Carbon to add PHP typing (I saw that 7.1 compat is still maintained, so I expect it'll take a while), but I hope you'll accept these PHPdocs. It'll make the life of all your Symfony users better, and I think it's also more explicit for all your Laravel/PHP users :)

@wouterj wouterj changed the title Add missing @return PHPdoc Add @return PHPdoc to {@inheritdoc} blocks from vendors Sep 15, 2021
Copy link
Collaborator

@kylekatarnls kylekatarnls left a comment

Choose a reason for hiding this comment

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

I checked a bit if having {@inheritdoc} + @return was a standard practice. While it looks like it's quite rare, I could find some in Symfony such as ServiceLocatorTrait so I guess we can add it in Carbon.

@wouterj
Copy link
Contributor Author

wouterj commented Sep 18, 2021

Thanks for the review! (and unblocking the CI, I guess the build failure is unrelated?)

If it helps in making the decision, we've recently been contributing (successfully) the same change to other projects like Doctrine (doctrine/orm#8905) and Laravel (laravel/framework#38819).

The @return on the method's PHPdoc essentially serves the same purpose as #[\ReturnTypeWillChange] for PHP internals: it can tell static analysers that you are aware that this method needs to get a return type eventually (when the vendor adds it), but you can't yet (because of your own roadmap).

Currently, the static analyser for method return type declarations in Symfony 5.4 is using this method (https://symfony.com/doc/5.4/setup/upgrade_major.html#upgrading-to-symfony-6-add-native-return-types). It allows Symfony to add return type declarations to Symfony 6 without hard breaking all dependencies/project code. (fun fact, the complete diff of this PR is generated by this tool)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants