-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@GuilhemN Would you like to document the abstract controller introduced in symfony/symfony#22157 instead? Thanks! |
@HeahDude oh that's already merged, I didn't expect it to be so quick. I'll make the changes :) |
Updated to document the ping @nicolas-grekas |
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 |
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 previous Controller
." Which then raises "why did they tell me about this? do I miss anything?" leading to confusion.
my 2cts :)
Updated to document the |
controller.rst
Outdated
@@ -116,19 +116,26 @@ For more information on routing, see :doc:`/routing`. | |||
.. index:: | |||
single: Controller; Base controller class | |||
|
|||
The Base Controller Class & Services | |||
------------------------------------ | |||
The Base Controller Classes & 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.
@@ -241,10 +251,9 @@ are used for rendering templates, sending emails, querying the database and | |||
any other "work" you can think of. When you install a new bundle, it probably | |||
brings in even *more* services. | |||
|
|||
When extending the base controller 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 AbstractController Document symfony/symfony#18193. I'm not sure it should be merged right now though as it's an experimental feature. \cc @dunglas Commits ------- 4ac5da7 Fix 88a4806 Fix cf2ae91 [FrameworkBundle] Document the AbstractController
The Base Controller Class & Services | ||
------------------------------------ | ||
.. _anchor-name: | ||
:ref:`The Base Controller Classes & 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 this anchor-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 was a request from @xabbuh.
Thanks for the example, I never did that before and obviously didn't do it well by randomly trying :)
Document symfony/symfony#18193.
I'm not sure it should be merged right now though as it's an experimental feature.
\cc @dunglas