Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Routing] Add possibility to create a request context with parameters directly#60120
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
base:7.4
Are you sure you want to change the base?
Conversation
| @@ -43,6 +43,7 @@ public function __construct(string $baseUrl = '', string $method = 'GET', string | |||
| $this->setHttpsPort($httpsPort); | |||
| $this->setPathInfo($path); | |||
| $this->setQueryString($queryString); | |||
| $this->setParameters([...$this->parameters, ...$parameters]); | |||
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.
The class is not final, so we merge it with eventually parent set parameters.
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.
which parent set parameters are you talking about ?
alexander-schranzApr 2, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
A class notfinal like RequestContext can beextends and call inside it constructor before the parent construct thesetter methods:
class MyCustomContextextends RequestContext {publicfunction__construct() {$this->setParameters(['key' =>'value']);parent::__construct(); }}
See also new added test. Doing justsetParameters($parameters); would override existing parameters of that class.
alexander-schranzApr 2, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
we could also create a deprecationif (count($this->parameters) > 0) { and simplify this in Symfony 8?
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.
One other question is maybe what is expected behaviour for:
$this->setParameters(['key' =>'value']);parent::__construct(parameters: ['other' =>'test']);
If not merged we could go for bc layer with:
if (count($parameters) >0) {$this->setParameters($parameters);}elseif (count($this->parameters) >0) {trigger_deprecation('symfony/routing','7.3','Set parameters directly via the own constructor argument');}
which in 8.0 gets:
$this->setParameters($parameters);
Uh oh!
There was an error while loading.Please reload this page.
| $this->assertEquals(['foo' => 'bar'], $requestContext->getParameters()); | ||
| } | ||
| public function testConstructParametersBcLayer() |
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.
not sure what this test case is about
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.
its about why theif is needed for BC reasons.
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.
This lines. As the require for the bc case in this tests, other tests wouldn't catch that.
+ if ($parameters) { $this->parameters = $parameters;+ }
First I wanted to trigger deprecation so we could just remove it with 8.0.
Co-authored-by: Nicolas Grekas <nicolas.grekas@gmail.com>
25f7f5f to32a29d8Compare
Uh oh!
There was an error while loading.Please reload this page.
While I worked on the route integration into Sulu. I had to write a few tests for depending on the RequestContext. To improve the DX I think the parameters should be possible also inside the construct directly so instead of:
I can do: