Deprecate bundle:controller:action and service:method notation #26085

Tobion · 2018-02-08T03:59:44Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#25910
License	MIT
Doc PR

The a::b notation had some awkward limitations. It supported MyControllerClass::method where MyControllerClass is either plain class or a service with the same name but the class must exists. This meant it did NOT support my_service_controller_id::method because the class_exists check would fail at the wrong point in time. But it did support services where class name == id, i.e. the new auto registration based psr naming. This made it very confusing.

I enhanced the a::b notation to be very straight forward:

if a exists as a service then use a as a service
otherwise try to use a as a class, i.e. new $a()
otherwise check if a::b is a static method (only relevant when the class is abstract or has private contructor). this was potentially supported when using array controller syntax. it now works the same when using the :: string syntax, like in php itself. since it only happens when nothing else works, it does not have any performance impact.

The old a:b syntax is deprecated and just forwards to a::b now internally, just as bundle:controller:action.
In general I was able to refactor the logic quite a bit because it always goes through instantiateController now.
Spotting deprecated usages is very easy as all outdated routing configs will trigger a deprecation with the DelegatingLoader and it will be normalized in the dumped routes. So you don't get a deprecation again in the ControllerResolver. But if the controller does not come from routing, e.g. twigs render controller function, then it will still be triggered there.

Tobion · 2018-02-08T04:07:39Z

src/Symfony/Bundle/FrameworkBundle/Controller/ControllerResolver.php


-        if (1 === substr_count($controller, ':') && is_array($resolvedController)) {
-            $resolvedController[0] = $this->configureController($resolvedController[0]);


This did not configure the controller when it's invokable. It's fixed now since it's also going through instantiateController now like all the other cases.

Tobion · 2018-02-08T04:10:27Z

src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php

-        $controller = parent::getController($request);
-
-        if (is_array($controller) && isset($controller[0]) && is_string($controller[0]) && $this->container->has($controller[0])) {
-            $controller[0] = $this->instantiateController($controller[0]);


This does not belong here. I moved it to the base controller resolver. This is much more consistent and also makes array controller work where the class needs to be instantiated with old-school new $class.

Tobion · 2018-02-08T04:36:59Z

src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php

+            }
+
+            if (!is_callable($controller)) {
+                throw new \InvalidArgumentException(sprintf('The controller for URI "%s" is not callable. %s', $request->getPathInfo(), $this->getControllerError($controller)));


this array controller case was missing validation. otherwise it fails in the kernel with a type error at

symfony/src/Symfony/Component/HttpKernel/Event/FilterControllerEvent.php

Line 32 in 89d1b65

public function __construct(HttpKernelInterface $kernel, callable $controller, Request $request, ?int $requestType)

fabpot · 2018-02-09T06:12:40Z

src/Symfony/Bundle/FrameworkBundle/Controller/ControllerResolver.php

        }

-        return $resolvedController;
+        return parent::createController($controller);


The whole class can be deprecated after removing the deprecated code in 5.1. Which means removing this class in 6.0. What about deprecating the class right now. Upgrading could then be done by 1/ removing all old notation usage 2/ switching to the Component class equivalent. WDYT?

You mean 4.1 and 5.0 right :)

@fabpot we still need to class for the ContainerAwareInterface logic, see configureController. So we can't deprecate it now. We might want to remove ContainerAware logic in the future as well, but that's a different issue.

Indeed, I missed that. It could be deprecated at some point, but definitely not part of this work.

Can't we move the ContainerAwareInterface logic to the ContainerControllerResolver of the component ?

I don't think it's worth it because there is also logic for the AbstractController which is part of the framework bundle. So it fits in the framework bundle.

stof · 2018-02-09T14:31:36Z

src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php

@@ -47,23 +43,27 @@ public function getController(Request $request)
        }

        if (is_array($controller)) {
+            if (isset($controller[0]) && is_string($controller[0])) {
+                $controller[0] = $this->instantiateController($controller[0]);


wouldn't this break support for using static methods as controller ?

Yeah I also thought about this. We don't have tests using static methods and class::method already had the problem of not supporting static methods while that would be a valid php callable.
But I already have a solution for this in my mind that will support both cases.

implemented

stof · 2018-02-09T14:34:12Z

src/Symfony/Bundle/FrameworkBundle/Controller/ControllerResolver.php

        }

-        return $resolvedController;
+        return parent::createController($controller);


Can't we move the ContainerAwareInterface logic to the ContainerControllerResolver of the component ?

Tobion · 2018-02-11T00:34:55Z

Tests should pass once merged.

lyrixx · 2018-02-12T10:59:39Z

src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php

+                }
+            }
+
+            if (!is_callable($controller)) {


What about using http://php.net/manual/en/closure.fromcallable.php to ensure it's callable?

I don't know what you intend. In which way does Closure::fromCallable help here?

The Closure::fromCallable method calls is_callable for you.
Then, It "standardize" how we (will) manage callable

Using a closure here provides no benefit and the error it would return for wrong callables is worse than what getControllerError does. For example it does not hint that an __invoke method might be missing. https://3v4l.org/fJJNs

ro0NL

just some random comments :) 👍 another great step forward.

ro0NL · 2018-02-12T19:09:07Z

src/Symfony/Bundle/FrameworkBundle/Controller/ControllerNameParser.php

@@ -41,6 +43,8 @@ public function __construct(KernelInterface $kernel)
     */
    public function parse($controller)
    {
+        @trigger_error(sprintf('The %s class is deprecated since version 4.1 and will be removed in 5.0.', __CLASS__), E_USER_DEPRECATED);


why not put the trigger on top of the file?

because we still use the class and service internally. but we only want to trigger when it gets executed

ro0NL · 2018-02-12T19:09:25Z

src/Symfony/Bundle/FrameworkBundle/Controller/ControllerNameParser.php

@@ -19,6 +19,8 @@
 * (Bundle\BlogBundle\Controller\PostController::indexAction).
 *
 * @author Fabien Potencier <fabien@symfony.com>
+ *
+ * @deprecated since version 4.1, will be removed in 5.0.


, to be removed in Symfony 5.0

I normalized all deprecation messages

ro0NL · 2018-02-12T19:09:48Z

src/Symfony/Bundle/FrameworkBundle/Controller/ControllerNameParser.php

@@ -86,6 +90,8 @@ public function parse($controller)
     */
    public function build($controller)
    {
+        @trigger_error(sprintf('The %s class is deprecated since version 4.1 and will be removed in 5.0.', __CLASS__), E_USER_DEPRECATED);


"%s" quoted + Symfony 5.0

ro0NL · 2018-02-12T19:13:53Z

src/Symfony/Component/HttpKernel/Controller/ControllerResolverInterface.php

@@ -17,8 +17,6 @@
 * A ControllerResolverInterface implementation knows how to determine the
 * controller to execute based on a Request object.
 *
- * It can also determine the arguments to pass to the Controller.


to be applied on 4.0?

I guess also for 3.4 but no priority for me

HeahDude

Great!

HeahDude · 2018-02-12T19:24:06Z

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

+                }
+            }
+
+            if (1 === substr_count($controller, ':')) {


You could use an elseif here if the condition is mutually exclusive with the one above. It would prevent evaluating this one when the first is true.

Depends if someone used an extended name parser that returned something else. I would leave it like this for simplicitly.

HeahDude · 2018-02-12T19:31:23Z

...mfony/Component/HttpKernel/DependencyInjection/RemoveEmptyControllerArgumentLocatorsPass.php

@@ -47,8 +47,7 @@ public function process(ContainerBuilder $container)
            } else {
                // any methods listed for call-at-instantiation cannot be actions
                $reason = false;
-                $action = substr(strrchr($controller, ':'), 1);
-                $id = substr($controller, 0, -1 - strlen($action));
+                list($id, $action) = explode('::', $controller);


What about using [$id, $action] here?

We could but I don't think we use that syntax in the code base yet.

Tobion · 2018-02-20T21:51:12Z

Can we merge this? I'd prefer to not having to rebase this again.

fabpot · 2018-02-21T05:14:11Z

Thank you @Tobion.

…notation (Tobion) This PR was squashed before being merged into the 4.1-dev branch (closes #26085). Discussion ---------- Deprecate bundle:controller:action and service:method notation | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #25910 | License | MIT | Doc PR | The `a::b` notation had some awkward limitations. It supported `MyControllerClass::method` where `MyControllerClass` is either plain class or a service with the same name but the class must exists. This meant it did NOT support `my_service_controller_id::method` because the `class_exists` check would fail at the wrong point in time. But it did support services where class name == id, i.e. the new auto registration based psr naming. This made it very confusing. I enhanced the `a::b` notation to be very straight forward: - if `a` exists as a service then use `a` as a service - otherwise try to use `a` as a class, i.e. `new $a()` - otherwise check if a::b is a static method (only relevant when the class is abstract or has private contructor). this was potentially supported when using array controller syntax. it now works the same when using the `::` string syntax, like in php itself. since it only happens when nothing else works, it does not have any performance impact. The old `a:b` syntax is deprecated and just forwards to `a::b` now internally, just as bundle:controller:action. In general I was able to refactor the logic quite a bit because it always goes through `instantiateController` now. Spotting deprecated usages is very easy as all outdated routing configs will trigger a deprecation with the DelegatingLoader and it will be normalized in the dumped routes. So you don't get a deprecation again in the ControllerResolver. But if the controller does not come from routing, e.g. twigs render controller function, then it will still be triggered there. - [x] deprecate `a:b:c` - [x] deprecate `a:b` - [x] update existing references to `a::b` - [x] fix tests - [x] fix/add support for static controllers - [x] add support for closures as controllers - [x] update Symfony\Component\Routing\Loader\ObjectRouteLoader - [x] deprecate \Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser but we still need to use it in several places for BC. - [x] add changelog/upgrade - [x] update controller.service_arguments logic for double colon controller syntax Commits ------- f8a609c Deprecate bundle:controller:action and service:method notation

… docblock (hacfi) This PR was merged into the 4.1-dev branch. Discussion ---------- [FrameworkBundle] Use `a::b` notation in ControllerTrait docblock | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #26085 | License | MIT | Doc PR | already documented  Replaced the deprecated the bundle notation with the new `a::b` notation in the docblock of `ControllerTrait::forward` Commits ------- 973c5ec [FrameworkBundle] Use `a::b` notation in ControllerTrait docblock

… (Tobion) This PR was merged into the 4.1-dev branch. Discussion ---------- [HttpKernel] fix duplicate controller resolver test case | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | #26085 | License | MIT | Doc PR | Commits ------- fc5afea fix duplicate controller resolver test case

…anged browser kit client to not catch exceptions to fix our test suite. References: - https://symfony.com/blog/new-in-symfony-4-1-deprecated-the-bundle-notation - symfony/symfony#26085 - symfony/symfony#22890 Signed-off-by: Rob Frawley 2nd <rmf@src.run>

Majkl578 · 2018-06-06T13:19:52Z

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

+            }
+
+            if (1 === substr_count($controller, ':')) {
+                $controller = str_replace(':', '::', $controller);


Unfortunately this change is a BC break and breaks custom code relying on single colon notation. (Simply commenting this out fixes the BC break.)

I see. Removing this is an option. Do you want to create a PR?

Do you want to create a PR?

Not necessarily, I don't have knowledge of Symfony test suite so I'd be happy if you do that, unless you are busy. 👍

Btw already reported in #27522.

@Tobion

… notation (dmaicher) This PR was merged into the 4.1 branch. Discussion ---------- [FrameworkBundle] fix for allowing single colon controller notation | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27522 | License | MIT | Doc PR | - This fixes a BC break introduced in #26085 (review). ping @Tobion Commits ------- 1680674 [FrameworkBundle] fix for allowing single colon controller notation

…roller (Tobion) This PR was merged into the 4.3 branch. Discussion ---------- [FrameworkBundle] fix test fixture using deprecated controller | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | no | New feature? | no  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | | License | MIT | Doc PR | https://github.com/symfony/symfony/pull/21035/files#diff-636946f5b52668178ede98d59135a181 reintroduced some deprecated controller notations (#26085) which wasn't spotted because ResolveControllerNameSubscriber missed a deprecation trigger. This also adds this trigger. Commits ------- 7f00a74 [FrameworkBundle] fix test fixture using deprecated controller and add missing deprecation

https://www.php.net/manual/en/language.oop5.paamayim-nekudotayim.php https://symfony.com/blog/new-in-symfony-4-1-deprecated-the-bundle-notation symfony/symfony#26085 Co-authored-by: Adam Wójs <adam@wojs.pl>

carsonbot added Status: Needs Review Feature Deprecation labels Feb 8, 2018

Tobion changed the title ~~WIP: Deprecate old controller notations~~ WIP: Deprecate bundle:controller:action and service:method notation Feb 8, 2018

Tobion commented Feb 8, 2018

View reviewed changes

Tobion force-pushed the deprecate-old-controller-notations branch 3 times, most recently from 4e2d6ef to f1d6484 Compare February 8, 2018 19:47

carsonbot added Status: Needs Work and removed Status: Needs Review labels Feb 9, 2018

fabpot requested changes Feb 9, 2018

View reviewed changes

stof reviewed Feb 9, 2018

View reviewed changes

Tobion mentioned this pull request Feb 9, 2018

Annotation routing in the core #25103

Closed

Tobion force-pushed the deprecate-old-controller-notations branch 2 times, most recently from 252cbcc to 0632c6b Compare February 11, 2018 00:21

Tobion changed the title ~~WIP: Deprecate bundle:controller:action and service:method notation~~ Deprecate bundle:controller:action and service:method notation Feb 11, 2018

Tobion added Status: Needs Review and removed Status: Needs Work labels Feb 11, 2018

nicolas-grekas added this to the 4.1 milestone Feb 11, 2018

Tobion force-pushed the deprecate-old-controller-notations branch 2 times, most recently from a7d6d0a to 23c999d Compare February 11, 2018 22:34

lyrixx reviewed Feb 12, 2018

View reviewed changes

ro0NL reviewed Feb 12, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 12, 2018

HeahDude approved these changes Feb 12, 2018

View reviewed changes

Tobion force-pushed the deprecate-old-controller-notations branch from b3fe6f0 to 748f725 Compare February 12, 2018 20:36

fabpot closed this Feb 21, 2018

Tobion mentioned this pull request Feb 21, 2018

Update doc to avoid bundle:controller:action and service:method controller syntax symfony/symfony-docs#9306

Closed

Tobion deleted the deprecate-old-controller-notations branch February 21, 2018 20:42

Tobion mentioned this pull request Feb 24, 2018

[HttpKernel] [DX] Configurable controller layout #25422

Closed

2 tasks

spackmat mentioned this pull request Mar 7, 2018

Feature request: Stop using service_id:method notation for _controller of routes FriendsOfSymfony/FOSUserBundle#2751

Closed

Haehnchen mentioned this pull request Mar 17, 2018

Symfony 4.1 Routing: support double colon changes Haehnchen/idea-php-symfony2-plugin#1159

Open

hacfi mentioned this pull request Mar 18, 2018

[FrameworkBundle] Use a::b notation in ControllerTrait docblock #26592

Merged

Tobion mentioned this pull request Apr 28, 2018

[HttpKernel] fix duplicate controller resolver test case #27085

Merged

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

robfrawley mentioned this pull request May 7, 2018

[Tests] [Deprecation] Updated bundle notation to accommodate Symfony 4.1 and set browser client to not catch exceptions liip/LiipImagineBundle#1090

Merged

Majkl578 reviewed Jun 6, 2018

View reviewed changes

dmaicher mentioned this pull request Jun 9, 2018

[FrameworkBundle] fix for allowing single colon controller notation #27566

Merged

vudaltsov mentioned this pull request Jul 6, 2018

[FrameworkBundle] Fixed phpdoc in MicroKernelTrait::configureRoutes() #27870

Merged

gregurco mentioned this pull request Jul 12, 2018

Replace deprecated configuration of routes with one colon markitosgv/JWTRefreshTokenBundle#104

Merged

teohhanhui mentioned this pull request Aug 7, 2018

Fix controller notation (single colon is deprecated) Sylius/Sylius#9631

Closed

nicolas-grekas mentioned this pull request May 6, 2019

[FrameworkBundle]: use __invoke function if no method is defined in r… #31367

Closed

Tobion mentioned this pull request May 6, 2019

[FrameworkBundle] Controller resolver doesn't inject container for invokable controller (no specified method in route) #31271

Closed

This was referenced May 30, 2019

[FrameworkBundle] fix test fixture using deprecated controller #31713

Merged

[FrameworkBundle] Remove support for the bundle:controller:action notation #31702

Merged

chapterjason mentioned this pull request Aug 2, 2021

test: Add routing tests SymfonyTest/symfony-bundle-test#45

Merged

delyriand mentioned this pull request Jan 29, 2024

Support Symfony 6 and drop Symfony 4 and PHP < 8.1 Setono/SyliusFeedPlugin#88

Merged


		if (1 === substr_count($controller, ':') && is_array($resolvedController)) {
		$resolvedController[0] = $this->configureController($resolvedController[0]);

Deprecate bundle:controller:action and service:method notation #26085

Deprecate bundle:controller:action and service:method notation #26085

Conversation

Tobion commented Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented Feb 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeahDude left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented Feb 20, 2018

fabpot commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented Feb 8, 2018 •

edited

Loading

Tobion Feb 8, 2018 •

edited

Loading

Tobion Feb 9, 2018 •

edited

Loading

Tobion Feb 12, 2018 •

edited

Loading