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] Add type-hints to all public interfaces #32176

Merged
merged 1 commit into from Jun 26, 2019

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Jun 25, 2019

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #32179
License MIT
Doc PR N/A

This PR adds type-hints to all interfaces of the Routing component to give them a more php7-like feeling. Adding these type-hints on master should not be a breaking change because we require php 7.2 there, which allows downstream classes/interfaces to remove a type-hint from a method signature.

Notes:

  • There is also an implementation of RedirectableUrlMatcherInterface in the Framework Bundle. I did not upgrade its interface in order to keep the bundle compatible with Routing 4.4.
  • We could apply similar changes to the Route, RouteCollection etc. in a follow-up PR. This PR only concentrated on the interfaces and their implementations.

@derrabus derrabus force-pushed the improvements/routing-type-hints branch from 17a86c9 to 457b322 Compare June 25, 2019 21:53
@derrabus derrabus mentioned this pull request Jun 25, 2019
57 tasks
@fabpot
Copy link
Member

fabpot commented Jun 26, 2019

Thank you @derrabus.

@fabpot fabpot merged commit 457b322 into symfony:master Jun 26, 2019
fabpot added a commit that referenced this pull request Jun 26, 2019
…bus)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[Routing] Add type-hints to all public interfaces

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32179
| License       | MIT
| Doc PR        | N/A

This PR adds type-hints to all interfaces of the Routing component to give them a more php7-like feeling. Adding these type-hints on master should not be a breaking change because we require php 7.2 there, which allows downstream classes/interfaces to remove a type-hint from a method signature.

Notes:
* There is also an implementation of `RedirectableUrlMatcherInterface` in the Framework Bundle. I did not upgrade its interface in order to keep the bundle compatible with Routing 4.4.
* We could apply similar changes to the `Route`, `RouteCollection` etc. in a follow-up PR. This PR only concentrated on the interfaces and their implementations.

Commits
-------

457b322 [Routing] Add type-hints to all public interfaces.
@derrabus derrabus deleted the improvements/routing-type-hints branch June 26, 2019 07:16
fabpot added a commit that referenced this pull request Jun 26, 2019
This PR was merged into the 3.4 branch.

Discussion
----------

[Routing] Fixed type annotation

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | N/A
| License       | MIT
| Doc PR        | N/A

The `UrlGeneratorInterface::generate()` method expects an array as argument `$parameters`, but the docblock does not reflect that. This PR fixes the type. Discovered while working on #32176.

Commits
-------

753bf7e Fixed type annotation.
@derrabus
Copy link
Member Author

On #32185 @Tobion pointed out that UrlGeneratorInterface was originally also designed for generators that support objects as $parameters. I wasn't aware of that.

Should we reconsider the array type-hint I've added there?

I would argue that array $parameters is the contract that other components implicitly expect, meaning if a custom router would not understand an array of parameters, that router would break the debug toolbar or redirects for instance.

A custom implementation of RouterInterface would still be able to widen the parameter type (contravariance) without violating the contract of the interface. That means, the implementation could remove the type-hint and allow custom objects to be passed as long as it is still capable of handling arrays.

WDYT? Keep the type-hint or drop it?

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