[FrameworkBundle][Routing] Use a PSR-11 container in FrameworkBundle Router #24738

ogizanagi · 2017-10-29T08:43:49Z

Q	A
Branch?	4.1
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	not yet
Tests pass?	yes
Fixed tickets	N/A
License	MIT
Doc PR	N/A

~~3.4 because~~ it allows to make the routing.loader service private and add sense into implementing the ServiceSubscriberInterface in the Router by injecting a ServiceLocator instead of the DI container.

Should we deprecate passing a DI ContainerInterface instance without providing the $paramFetcher argument?
Move the whole Router::resolve() method into a dedicated callable $paramResolver ?

nicolas-grekas · 2017-10-29T09:03:51Z

For 4.1 is fine also, no need to hurry IMHO.

ogizanagi · 2017-10-29T09:06:10Z

But we'll keep the routing.loader service public then?

nicolas-grekas · 2017-10-29T09:55:13Z

Public services don't really hurt anymore, and we'll always be able to find a way to make it privta later if we really want to.
Better prefer stabilization now IMHO.

fabpot · 2017-10-29T16:13:15Z

Moving the target to 4.1 as I agree with @nicolas-grekas, we need to restrict changes to important flaws and bug fixes.

fabpot · 2017-12-01T17:53:43Z

@ogizanagi Can you rebase this PR? Thanks.

ogizanagi · 2017-12-02T09:37:17Z

Rebased. Should we do anything further with this one?

fabpot

Not a big fan of the param fetcher callable, but I don't have any better idea.

fabpot · 2017-12-02T16:50:27Z

src/Symfony/Bundle/FrameworkBundle/Routing/Router.php

     * @param LoggerInterface|null $logger
     */
-    public function __construct(ContainerInterface $container, $resource, array $options = array(), RequestContext $context = null, LoggerInterface $logger = null)
+    public function __construct(ContainerInterface $container, $resource, array $options = array(), RequestContext $context = null, callable $paramFetcher = null, LoggerInterface $logger = null)


Rearranging the arguments is a BC break.

Sorry, it's not as the logger was added in 4.1 as well. Nevermind

nicolas-grekas · 2017-12-02T18:36:21Z

Not a big fan of the param fetcher callable, but I don't have any better idea.

I have one (two in fact):

make ParameterBag implement PSR-11 (should be easy),
make the container's parameter bag a service (may be a little bit more involving).

nicolas-grekas · 2017-12-03T15:06:19Z

See #25288

…ess parameters as-a-service (nicolas-grekas, sroze) This PR was merged into the 4.1-dev branch. Discussion ---------- [DI][FrameworkBundle] Add PSR-11 "ContainerBag" to access parameters as-a-service | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17160 | License | MIT | Doc PR | - There is one thing that prevents us from not injecting the container: access to the parameter bag. This PR fixes this limitation by providing a PSR-11 `ContainerBagInterface` + related implementation, and wiring it as a service that ppl can then also autowire using the new interface as a type hint, or `ParameterBagInterface`. Needed to complete e.g. #24738 Commits ------- 561cd7e Add tests on the ContainerBag 0e18d3e [DI][FrameworkBundle] Add PSR-11 "ContainerBag" to access parameters as-a-service

nicolas-grekas · 2017-12-08T12:41:35Z

@ogizanagi now that #25288 is merged, you can move forward on this one when you have some time.

ogizanagi · 2017-12-09T10:35:06Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/routing.xml

@@ -67,6 +68,7 @@
                <argument key="matcher_cache_class">%router.cache_class_prefix%UrlMatcher</argument>
            </argument>
            <argument type="service" id="router.request_context" on-invalid="ignore" />
+            <argument type="service" id="parameter_bag" on-invalid="ignore" />


on-invalid="ignore", because we still allow to install the Fwb 4.x with DI component 3.4

ogizanagi · 2017-12-09T10:37:14Z

src/Symfony/Bundle/FrameworkBundle/Routing/Router.php

+        if ($parameters) {
+            $this->paramFetcher = array($parameters, 'get');
+        } elseif ($container instanceof SymfonyContainerInterface) {
+            $this->paramFetcher = array($container, 'getParameter');


Could be simplified by using new ContainerBag($container) and always use $this->parameters->get() instead of this callable $paramFetcher property, but I'd be the one breaking the 3-cross-4 compat' 😄

Not true actually. The ContainerBag only accepts a \Symfony\Component\DependencyInjection\Container instance. Not the interface.

ogizanagi · 2017-12-09T10:39:59Z

@nicolas-grekas : Done. Thanks for working on the PSR-11 ContainerBag feature and for the ping. I'm kinda out of the game currently.

Changed to use a PSR-11 bag for parameters

nicolas-grekas · 2017-12-09T13:49:13Z

src/Symfony/Bundle/FrameworkBundle/Routing/Router.php

     */
-    public function __construct(ContainerInterface $container, $resource, array $options = array(), RequestContext $context = null, LoggerInterface $logger = null)
+    public function __construct(ContainerInterface $container, $resource, array $options = array(), RequestContext $context = null, ContainerInterface $parameters = null, LoggerInterface $logger = null)


the new arg should be added last, otherwise that's a BC break, isn't it?

Sorry, it's not as the logger was added in 4.1 as well. Nevermind

:)

nicolas-grekas · 2017-12-09T13:50:55Z

src/Symfony/Bundle/FrameworkBundle/Routing/Router.php

@@ -130,6 +138,10 @@ private function resolveParameters(RouteCollection $collection)
     */
    private function resolve($value)
    {
+        if (!$paramFetcher = $this->paramFetcher) {


shouldn't we throw instead in the constructor to prevent dealing with this case here?

One could theoretically use this router without the need to resolve parameters, so instead we could rather move up this check in getRouteCollection to never call resolveParameters on no way to retrieve parameters.
But yes for now the exception looks fine to me. Updated.

nicolas-grekas · 2017-12-09T14:44:07Z

(but failing test for now)

…FrameworkBundle Router

ogizanagi · 2017-12-09T15:22:10Z

This failure on deps="low" build means we must either:

drop 3-cross-4 compat' for this by requiring DI 4.1 in the Fwb, so we can always use the parameter_bag service.
replace the injected service locator by the service_container as injected before if parameter_bag isn't available.

For now, 2 is implemented.

fabpot · 2017-12-11T22:00:52Z

Thank you @ogizanagi.

…rameworkBundle Router (ogizanagi) This PR was merged into the 4.1-dev branch. Discussion ---------- [FrameworkBundle][Routing] Use a PSR-11 container in FrameworkBundle Router | Q | A | ------------- | --- | Branch? | 4.1  | Bug fix? | no | New feature? | yes  | BC breaks? | no | Deprecations? | not yet  | Tests pass? | yes | Fixed tickets | N/A  | License | MIT | Doc PR | N/A ~3.4 because~ it allows to make the `routing.loader` service private and add sense into implementing the `ServiceSubscriberInterface` in the `Router` by injecting a ServiceLocator instead of the DI container. Should we deprecate passing a DI `ContainerInterface` instance without providing the `$paramFetcher` argument? Move the whole `Router::resolve()` method into a dedicated `callable $paramResolver` ? Commits ------- 5a2f295 [FrameworkBundle][Routing] Use a PSR-11 container & parameter bag in FrameworkBundle Router

carsonbot added Status: Needs Review FrameworkBundle Routing Feature labels Oct 29, 2017

fabpot added this to the 4.1 milestone Oct 29, 2017

ogizanagi mentioned this pull request Nov 10, 2017

[FrameworkBundle][Routing] deprecation error when decorating routing.loader #24886

Closed

carsonbot added Status: Needs Work and removed Status: Needs Review labels Dec 2, 2017

fabpot previously requested changes Dec 2, 2017

View reviewed changes

nicolas-grekas mentioned this pull request Dec 3, 2017

[DI][FrameworkBundle] Add PSR-11 "ContainerBag" to access parameters as-a-service #25288

Merged

ogizanagi commented Dec 9, 2017

View reviewed changes

chalasr added Status: Needs Review and removed Status: Needs Work labels Dec 9, 2017

nicolas-grekas reviewed Dec 9, 2017

View reviewed changes

nicolas-grekas approved these changes Dec 9, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 9, 2017

[FrameworkBundle][Routing] Use a PSR-11 container & parameter bag in …

5a2f295

…FrameworkBundle Router

nicolas-grekas approved these changes Dec 9, 2017

View reviewed changes

Simperfit approved these changes Dec 9, 2017

View reviewed changes

chalasr approved these changes Dec 10, 2017

View reviewed changes

fabpot approved these changes Dec 11, 2017

View reviewed changes

fabpot merged commit 5a2f295 into symfony:master Dec 11, 2017

ogizanagi deleted the fwb/router/param_fetcher branch December 12, 2017 07:20

javiereguiluz mentioned this pull request Jan 8, 2018

[FrameworkBundle][Routing] Use a PSR-11 container in FrameworkBundle Router symfony/symfony-docs#9006

Closed

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle][Routing] Use a PSR-11 container in FrameworkBundle Router #24738

[FrameworkBundle][Routing] Use a PSR-11 container in FrameworkBundle Router #24738

ogizanagi commented Oct 29, 2017 •

edited

Loading

nicolas-grekas commented Oct 29, 2017

ogizanagi commented Oct 29, 2017

nicolas-grekas commented Oct 29, 2017

fabpot commented Oct 29, 2017

fabpot commented Dec 1, 2017

ogizanagi commented Dec 2, 2017

fabpot left a comment

fabpot Dec 2, 2017

fabpot Dec 2, 2017

nicolas-grekas commented Dec 2, 2017

nicolas-grekas commented Dec 3, 2017

nicolas-grekas commented Dec 8, 2017

ogizanagi Dec 9, 2017

ogizanagi Dec 9, 2017 •

edited

Loading

ogizanagi commented Dec 9, 2017

nicolas-grekas Dec 9, 2017

nicolas-grekas Dec 9, 2017

nicolas-grekas Dec 9, 2017

ogizanagi Dec 9, 2017

nicolas-grekas commented Dec 9, 2017

ogizanagi commented Dec 9, 2017

fabpot commented Dec 11, 2017

[FrameworkBundle][Routing] Use a PSR-11 container in FrameworkBundle Router #24738

[FrameworkBundle][Routing] Use a PSR-11 container in FrameworkBundle Router #24738

Conversation

ogizanagi commented Oct 29, 2017 • edited Loading

nicolas-grekas commented Oct 29, 2017

ogizanagi commented Oct 29, 2017

nicolas-grekas commented Oct 29, 2017

fabpot commented Oct 29, 2017

fabpot commented Dec 1, 2017

ogizanagi commented Dec 2, 2017

fabpot left a comment

Choose a reason for hiding this comment

fabpot Dec 2, 2017

Choose a reason for hiding this comment

fabpot Dec 2, 2017

Choose a reason for hiding this comment

nicolas-grekas commented Dec 2, 2017

nicolas-grekas commented Dec 3, 2017

nicolas-grekas commented Dec 8, 2017

ogizanagi Dec 9, 2017

Choose a reason for hiding this comment

ogizanagi Dec 9, 2017 • edited Loading

Choose a reason for hiding this comment

ogizanagi commented Dec 9, 2017

nicolas-grekas Dec 9, 2017

Choose a reason for hiding this comment

nicolas-grekas Dec 9, 2017

Choose a reason for hiding this comment

nicolas-grekas Dec 9, 2017

Choose a reason for hiding this comment

ogizanagi Dec 9, 2017

Choose a reason for hiding this comment

nicolas-grekas commented Dec 9, 2017

ogizanagi commented Dec 9, 2017

fabpot commented Dec 11, 2017

ogizanagi commented Oct 29, 2017 •

edited

Loading

ogizanagi Dec 9, 2017 •

edited

Loading