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

[FrameworkBundle][Routing] Add a new tag to be able to use a private service as a service route loader #30926

Open
wants to merge 1 commit into
base: master
from

Conversation

@fancyweb
Copy link
Contributor

commented Apr 6, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #30402
License MIT
Doc PR symfony/symfony-docs#11337

#eufossa

This PR adds a new tag routing.route_loader autoconfigured thanks to a new ServiceRouterLoader interface to be able to use private route loader services.

The deprecation layer is done through a temporary container that will be removed in 5.0.

TODO :

  • Changelog and upgrade entry
  • Doc PR

@fancyweb fancyweb force-pushed the fancyweb:service-router-loader-private-service- branch 3 times, most recently from e05abac to d3a2998 Apr 6, 2019

@chalasr chalasr added this to the next milestone Apr 7, 2019

@fancyweb fancyweb force-pushed the fancyweb:service-router-loader-private-service- branch from d3a2998 to 93584eb Apr 7, 2019

@chalasr chalasr self-requested a review Apr 7, 2019

@nicolas-grekas
Copy link
Member

left a comment

changelog+upgrade files need an update also

@fancyweb

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

@nicolas-grekas I added the UPGRADE entries already.

For the CHANGELOG of the Routing component there is nothing to say IMO. We added a marker interface, an instantly deprecated / internal class, and a compiler pass used by the Framework Bundle 😕

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Ping @fancyweb. Will you have some time to make the last requested changes? It'd be nice to have this for Symfony 4.3. Thanks!

@fancyweb

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

I will definitely finish this work before the end of the month.

@fancyweb fancyweb force-pushed the fancyweb:service-router-loader-private-service- branch 2 times, most recently from 3a30564 to 9c5e31a Apr 29, 2019

<service id="routing.loader.service" class="Symfony\Component\Routing\Loader\DependencyInjection\ServiceRouterLoader">
<tag name="routing.loader" />
<argument type="service" id="service_container" />
<argument type="service" id="routing.loader.service.container" />

This comment has been minimized.

Copy link
@fancyweb

fancyweb Apr 29, 2019

Author Contributor

In 5.0, replace this argument with the above tagged locator.

@chalasr

This comment has been minimized.

Copy link
Member

commented May 5, 2019

Although I find route loader more clear, naming should be consistent between the interface and the tag (RouterLoader vs RouteLoader). We should stick with the current naming and rename the tag to routing.router_loader IMHO

@fancyweb fancyweb force-pushed the fancyweb:service-router-loader-private-service- branch from 9c5e31a to 0bb19b7 May 5, 2019

@chalasr

chalasr approved these changes May 5, 2019

@chalasr chalasr modified the milestones: next, 4.3 May 5, 2019

@@ -123,6 +123,7 @@ Routing
options have been deprecated.
* Implementing `Serializable` for `Route` and `CompiledRoute` is deprecated; if you serialize them, please
ensure your unserialization logic can recover from a failure related to an updated serialization format
* Not tagging the route loader services with `routing.router_loader` has been deprecated.

This comment has been minimized.

Copy link
@fabpot

fabpot May 6, 2019

Member

route loader vs router_loader

This comment has been minimized.

Copy link
@fabpot

fabpot May 6, 2019

Member

I think router loader is way better.

This comment has been minimized.

Copy link
@fancyweb

fancyweb May 6, 2019

Author Contributor

In this case, if the "public" name is now router loader, I guess we should update the whole documentation page (https://symfony.com/doc/current/routing/custom_route_loader.html)

This comment has been minimized.

Copy link
@fabpot

fabpot May 6, 2019

Member

and I meant route loader, sorry

This comment has been minimized.

Copy link
@fancyweb

fancyweb May 6, 2019

Author Contributor

Do you mean you want to move back to the original tag name routing.route_loader?

This comment has been minimized.

Copy link
@chalasr

chalasr May 6, 2019

Member

Works for me, let's rename the interface + existing implementation, I suggest deprecating ServiceRouterLoader in favor of ContainerRouteLoader.

This comment has been minimized.

Copy link
@fancyweb

fancyweb May 6, 2019

Author Contributor

WDYT of renaming everything in another PR after this one is merged for clarity ? First, finish this one (I just need to rename the tag back to routing.route_loader). Then, another one of pure refacto.

This comment has been minimized.

Copy link
@fancyweb

fancyweb May 10, 2019

Author Contributor

friendly ping @fabpot @chalasr, would be cool to have this in 4.3

@fabpot fabpot modified the milestones: 4.3, next May 6, 2019

@fancyweb fancyweb force-pushed the fancyweb:service-router-loader-private-service- branch from 0bb19b7 to 205497b May 6, 2019

*
* @deprecated since Symfony 4.3, to be removed in 5.0
*/
class ServiceRouterLoaderContainer implements ContainerInterface

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 10, 2019

Member

RouterLoaderContainer?

This comment has been minimized.

Copy link
@chalasr

fabpot added a commit that referenced this pull request May 13, 2019

bug #31463 [DI] default to service id - *not* FQCN - when building ta…
…gged locators (nicolas-grekas)

This PR was merged into the 4.3 branch.

Discussion
----------

[DI] default to service id - *not* FQCN - when building tagged locators

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | no
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

While reviewing #30926 I realized that defaulting to the FQCN is a shortcut that isn't useful enough.
Defaulting to the service id provides the same experience in practice because service ids are FQCN by default.
But when they aren't, the service id is the proper index to default to when building the locator.

Commits
-------

52e827c [DI] default to service id - *not* FQCN - when building tagged locators
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 13, 2019

#31463 is now merged, this can be rebased to leverage it and take comments into account. Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.