8000 [FrameworkBundle] Simplify "invokable" controllers as services by kbond · Pull Request #11193 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[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

Closed
wants to merge 2 commits into from
Closed

[FrameworkBundle] Simplify "invokable" controllers as services #11193

wants to merge 2 commits into from

Conversation

kbond
Copy link
Member
@kbond kbond commented Jun 21, 2014
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR symfony/symfony-docs#3966

Simplifies the configuration when defining controllers as services that implement __invoke.

Old:

my_route:
    path: /path
    defaults: { _controller: my.controller.service:__invoke }

New:

my_route:
    path: /path
    defaults: { _controller: my.controller.service }

@Tobion
Copy link
Contributor
Tobion commented Jun 21, 2014

Cannot be merged into 2.3

@kbond
Copy link
Member Author
kbond commented Jun 21, 2014

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) &&
Copy link
Member

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.

@stof
Copy link
Member
stof commented Jun 21, 2014

This should indeed go into master.

@kbond
Copy link
Member Author
kbond commented Jun 21, 2014

Ok, is it difficult for a merger to merge this into master or should I open a new PR?

@stof
Copy link
Member
stof commented Jun 21, 2014

I think it can be moved automatically without conflict. This can has not changed in later releases IIRC

@stof
Copy link
Member
stof commented Jun 21, 2014

👍

@Tobion
Copy link
Contributor
Tobion commented Jun 21, 2014

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) &&
Copy link
Contributor

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.

Copy link
Member

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.

@kbond
Copy link
Member Author
kbond commented Jun 21, 2014

OK. Will add.

It looks like there are no tests for this class. Or am I missing something?

@kbond
Copy link
Member Author
kbond commented Jun 22, 2014

I added tests for Symfony/Bundle/FrameworkBundle/Controller/ControllerResolver - should cover this and all existing scenarios.

use Symfony\Component\HttpKernel\Tests\Controller\ControllerResolverTest as BaseControllerResolverTest;

/**
* @author Kevin Bond <kevinbond@gmail.com>
Copy link
Contributor

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

@romainneutron
Copy link
Contributor

👍 for a merge in master

@fabpot
Copy link
Member
fabpot commented Jun 26, 2014

Thank you @kbond.

@fabpot fabpot closed this in 579c465 Jun 26, 2014
weaverryan added a commit to symfony/symfony-docs that referenced this pull request Aug 16, 2014
…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
fabpot added a commit to sensiolabs/SensioFrameworkExtraBundle that referenced this pull request Nov 10, 2014
…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
fabpot added a commit that referenced this pull request Nov 12, 2014
… 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0