Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Routing] Add type-hints RequestContext and Route classes.#32181
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
Uh oh!
There was an error while loading.Please reload this page.
6a69605 tocece78cCompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
cece78c tod321a1aCompare| * @param RouteCollectionBuilder $builder | ||
| */ | ||
| publicfunctionmount($prefix,self$builder) | ||
| publicfunctionmount(string$prefix,self$builder) |
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.
prefix is a string here but could also be null in
| publicfunctionimport($resource, ?string$prefix ='/',string$type =null) |
but the prefix handling in this class seems strange anyway.
mount will make the prefix an empty string when prefix is/ (the default). but in| if (null !==$this->prefix) { |
'' instead.but I would ask you to not add types in RouteCollectionBuilder because I proposed to deprecate it in#32240
this way we can merge this now and decide later if we need to worry about all those edge-cases
…rrabus)This PR was merged into the 4.4 branch.Discussion----------[Routing] Deprecate RouteCollection::addPrefix(null)| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#32179| License | MIT| Doc PR | N/AThis PR deprecates the undocumented possibility to pass `null` as prefix to `RouteCollection::addPrefix()`. This allows us to add a `string` type-hint on the parameter in 5.0, see#32181 (comment)/cc@TobionCommits-------2a88752 [Routing] Deprecate RouteCollection::addPrefix(null).
Tobion commentedJun 28, 2019
Please rebase, remove deprecations and revert RouteCollectionBuilder |
d321a1a tobb0725fComparebb0725f to614e824Comparederrabus commentedJun 28, 2019
Done. |
Tobion commentedJun 28, 2019
Thanks Alexander. |
…s. (derrabus)This PR was merged into the 5.0-dev branch.Discussion----------[Routing] Add type-hints RequestContext and Route classes.| 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/AThis PR adds type-hints to `RequestContext` and the Route classes 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.Commits-------614e824 [Routing] Add type-hints RequestContext and Route classes.
This PR adds type-hints to
RequestContextand the Route classes 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.