Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
More "id" => type hints / autowiring changes#7883
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
GuilhemN left a comment
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.
First quick review ☺
controller/error_pages.rst Outdated
| And then configure ``twig.exception_controller`` using the controller as | ||
| services syntax (e.g. ``app.exception_controller:showAction``). | ||
| $$container->autowire(CustomExceptionController::class) |
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.
Typo
controller/soap_web_service.rst Outdated
| http://symfony.com/schema/dic/services/services-1.0.xsd"> | ||
| <services> | ||
| <defaultsautowire="true"autoconfigure="true" /> |
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.
Autoconfigure should be enabled in the yaml file or removed here imo
| use Symfony\Bundle\FrameworkBundle\Controller\Controller; | ||
| use Symfony\Component\HttpFoundation\Response; | ||
| use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route; | ||
| use AppBundle\Service\HelloService; |
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.
Shouldn't use statements be ordered ?
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 actually don't know, I didn't see anything (looked quickly) in the docs contributor page, but I could be wrong
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.
Me neither but I often see them ordered in symfony's code.
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.
Ah, theydo follow that standard in core. I suppose we could do, but I'll worry about that later ;)
| # ... | ||
| AppBundle\ArgumentResolver\UserValueResolver: | ||
| # arguments is not needed if you have autowire above under _defaults |
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.
Shouldn't we have a_defaults section then ?
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 is the balance I'm thinking about a lot. We have some options:
Assume the user has the out-of-the-box
_defaultssettings, withautowireandautoconfigure. In that case, all code examples like this one are super simpleDon't assume the user has the correct
_defaults, and show it at the top of every code block (or, add some comment like this one).
This is really a global decision to make - I'm already solving it slightly different in different places, as I'm not 100% sure.
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 personally prefer having it everywhere, this would limit situations hard to debug for new comers.
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.
A while ago I agreed with Ryan's idea ... but lately I'm aligning with Guilhem's idea.
Imagine that you land in this page and you see that message:
"arguments is not needed if you have autowire above under _defaults"
Well ... which "above" are we talking about? Which "_defaults" (whatever that is) are we talking about? etc.
This is truly critical because it's not only for newcomers ... but also for"newcomers to the new DI features" (I'm one of those 😊 ).
But of course Ryan is right and we can't add this continuously ... at least not by hand. Would some global include or custom directive help us automating this?
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 don't mind putting it everywhere :). I just want the absolute best message. My last commit - sha:ab4de02 - gives a uniform handling of this (for the files in this PR).
The big risk of confusion now is people thinking that they need to add_defaults to their file (e.g. so they have 2_defaults) or make their_defaults look exactly like our's (so, removepublic andautoconfigure). I'm interested in using thediff type for this (there's not aton of highlighting), but I really want#7806 first (if my new comment is possible, it would be AMAZING!). I haven't seen@wouterj in a bit though... I'm trying to ping and bother him :)
controller/error_pages.rst Outdated
| And then configure ``twig.exception_controller`` using the controller as | ||
| services syntax (e.g. ``app.exception_controller:showAction``). | ||
| $$container->autowire(CustomExceptionController::class) | ||
| setArgument('$debug', '%kernel.debug%'); |
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.
Typo
controller/soap_web_service.rst Outdated
| instance. Using the Service Container, you can configure Symfony to construct | ||
| a ``HelloService`` object properly: | ||
| Next, make sure that your new class is registered as a service. If you use | ||
| :doc:`autowiring</service_container/autowiring>` (enabled by default), this is easy: |
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 would precise that it's "enabled by defaultin the standard edition"
controller/soap_web_service.rst Outdated
| WSDL document can be retrieved via ``/soap?wsdl``. | ||
| ..code-block::php | ||
| request. BEcause ``indexAction()`` is accessible via ``/soap``, the WSDL document |
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.
Typobecause
controller/upload_file.rst Outdated
| // src/AppBundle/FileUploader.php | ||
| namespace AppBundle; | ||
| // src/AppBundle/Service\FileUploader.php |
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.
Typo
| // ... | ||
| public function newAction(Request $request) | ||
| public function newAction(Request $request, FileUploader $fileUploader) |
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 about adding a use statement forRequest too?
controller/soap_web_service.rst Outdated
| class:Acme\SoapBundle\Services\HelloService | ||
| arguments:['@mailer'] | ||
| _defaults: | ||
| # be sure autowire is enabled |
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.
autowiring?
doctrine/associations.rst Outdated
| public function findOneByIdJoinedToCategory($productId) | ||
| use Doctrine\ORM\EntityManagerInterface; | ||
| public function findOneByIdJoinedToCategory($productId, EntityManagerInterface $em) |
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 one should be reverted, it's a repository 😉
| <services> | ||
| <serviceid="my.listener"class="AppBundle\EventListener\SearchIndexer"> | ||
| <!-- ... -- |
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.
missing closing>
| use Doctrine\Common\Persistence\ManagerRegistry; | ||
| class UserController extends 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.
Shouldn't we begin usingAbstractController?
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've been usingController everywhere to ease the transition to the "type" based model. I think if we switch to it too fast, we'll have problems with people doing$this->get('...') while reading blog posts, README's, etc and it not working. And there's really not much profit in usingAbstractController.
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.
Ok 👍
doctrine/pdo_session_storage.rst Outdated
| // ... | ||
| $storageDefinition = new Definition(PdoSessionHandler::class,array( | ||
| $container->register(PdoSessionHandler::class)->setArguments(array( |
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 would add a line break here
doctrine/pdo_session_storage.rst Outdated
| // ... | ||
| $storageDefinition = new Definition(PdoSessionHandler::class,array( | ||
| $container->register(PdoSessionHandler::class)->setArguments(array( |
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.
same here
| use Symfony\Bundle\FrameworkBundle\Controller\Controller; | ||
| use Symfony\Component\HttpFoundation\Request; | ||
| use Symfony\Component\Security\Core\Encoder\UserPasswordEncoderInterface; | ||
| use Doctrine\ORM\EntityManagerInterface; |
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.
order?
controller/soap_web_service.rst Outdated
| # be sure autowire is enabled | ||
| autowire:true | ||
| # load services from the Service directory |
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.
Not sure, but:
# load services from the Service directory-->
# add Service/ to the list of directories to load services fromevent_dispatcher.rst Outdated
| If your methods are *not* called when an exception is thrown, double-check that | ||
| you're:ref:`loading services<service-container-services-load-example>` from | ||
| the ``EventSubscriber`` directory and have:ref:`autoconfigure<services-autoconfigure>` | ||
| enabled. You can also manually tag with ``kernel.event_subscriber``. |
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.
[...] tag with ``kernel.event_subscriber``-->
[...] add the ``kernel.event_subscriber`` tag.I know both look the same, but to me the second is easier to understand what should I do to fix this issue.
| # ... | ||
| AppBundle\ArgumentResolver\UserValueResolver: | ||
| # arguments is not needed if you have autowire above under _defaults |
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.
A while ago I agreed with Ryan's idea ... but lately I'm aligning with Guilhem's idea.
Imagine that you land in this page and you see that message:
"arguments is not needed if you have autowire above under _defaults"
Well ... which "above" are we talking about? Which "_defaults" (whatever that is) are we talking about? etc.
This is truly critical because it's not only for newcomers ... but also for"newcomers to the new DI features" (I'm one of those 😊 ).
But of course Ryan is right and we can't add this continuously ... at least not by hand. Would some global include or custom directive help us automating this?
controller/soap_web_service.rst Outdated
| # be sure autowire is enabled | ||
| autowire:true | ||
| # load services from the Service directory |
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 like your's better... at the very least because it has the important word -Service earlier.
| public function findOneByIdJoinedToCategory($productId, EntityManagerInterface $em) | ||
| public function findOneByIdJoinedToCategory($productId) | ||
| { | ||
| $query = $em->createQuery( |
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.
Should be fixed now 😉
This PR was merged into the master branch.Discussion----------More "id" => type hints / autowiring changesHi guys!I'm going through the docs to update things to the 3.3 features: autowiring, service `resource` loading, etc. This is just another step in this fairly significant change, so thoughts are warmly welcomed (and review is needed!)Thanks!Commits-------e1d36e6 Going through more chapters to use types and autowiring
Hi guys!
I'm going through the docs to update things to the 3.3 features: autowiring, service
resourceloading, etc. This is just another step in this fairly significant change, so thoughts are warmly welcomed (and review is needed!)Thanks!