Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 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
Tobion commentedJun 21, 2014
Cannot be merged into 2.3 |
kbond commentedJun 21, 2014
Ok, I wondered about that - I guess it is a new feature. Do I need to re-submit? |
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 useelseif instead of adding another if inside theelse.
stof commentedJun 21, 2014
This should indeed go into master. |
kbond commentedJun 21, 2014
Ok, is it difficult for a merger to merge this into master or should I open a new PR? |
stof commentedJun 21, 2014
I think it can be moved automatically without conflict. This can has not changed in later releases IIRC |
stof commentedJun 21, 2014
👍 |
Tobion commentedJun 21, 2014
Please add a test for this, then I'm 👍 for merge into master |
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.
kbond commentedJun 21, 2014
OK. Will add. It looks like there are no tests for this class. Or am I missing something? |
kbond commentedJun 22, 2014
I added tests for |
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
romainneutron commentedJun 22, 2014
👍 for a merge in master |
fabpot commentedJun 26, 2014
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 ParamConverterListenerInvokable controller support was added to Symfony insymfony/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/AAs part of#11193 support for controllers using `__invoke()` was added.The `RequestDataCollector` did not support controllers that were defined in the routing as...```phproute_name: path: /{id} defaults: { _controller: acme_app.page.controller.page } requirements: id: \d+```Where the controller was defined as...```phpclass 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: