Skip to content

[Routing] Add params variable to condition expression #46042

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

Merged
merged 1 commit into from
Apr 21, 2022

Conversation

HypeMC
Copy link
Contributor

@HypeMC HypeMC commented Apr 14, 2022

Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? yes
Tickets -
License MIT
Doc PR symfony/symfony-docs#16747

Currently it's not possible to use request.attributes.get('_route_params') in route conditions because the parameter is not yet set when the expression is being evaluated. That happens after the matcher is finished.

This PR adds a new params variable to the condition option expression syntax:

class FooController
{
    #[Route('/foo/{id}', requirements: ['id' => '\d+'], condition: "params['id'] < 100")]
    public function index(int $id): Response
    {
    }
}

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.

Thanks, that's a nice feature, and the implementation is perfect. This leaves me only bike-shedding :D

So: what about injecting the parameter as variables directly in the expression? eg
#[Route('/foo/{id}', requirements: ['id' => '\d+'], condition: "id < 100")]

Of course we should then forbid to use context/request as var names, or at least give them higher priority.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 19, 2022

Or maybe just rename route_parameters to params?
#[Route('/foo/{id}', requirements: ['id' => '\d+'], condition: "params.id < 100")]

@nicolas-grekas nicolas-grekas modified the milestones: 6.1, 6.2 Apr 19, 2022
@HypeMC HypeMC force-pushed the route-params-in-conditions branch from 50fa6ef to acefeb9 Compare April 19, 2022 10:17
@HypeMC
Copy link
Contributor Author

HypeMC commented Apr 19, 2022

Thanks, that's a nice feature, and the implementation is perfect. This leaves me only bike-shedding :D

So: what about injecting the parameter as variables directly in the expression? eg #[Route('/foo/{id}', requirements: ['id' => '\d+'], condition: "id < 100")]

Of course we should then forbid to use context/request as var names, or at least give them higher priority.

Or maybe just rename route_parameters to params? #[Route('/foo/{id}', requirements: ['id' => '\d+'], condition: "params.id < 100")]

Thanks for the review, I've renamed the variable to params. I like the idea of injecting the parameters as variables as well, but as you mentioned, that might cause collisions with var names. This seems like the safer option, but if you want, I can inject them instead, I'm fine either way.

@HypeMC HypeMC changed the title [Routing] Add route_parameters variable to condition expression [Routing] Add params variable to condition expression Apr 19, 2022
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.

To the next reviewer: please help us answer this open question above :)

@stof
Copy link
Member

stof commented Apr 19, 2022

I prefer injecting a params variable.
From prior experience in the @Security annotation of FrameworkExtraBundle, mixing predefined variables and user-defined variable names in the same expression is a mess regarding conflicts (and still requires injecting a variable allowing to access params that could not be injected as variables due to the conflict, with a weird DX when a few param names need a special access pattern, and a forever-freeze of the list of predefined variables to avoid BC breaks)

@Tobion
Copy link
Contributor

Tobion commented Apr 20, 2022

I agree with stof.

@Tobion
Copy link
Contributor

Tobion commented Apr 20, 2022

I think it would be ok to still release this in 6.1 before the RC. It fits with #44405 and it would make sense to document/new in symfony this together.

---

* Add `params` variable to condition expression
* Deprecate not passing route parameters as the forth argument to `UrlMatcher::handleRouteRequirements()`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Deprecate not passing route parameters as the forth argument to `UrlMatcher::handleRouteRequirements()`
* Deprecate not passing route parameters as the fourth argument to `UrlMatcher::handleRouteRequirements()`

@fabpot fabpot modified the milestones: 6.2, 6.1 Apr 21, 2022
@fabpot
Copy link
Member

fabpot commented Apr 21, 2022

Thank you @HypeMC.

@fabpot fabpot merged commit 4e39d25 into symfony:6.1 Apr 21, 2022
@HypeMC HypeMC deleted the route-params-in-conditions branch April 21, 2022 06:24
HypeMC added a commit to HypeMC/symfony that referenced this pull request Apr 21, 2022
Tobion added a commit that referenced this pull request Apr 21, 2022
…(HypeMC)

This PR was merged into the 6.1 branch.

Discussion
----------

[Routing] Fix changelog & deprecation message for #46042

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Since #46042 was merged into 6.1 the changelog & deprecation message are now wrong. cc `@fabpot`

Commits
-------

bb37826 [Routing] Fix changelog & deprecation message for #46042
@fabpot fabpot mentioned this pull request Apr 27, 2022
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