Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
[FrameworkBundle] Document the AbstractController#7657
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
HeahDude commentedMar 26, 2017
@GuilhemN Would you like to document the abstract controller introduced insymfony/symfony#22157 instead? Thanks! |
GuilhemN commentedMar 26, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@HeahDude oh that's already merged, I didn't expect it to be so quick. I'll make the changes :) |
GuilhemN commentedMar 26, 2017
Updated to document the ping@nicolas-grekas |
HeahDude commentedMar 26, 2017
Closes#7705. |
controller/service.rst Outdated
| usually pretty easy and the base `Controller class source code`_ is a great | ||
| source on how to perform many common tasks. | ||
| In case you don't want to inject the container in your controllers, | ||
| it's possible to get ride of controller shortcut methods: you can interact |
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 get rid of (no "e")
controller/service.rst Outdated
| # app/config/services.yml | ||
| services: | ||
| _instanceof: | ||
| Symfony\Bundle\FrameworkBundle\Controller\AbstractController: |
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 block is not needed, let's remove it from the doc
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.
In fact, there is no config to do at all, an AbstractController doesn't need to be a service at all
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 think it should be documented in a different section then?
Imo its main usage is using controllers as a service and having access to the shortcuts.
controller/service.rst Outdated
| Using the controller shortcuts | ||
| ------------------------------ | ||
| To use the traditional controller shortcuts, you can use the |
nicolas-grekasMar 26, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 paragraph is a bit strange to me, it doesn't really teach enough to the reader imho.
eg
if you prefer explicit injection (you can't access the container)
"why would I prefer explicit injection? esp when the words in the brackets suggest that this is less powerful, so let's just to the previousController." Which then raises "why did they tell me about this? do I miss anything?" leading to confusion.
my 2cts :)
GuilhemN commentedMar 31, 2017
Updated to document the |
| The Base ControllerClass & Services | ||
| ------------------------------------ | ||
| The Base ControllerClasses & Services |
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 should add a label for the old headline to not break any deep links
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.
Fixed, I'm not 100% sure this is how it works though.
| brings in even *more* services. | ||
| When extending the basecontroller class, you can access any Symfony service | ||
| When extending the base``Controller`` class, you can access any Symfony 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.
I think we're missing pros of using the new AbstractController, dependencies are explicit either in a constructor or in action signatures, but the controller needs to be configured to get injection.
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 mean we have to tell that the controller has to be configured as a service ? I updated the text to reflect that.
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.
Hmm it seems I got confused, reading@nicolas-grekas's comment above:
In fact, there is no config to do at all, an AbstractController doesn't need to be a service at all
The injection in this case is relying on an argument value resolver, so the controller does not need any configuration indeed.
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 I just said is wrong, the getters has nothing to do with action injections, so I don't get it, this must be configured somehow, it's just already done by the bundle.
…hemN)This PR was squashed before being merged into the master branch (closes#7657).Discussion----------[FrameworkBundle] Document the AbstractControllerDocumentsymfony/symfony#18193.I'm not sure it should be merged right now though as it's an experimental feature.\cc@dunglasCommits-------4ac5da7 Fix88a4806 Fixcf2ae91 [FrameworkBundle] Document the AbstractController
weaverryan commentedMay 2, 2017
| The Base Controller Class & Services | ||
| ------------------------------------ | ||
| .. _anchor-name: | ||
| :ref:`TheBaseControllerClasses&Services<the-base-controller-class-services>` |
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'm not familiar with thisanchor-name thing - were you trying to make sure the old anchor still worked? If so,#7867 for an example. I'll update this in my 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.
Yes that wasa request from @xabbuh.
Thanks for the example, I never did that before and obviously didn't do it well by randomly trying :)
Documentsymfony/symfony#18193.
I'm not sure it should be merged right now though as it's an experimental feature.
\cc@dunglas