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

[Routing] adding typehint to some functions and parameters #51005

Closed
wants to merge 3 commits into from

Conversation

helyakin
Copy link

Q A
Branch? 6.4
Bug fix? no
New feature? no
Deprecations? no

So i was walking around the Routing component for diverse reasons, and I came across some not type hinted:

  • Function
  • Parameter

I've also taken the freedom to add the property accessibility into the constructor since we are using php81

I have absolutely no idea if this PR is welcome, as this might be something you would not want to add to this framework for now.

Feel free to share your thoughts and opinions about this PR, I'll gladly learn whatever you might say about it !

Thanks again for your time

@wouterj
Copy link
Member

wouterj commented Jul 17, 2023

Hi!

Adding a return type is a backwards compatibility break for child classes overriding this method. In 6.4, all methods have either a PHP return type or an @return PHPdoc (#47551, #50821). This results in a deprecation notices when you override a method without adding the correct PHP return type. In 7.0, we've added all missing return types (#50852, #50846) - except from some that we want to extend for another 2 years to avoid a big burden on the community.

For the same reasons, we've also limited property types to private or internal properties, or properties on final or internal classes (see #41924 for a bit more context).

Finally, I'm not sure what our policy is with CPP. I'm not too fond of it, as it's merely syntax sugar and can cause edge-case BC breaks as well: #45721 (comment)

@nicolas-grekas
Copy link
Member

Closing as explained by @wouterj, thanks for proposing.

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

Successfully merging this pull request may close these issues.

None yet

4 participants