Skip to content
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

[Routing] Allow using services in the route condition #44405

Merged

Conversation

renanbr
Copy link
Contributor

@renanbr renanbr commented Dec 1, 2021

Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets n/a
License MIT
Doc PR n/a
#[AsRoutingConditionService(alias: 'foo')]
class SomeFooService
{
    public function bar(): bool { ... }
}
class DefaultController
{
    #[Route('/page', condition: "service('foo').bar()")]
    public function page(): Response { ... }
} 

@carsonbot
Copy link

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@renanbr renanbr force-pushed the inject-variable-route-expression-condition branch from 95cf5ab to cc6df1c Compare December 1, 2021 21:41
@renanbr renanbr marked this pull request as ready for review December 1, 2021 21:41
@carsonbot carsonbot added this to the 6.1 milestone Dec 1, 2021
@nicolas-grekas
Copy link
Member

On second though, we might want to go a bit further: with the current approach, referenced services are going to be instantiated all the time, while they might be needed only on very specific routes.
What about providing a service locator instead, after making EL able to deal with them?

@chapterjason
Copy link
Contributor

What if two services are registered with the same name? This could cause some confusing issues, every service will override the one registered before.

If docs will be added: prefix the service names in third party bundles.

@renanbr
Copy link
Contributor Author

renanbr commented Dec 2, 2021

@nicolas-grekas

On second though, we might want to go a bit further: with the current approach, referenced services are going to be instantiated all the time, while they might be needed only on very specific routes.
What about providing a service locator instead, after making EL able to deal with them?

Instantiating services all the time is indeed not good. Based on your feedback I was wondering if we could inject a function that relies on a service locator instead of injecting variables

#[Route('/page', condition: "service('myService').myMethod()")]

Pros:

  • No changes in UrlMatcher (we can use the already available function provider feature)
  • We'll create services only when requested

What do you think?

@nicolas-grekas
Copy link
Member

I like it @renanbr!

@renanbr
Copy link
Contributor Author

renanbr commented Dec 3, 2021

Status: Since eafc9c5, the FrameworkBundle wraps tagged services into a ServiceLocator, then passes it to the routing component, which unpacks services in the matcher when necessary.

I'm satisfied with this ☝🏼 solution.

My next shot will be implementing service('name') as described in #44405 (comment)

@renanbr renanbr force-pushed the inject-variable-route-expression-condition branch from eafc9c5 to 9759f3b Compare December 6, 2021 19:47
@jvasseur
Copy link
Contributor

jvasseur commented Mar 9, 2022

Why not using the ExpressionFunctionProviderInterface extension point instead of creating a new way of extending the expression language ?

@nicolas-grekas
Copy link
Member

Why not using the ExpressionFunctionProviderInterface extension point instead of creating a new way of extending the expression language ?

For convenience I think. What would be the DX of your proposal?

@nicolas-grekas
Copy link
Member

@renanbr up to finish this PR?

@GromNaN
Copy link
Member

GromNaN commented Apr 3, 2022

Why not using the ExpressionFunctionProviderInterface extension point instead of creating a new way of extending the expression language ?

For convenience I think. What would be the DX of your proposal?

Late it the discussion. A new function service() would provide access to the container.

class DefaultController
{
    #[Route('/page', condition: 'service('foo').bar()')]
    public function page(): Response { ... }
} 

@nicolas-grekas
Copy link
Member

A new function service() would provide access to the container.

The one true container you mean? I guess that's no-go since that'd require services to be made public. The benefit of this proposal is that it allows labeling which services need to be made available for the routing layer.

@GromNaN
Copy link
Member

GromNaN commented Apr 3, 2022

Maybe a service locator, feed with regular tagged services.

@nicolas-grekas
Copy link
Member

That's exactly what this PR does IIUC :)

@GromNaN
Copy link
Member

GromNaN commented Apr 3, 2022

Indeed. Then the description needs to be updated.

@renanbr
Copy link
Contributor Author

renanbr commented Apr 5, 2022

@renanbr up to finish this PR?

I'll work on this week and the next week.

thanks for the review btw

@renanbr renanbr force-pushed the inject-variable-route-expression-condition branch from 9051221 to 61646a2 Compare April 5, 2022 17:39
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.

This is so rad! Please add a few tests and we'll be good!

@renanbr renanbr force-pushed the inject-variable-route-expression-condition branch 2 times, most recently from 3db2a42 to 2440987 Compare April 12, 2022 19:07
@renanbr
Copy link
Contributor Author

renanbr commented Apr 12, 2022

updated,
latest changes

  • rename tag to routing.condition_service;
  • rename tag identifier to alias;
  • rename class attribute to AsRoutingConditionService;
  • move class attribute under FrameworkBundle;
  • rename expression language function to container.get_routing_condition_service;
  • add tests

@renanbr renanbr force-pushed the inject-variable-route-expression-condition branch from 2440987 to 01c0a40 Compare April 13, 2022 18:06
Copy link
Member

@Tobion Tobion left a comment

Choose a reason for hiding this comment

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

Well done

Gather routing variables into a service locator

Add service() function for route condition

Add missing file

Fix psalm

remove routing variable concept

Add tests and rename routing condition service

make fabbot happy

add entry in changelog
@renanbr renanbr force-pushed the inject-variable-route-expression-condition branch from 01c0a40 to 3724576 Compare April 14, 2022 07:04
@Tobion
Copy link
Member

Tobion commented Apr 14, 2022

Thanks @renanbr for working on this feature, this is much appreciated.

@Tobion Tobion merged commit a541f15 into symfony:6.1 Apr 14, 2022
@fancyweb fancyweb mentioned this pull request Apr 14, 2022
nicolas-grekas added a commit that referenced this pull request Apr 14, 2022
This PR was merged into the 6.1 branch.

Discussion
----------

Minor cleanup

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

I was reviewing #44405 when it was merged. I had some minor fixes to do so I added them to this PR.

Commits
-------

c68878c Minor cleanup
@renanbr renanbr deleted the inject-variable-route-expression-condition branch April 14, 2022 09:22
@fabpot fabpot mentioned this pull request Apr 15, 2022
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request May 5, 2022
…enanbr)

This PR was merged into the 6.1 branch.

Discussion
----------

[Routing] Allow using services in the route condition

Documents symfony/symfony#44405

Closes #16714

Commits
-------

c2478da [Routing] Allow using services in the route condition
nicolas-grekas added a commit that referenced this pull request May 21, 2022
…nditionService] (nicolas-grekas)

This PR was merged into the 6.1 branch.

Discussion
----------

[FrameworkBundle] Simplify registration of #[AsRoutingConditionService]

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

Small simplification on top of #44405

The changes on `composer.json` just bump explicitly what is already bumped transitively.

Commits
-------

953f505 [FrameworkBundle] Simplify registration of #[AsRoutingConditionService]
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.

None yet

7 participants