Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Deprecate bundle:controller:action and service:method notation#26085
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| $resolvedController =parent::createController($controller); | ||
| if (1 ===substr_count($controller,':') &&is_array($resolvedController)) { | ||
| $resolvedController[0] =$this->configureController($resolvedController[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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-schoolnew $class.
| } | ||
| if (!is_callable($controller)) { | ||
| thrownew \InvalidArgumentException(sprintf('The controller for URI "%s" is not callable. %s',$request->getPathInfo(),$this->getControllerError($controller))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this array controller case was missing validation. otherwise it fails in the kernel with a type error at
| publicfunction__construct(HttpKernelInterface$kernel,callable$controller,Request$request, ?int$requestType) |
4e2d6ef tof1d6484Compare| } | ||
| return$resolvedController; | ||
| returnparent::createController($controller); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You mean 4.1 and 5.0 right :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@fabpot we still need to class for theContainerAwareInterface logic, seeconfigureController. 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Indeed, I missed that. It could be deprecated at some point, but definitely not part of this work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can't we move the ContainerAwareInterface logic to the ContainerControllerResolver of the component ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| if (is_array($controller)) { | ||
| if (isset($controller[0]) &&is_string($controller[0])) { | ||
| $controller[0] =$this->instantiateController($controller[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
wouldn't this break support for using static methods as controller ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah I also thought about this. We don't have tests using static methods andclass::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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
implemented
| } | ||
| return$resolvedController; | ||
| returnparent::createController($controller); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can't we move the ContainerAwareInterface logic to the ContainerControllerResolver of the component ?
252cbcc to0632c6bCompareTobion commentedFeb 11, 2018
Tests should pass once merged. |
a7d6d0a to23c999dCompare| } | ||
| } | ||
| if (!is_callable($controller)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about usinghttp://php.net/manual/en/closure.fromcallable.php to ensure it's callable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't know what you intend. In which way does Closure::fromCallable help here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
TheClosure::fromCallable method callsis_callable for you.
Then, It "standardize" how we (will) manage callable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Using a closure here provides no benefit and the error it would return for wrong callables is worse than whatgetControllerError does. For example it does not hint that an __invoke method might be missing.https://3v4l.org/fJJNs
ro0NL left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
just some random comments :) 👍 another great step forward.
| */ | ||
| publicfunctionparse($controller) | ||
| { | ||
| @trigger_error(sprintf('The %s class is deprecated since version 4.1 and will be removed in 5.0.',__CLASS__),E_USER_DEPRECATED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why not put the trigger on top of the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
because we still use the class and service internally. but we only want to trigger when it gets executed
| * | ||
| * @author Fabien Potencier <fabien@symfony.com> | ||
| * | ||
| * @deprecated since version 4.1, will be removed in 5.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
, to be removed in Symfony 5.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I normalized all deprecation messages
| */ | ||
| publicfunctionbuild($controller) | ||
| { | ||
| @trigger_error(sprintf('The %s class is deprecated since version 4.1 and will be removed in 5.0.',__CLASS__),E_USER_DEPRECATED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"%s" quoted +Symfony 5.0
| * 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
to be applied on 4.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess also for 3.4 but no priority for me
HeahDude left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Great!
| } | ||
| } | ||
| if (1 ===substr_count($controller,':')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You could use anelseif here if the condition is mutually exclusive with the one above. It would prevent evaluating this one when the first is true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Depends if someone used an extended name parser that returned something else. I would leave it like this for simplicitly.
| $reason =false; | ||
| $action =substr(strrchr($controller,':'),1); | ||
| $id =substr($controller,0, -1 -strlen($action)); | ||
| list($id,$action) =explode('::',$controller); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about using[$id, $action] here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We could but I don't think we use that syntax in the code base yet.
b3fe6f0 to748f725CompareTobion commentedFeb 20, 2018
Can we merge this? I'd prefer to not having to rebase this again. |
fabpot commentedFeb 21, 2018
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 syntaxCommits-------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<!--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
… (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 <!-- seehttps://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
…angedbrowser 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#22890Signed-off-by: Rob Frawley 2nd <rmf@src.run>
| } | ||
| if (1 ===substr_count($controller,':')) { | ||
| $controller =str_replace(':','::',$controller); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Unfortunately this change is a BC break and breaks custom code relying on single colon notation. (Simply commenting this out fixes the BC break.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I see. Removing this is an option. Do you want to create a PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
… 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@TobionCommits-------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 <!-- please update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| 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
Uh oh!
There was an error while loading.Please reload this page.
The
a::bnotation had some awkward limitations. It supportedMyControllerClass::methodwhereMyControllerClassis either plain class or a service with the same name but the class must exists. This meant it did NOT supportmy_service_controller_id::methodbecause theclass_existscheck 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::bnotation to be very straight forward:aexists as a service then useaas a serviceaas a class, i.e.new $a()::string syntax, like in php itself. since it only happens when nothing else works, it does not have any performance impact.The old
a:bsyntax is deprecated and just forwards toa::bnow internally, just as bundle:controller:action.In general I was able to refactor the logic quite a bit because it always goes through
instantiateControllernow.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.
a:b:ca:ba::b