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

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

Closed
wants to merge 14 commits into
base: master
from

Conversation

@Tobion
Member

Tobion commented Feb 8, 2018

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.

  • deprecate a:b:c
  • deprecate a:b
  • update existing references to a::b
  • fix tests
  • fix/add support for static controllers
  • add support for closures as controllers
  • update Symfony\Component\Routing\Loader\ObjectRouteLoader
  • deprecate \Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser but we still need to use it in several places for BC.
  • add changelog/upgrade
  • update controller.service_arguments logic for double colon controller syntax

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

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

This comment has been minimized.

@Tobion

Tobion Feb 8, 2018

Member

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.

$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 comment has been minimized.

@Tobion

Tobion Feb 8, 2018

Member

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.

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

This comment has been minimized.

@Tobion

Tobion Feb 8, 2018

Member

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

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

}
return $resolvedController;
return parent::createController($controller);

This comment has been minimized.

@fabpot

fabpot Feb 9, 2018

Member

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?

This comment has been minimized.

@mvrhov

mvrhov Feb 9, 2018

Contributor

You mean 4.1 and 5.0 right :)

This comment has been minimized.

@Tobion

Tobion Feb 9, 2018

Member

@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.

This comment has been minimized.

@fabpot

fabpot Feb 9, 2018

Member

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

This comment has been minimized.

@stof

stof Feb 9, 2018

Member

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

This comment has been minimized.

@Tobion

Tobion Feb 11, 2018

Member

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.

@@ -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]);

This comment has been minimized.

@stof

stof Feb 9, 2018

Member

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

This comment has been minimized.

@Tobion

Tobion Feb 9, 2018

Member

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.

This comment has been minimized.

@Tobion

Tobion Feb 9, 2018

Member

implemented

}
return $resolvedController;
return parent::createController($controller);

This comment has been minimized.

@stof

stof Feb 9, 2018

Member

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

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

@Tobion

This comment has been minimized.

Member

Tobion commented Feb 11, 2018

Tests should pass once merged.

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

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

This comment has been minimized.

@lyrixx

lyrixx Feb 12, 2018

Member

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

This comment has been minimized.

@Tobion

Tobion Feb 12, 2018

Member

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

This comment has been minimized.

@lyrixx

lyrixx Feb 12, 2018

Member

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

This comment has been minimized.

@Tobion

Tobion Feb 12, 2018

Member

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.

@@ -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);

This comment has been minimized.

@ro0NL

ro0NL Feb 12, 2018

Contributor

why not put the trigger on top of the file?

This comment has been minimized.

@Tobion

Tobion Feb 12, 2018

Member

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

@@ -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.

This comment has been minimized.

@ro0NL

ro0NL Feb 12, 2018

Contributor

, to be removed in Symfony 5.0

This comment has been minimized.

@Tobion

Tobion Feb 12, 2018

Member

I normalized all deprecation messages

@@ -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);

This comment has been minimized.

@ro0NL

ro0NL Feb 12, 2018

Contributor

"%s" quoted + Symfony 5.0

@@ -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.

This comment has been minimized.

@ro0NL

ro0NL Feb 12, 2018

Contributor

to be applied on 4.0?

This comment has been minimized.

@Tobion

Tobion Feb 12, 2018

Member

I guess also for 3.4 but no priority for me

@HeahDude

Great!

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

This comment has been minimized.

@HeahDude

HeahDude Feb 12, 2018

Member

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.

This comment has been minimized.

@Tobion

Tobion Feb 12, 2018

Member

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

@@ -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);

This comment has been minimized.

@HeahDude

HeahDude Feb 12, 2018

Member

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

This comment has been minimized.

@Tobion

Tobion Feb 12, 2018

Member

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

@chalasr

This comment has been minimized.

Member

chalasr commented Feb 13, 2018

The controller_name_converter and resolve_controller_name_subscriber services should be deprecated.

@Tobion

This comment has been minimized.

Member

Tobion commented Feb 13, 2018

I can't deprecate them because we still need to use them internally.

$resolvedController = parent::createController($controller);
$deprecatedNotation = $controller;
$controller = $this->parser->parse($deprecatedNotation);

This comment has been minimized.

@chalasr

chalasr Feb 15, 2018

Member

will give an extra notice about the deprecated class being used, right? I would change ControllerNameParser to trigger from its constructor only (muting the notice using an implicit constructor arg for internal uses)

This comment has been minimized.

@chalasr

chalasr Feb 15, 2018

Member

or disable the notice in the method call

This comment has been minimized.

@Tobion
@@ -10,6 +10,9 @@ CHANGELOG
* Added option in workflow dump command to label graph with a custom label
* Using a `RouterInterface` that does not implement the `WarmableInterface` is deprecated and will not be supported in Symfony 5.0.
* The `RequestDataCollector` class has been deprecated and will be removed in Symfony 5.0. Use the `Symfony\Component\HttpKernel\DataCollector\RequestDataCollector` class instead.
* Deprecated `bundle:controller:action` syntax to reference controllers. Use `serviceOrFqcn::method` instead where `serviceOrFqcn`
is either the service ID or the FQCN of the controller.
* Deprecated `Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser`

This comment has been minimized.

@fabpot

fabpot Feb 19, 2018

Member

This means that the UPGRADE-5.0 file is missing some information.

'Referencing controllers with %s is deprecated since Symfony 4.1. Use %s instead.',
$deprecatedNotation,
$controller
), E_USER_DEPRECATED);

This comment has been minimized.

@fabpot

fabpot Feb 19, 2018

Member

This should be on one line.

'Referencing controllers with %s is deprecated since Symfony 4.1. Use %s instead.',
$deprecatedNotation,
$controller
), E_USER_DEPRECATED);

This comment has been minimized.

@fabpot

fabpot Feb 19, 2018

Member

On one line as well.

@trigger_error(sprintf(
'Referencing controllers with a single colon is deprecated since Symfony 4.1. Use %s instead.',
$controller
), E_USER_DEPRECATED);

This comment has been minimized.

@fabpot

fabpot Feb 19, 2018

Member

Same here

@@ -659,7 +659,7 @@ private function addValidationSection(ArrayNodeDefinition $rootNode)
->beforeNormalization()
->ifTrue(function ($v) { return isset($v['strict_email']); })
->then(function ($v) {
@trigger_error('The "framework.validation.strict_email" configuration key has been deprecated in Symfony 4.1. Use the "framework.validation.email_validation_mode" configuration key instead.', E_USER_DEPRECATED);
@trigger_error('The "framework.validation.strict_email" configuration key is deprecated since Symfony 4.1. Use the "framework.validation.email_validation_mode" configuration key instead.', E_USER_DEPRECATED);

This comment has been minimized.

@fabpot

fabpot Feb 19, 2018

Member

Unrelated changes should really be moved to another PR. This is especially important here as people will have a look at this PR when upgrading.

@@ -70,7 +70,7 @@
</service>
<service id="session.save_listener" class="Symfony\Component\HttpKernel\EventListener\SaveSessionListener">
<deprecated>The "%service_id%" service is deprecated since Symfony 4.1 and will be removed in 5.0. Use the "session_listener" service instead.</deprecated>
<deprecated>The "%service_id%" service is deprecated since Symfony 4.1. Use the "session_listener" service instead.</deprecated>

This comment has been minimized.

@fabpot

fabpot Feb 19, 2018

Member

This should be part of another PR as not related to this one.

@@ -145,13 +145,13 @@ public function guessClientExtension()
* It is extracted from the request from which the file has been uploaded.
* Then it should not be considered as a safe value.
*
* @deprecated since 4.1 will be removed in 5.0 use getSize() instead.
* @deprecated since Symfony 4.1, to be removed in 5.0. Use getSize() instead.

This comment has been minimized.

@fabpot

fabpot Feb 19, 2018

Member

Same here, should be moved to another PR.

This comment has been minimized.

@Tobion

Tobion Feb 19, 2018

Member

Luckily I saw you already did that in #26225

@@ -6,6 +6,7 @@ CHANGELOG
* added orphaned events support to `EventDataCollector`
* `ExceptionListener` now logs and collects exceptions at priority `2048` (previously logged at `-128` and collected at `0`)
* Deprecated `service:action` syntax with a single colon to reference controllers. Use `service::method` instead.

This comment has been minimized.

@fabpot

fabpot Feb 19, 2018

Member

should also be part of UPGRADE-5.0.md.

if ($e instanceof \ArgumentCountError) {
throw new \InvalidArgumentException(
sprintf('Controller "%s" has required constructor arguments and does not exist in the container. Did you forget to define such a service?', $class), 0, $e
);

This comment has been minimized.

@fabpot

fabpot Feb 19, 2018

Member

one line

throw new \LogicException(sprintf('Controller "%s" cannot be fetched from the container because it is private. Did you forget to tag the service with "controller.service_arguments"?', $controller), 0, $previous);
throw new \InvalidArgumentException(
sprintf('Controller "%s" cannot be fetched from the container because it is private. Did you forget to tag the service with "controller.service_arguments"?', $controller), 0, $previous
);

This comment has been minimized.

@fabpot

fabpot Feb 19, 2018

Member

one line

@Tobion

This comment has been minimized.

Member

Tobion commented Feb 19, 2018

@fabpot fixed your requests

@fabpot

fabpot approved these changes Feb 20, 2018

@Tobion

This comment has been minimized.

Member

Tobion commented Feb 20, 2018

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

@fabpot

This comment has been minimized.

Member

fabpot commented Feb 21, 2018

Thank you @Tobion.

@fabpot fabpot closed this Feb 21, 2018

fabpot added a commit that referenced this pull request Feb 21, 2018

feature #26085 Deprecate bundle:controller:action and service:method …
…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🅱️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

@Tobion Tobion deleted the Tobion:deprecate-old-controller-notations branch Feb 21, 2018

@Tobion Tobion referenced this pull request Feb 24, 2018

Closed

[HttpKernel] [DX] Configurable controller layout #25422

0 of 2 tasks complete

nicolas-grekas added a commit that referenced this pull request Mar 19, 2018

minor #26592 [FrameworkBundle] Use `a::b` notation in ControllerTrait…
… 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

<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
 - Bug fixes must be submitted against the lowest branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

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

nicolas-grekas added a commit that referenced this pull request Apr 29, 2018

minor #27085 [HttpKernel] fix duplicate controller resolver test case…
… (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 <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #26085
| License       | MIT
| Doc PR        |

Commits
-------

fc5afea fix duplicate controller resolver test case

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

robfrawley added a commit to robfrawley/liip-imagine-bundle that referenced this pull request May 7, 2018

Updated bundle notation to accomodate symfony 4.1 deprecations and ch…
…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>
}
if (1 === substr_count($controller, ':')) {
$controller = str_replace(':', '::', $controller);

This comment has been minimized.

@Majkl578

Majkl578 Jun 6, 2018

Contributor

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

This comment has been minimized.

@Tobion

Tobion Jun 6, 2018

Member

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

This comment has been minimized.

@Majkl578

Majkl578 Jun 6, 2018

Contributor

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.

fabpot added a commit that referenced this pull request Jun 11, 2018

bug #27566 [FrameworkBundle] fix for allowing single colon controller…
… 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment