[Routing] unify RequestContext as fluent class #9073
Conversation
| @@ -219,19 +227,19 @@ public function getHttpsPort() | |||
| /** | |||
| * Sets the HTTPS port. | |||
| * | |||
| * @param string $httpsPort The HTTPS port | |||
| * @param integer $httpsPort The HTTPS port | |||
There was a problem hiding this comment.
this can be integer|string no?
There was a problem hiding this comment.
it can but not sure if we want to document that
|
@Tobion What about this PR? Do you have time to finish it? |
|
@fabpot yes I will. Do you agree with all the points in the description? |
7fb7885 to
bf47577
Compare
| * @param array $parameters The parameters | ||
| * | ||
| * @return Route The current Route instance |
There was a problem hiding this comment.
this was even documenting the wrong class
|
@fabpot this is ready. IMO this should be merged into 2.3 to reduce the divergence. But lets see what the mergers say. |
bf47577 to
93b6050
Compare
| @@ -47,21 +48,26 @@ class RequestContext | |||
| * @param int $httpsPort The HTTPS port | |||
| * @param string $path The path | |||
| * @param string $queryString The query string | |||
| * | |||
| * @api | |||
There was a problem hiding this comment.
I think that @api calls must not be removed. We do have a lot of classes with @api on methods only. I understand that this is kind of redundant with the @api on the class, but that's probably not related to this PR and should be discussed for an overall change instead of a local one.
There was a problem hiding this comment.
ok I left this untouched
93b6050 to
6c31ce4
Compare
| * | ||
| * @param Request $request A Request instance | ||
| * | ||
| * @return RequestContext The current instance, implementing a fluent interface |
There was a problem hiding this comment.
Shouldn't this be self (instead of RequestContext)?
|
Looks good to me. I'm a bit reluctant to merge it into 2.3 though. What @symfony/deciders think? |
|
Well, fixing the inconsistencies in the type of values could go in 2.3 IMO. but turning the API into a fluent one should only be done in master IMO. It is not a bugfix |
|
@stof agreed |
|
I agree as well |
|
I would consider the fluent thing a bug fix because one method had this already (probably unintentional). So either the ugly code stays in an LTS version or we make all setters fluent (which is not considered a bc break). |
|
If you still want me to split it, I can do it though |
|
I prefer to stay strict on what we do in 2.3. So, the split looks like a better option to me (even if indeed these changes are not a big deal.) |
|
Here you go, the one for 2.3: #12190 |
|
If #12190 is merged and 2.3 merged into master, I can rebase this. |
|
#12190 merged and 2.3 merged into master |
6c31ce4 to
41412f9
Compare
|
Adjusted |
|
Thank you @Tobion. |
This PR was merged into the 2.6-dev branch. Discussion ---------- [Routing] unify RequestContext as fluent class | Q | A | ------------- | --- | Bug fix? | yes | New feature? | partly | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | - [x] <del>some methods were tagged with `@api` which makes no sense since the class is already tagged as stable. this automatically includes all methods. also our bc promise is only about `@api` on classes</del> - [x] setParameters the only fluent method! I made them all fluent which makes sense for this class Commits ------- 41412f9 [Routing] unify fluent interface in RequestContext
some methods were tagged with@apiwhich makes no sense since the class is already tagged as stable. this automatically includes all methods. also our bc promise is only about@apion classes