Skip to content

Conversation

alexander-schranz
Copy link
Contributor

@alexander-schranz alexander-schranz commented Apr 2, 2025

Q A
Branch? 7.4
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #...
License MIT

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:

$requestContext = new RequestContext();
$requestContext->setParameters(['foo' => 'bar']);
$service->myMethod($requestContext);

I can do:

$service->myMethod(new RequestContext(parameters: ['foo' => 'bar']));

@@ -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]);
Copy link
Contributor Author

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.

Copy link
Member

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 ?

Copy link
Contributor Author

@alexander-schranz alexander-schranz Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A class not final like RequestContext can be extends and call inside it constructor before the parent construct the setter methods:

class MyCustomContext extends RequestContext {
     public function __construct()
     {
          $this->setParameters(['key' => 'value']);
          parent::__construct();
     }
}

See also new added test. Doing just setParameters($parameters); would override existing parameters of that class.

Copy link
Contributor Author

@alexander-schranz alexander-schranz Apr 2, 2025

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 deprecation if (count($this->parameters) > 0) { and simplify this in Symfony 8?

Copy link
Contributor Author

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);

@carsonbot carsonbot changed the title Add possibility to create a request context with parameters directly [Routing] Add possibility to create a request context with parameters directly Apr 2, 2025
$this->assertEquals(['foo' => 'bar'], $requestContext->getParameters());
}

public function testConstructParametersBcLayer()
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its about why the if is needed for BC reasons.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@alexander-schranz alexander-schranz Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolas-grekas or should we think of that nobody did call setParameters before calling __construct then we can remove the if and this test completely.

@alexander-schranz alexander-schranz force-pushed the enhancement/quickly-create-request-context-with-parameters branch from 25f7f5f to 32a29d8 Compare May 26, 2025 17:51
@fabpot fabpot modified the milestones: 7.3, 7.4 May 26, 2025
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor CS changes

@nicolas-grekas
Copy link
Member

Oh, as usual: don't forget the changelog file

@fabpot fabpot force-pushed the enhancement/quickly-create-request-context-with-parameters branch from 9117f2f to a3d3cb5 Compare August 20, 2025 07:06
@fabpot
Copy link
Member

fabpot commented Aug 20, 2025

Thank you @alexander-schranz.

@fabpot fabpot merged commit 5ef1ccb into symfony:7.4 Aug 20, 2025
3 of 12 checks passed
@alexander-schranz alexander-schranz deleted the enhancement/quickly-create-request-context-with-parameters branch August 20, 2025 07:09
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.

6 participants