-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[FrameworkBundle] Simplify "invokable" controllers as services #11193
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
Conversation
Cannot be merged into 2.3 |
Ok, I wondered about that - I guess it is a new feature. Do I need to re-submit? |
@@ -64,6 +64,12 @@ protected function createController($controller) | |||
|
|||
return array($this->container->get($service), $method); | |||
} else { | |||
if ($this->container->has($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.
please use elseif
instead of adding another if inside the else
.
This should indeed go into master. |
Ok, is it difficult for a merger to merge this into master or should I open a new PR? |
I think it can be moved automatically without conflict. This can has not changed in later releases IIRC |
👍 |
Please add a test for this, then I'm 👍 for merge into master |
@@ -63,6 +63,10 @@ protected function createController($controller) | |||
list($service, $method) = explode(':', $controller, 2); | |||
|
|||
return array($this->container->get($service), $method); | |||
} elseif ($this->container->has($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.
Probably previous condition also must check whether container has such service.
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.
nope. The previous condition is explicitly a service syntax. Not checking if the container has a service means you get an exception telling you the service does not exist in case you made a typo.
OK. Will add. It looks like there are no tests for this class. Or am I missing something? |
I added tests for |
use Symfony\Component\HttpKernel\Tests\Controller\ControllerResolverTest as BaseControllerResolverTest; | ||
|
||
/** | ||
* @author Kevin Bond <kevinbond@gmail.com> |
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.
authors are not added to tests
👍 for a merge in master |
Thank you @kbond. |
…ler services (kbond) This PR was merged into the master branch. Discussion ---------- [Cookbook][Controller] Add note about invokable controller services | Q | A | ------------- | --- | Doc fix? | no | New docs? | yes (symfony/symfony#11193) | Applies to | 2.6+ | Fixed tickets | - Commits ------- 9879ee6 Add note about invokable controller services
…verterListener (jameshalsall) This PR was squashed before being merged into the 3.0.x-dev branch (closes #333). Discussion ---------- Adding support for invokable controllers to the ParamConverterListener Invokable controller support was added to Symfony in symfony/symfony#11193. The problem was that the `ParamConverterListener` did not support passing an object as the controller, this PR adds that support. Commits ------- 1b83587 Adding support for invokable controllers to the ParamConverterListener
… in the RequestDataCollector (jameshalsall) This PR was submitted for the master branch but it was merged into the 2.5 branch instead (closes #12443). Discussion ---------- [HttpKernel][2.6] Adding support for invokable controllers in the RequestDataCollector | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A As part of #11193 support for controllers using `__invoke()` was added. The `RequestDataCollector` did not support controllers that were defined in the routing as... ```php route_name: path: /{id} defaults: { _controller: acme_app.page.controller.page } requirements: id: \d+ ``` Where the controller was defined as... ```php class PageController { public function __invoke() { // } } ``` This PR adds that support. Tests have been updated. Commits ------- f1d043a [HttpKernel][2.6] Adding support for invokable controllers in the RequestDataCollector
Simplifies the configuration when defining controllers as services that implement
__invoke
.Old:
New: