More "id" => type hints / autowiring changes #7883

weaverryan · 2017-05-06T15:06:32Z

Hi 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!

GuilhemN

First quick review ☺

GuilhemN · 2017-05-07T10:29:54Z

controller/error_pages.rst


-    And then configure ``twig.exception_controller`` using the controller as
-    services syntax (e.g. ``app.exception_controller:showAction``).
+            $$container->autowire(CustomExceptionController::class)


GuilhemN · 2017-05-07T10:31:37Z

controller/soap_web_service.rst

+                http://symfony.com/schema/dic/services/services-1.0.xsd">
+
+            <services>
+                <defaults autowire="true" autoconfigure="true" />


Autoconfigure should be enabled in the yaml file or removed here imo

GuilhemN · 2017-05-07T10:32:17Z

controller/soap_web_service.rst


    use Symfony\Bundle\FrameworkBundle\Controller\Controller;
    use Symfony\Component\HttpFoundation\Response;
+    use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route;
+    use AppBundle\Service\HelloService;


Shouldn't use statements be ordered ?

I actually don't know, I didn't see anything (looked quickly) in the docs contributor page, but I could be wrong

Me neither but I often see them ordered in symfony's code.

Ah, they do follow that standard in core. I suppose we could do, but I'll worry about that later ;)

GuilhemN · 2017-05-07T10:42:16Z

controller/argument_value_resolver.rst

+            # ...
+
+            AppBundle\ArgumentResolver\UserValueResolver:
+                # arguments is not needed if you have autowire above under _defaults


Shouldn't we have a _defaults section then ?

This is the balance I'm thinking about a lot. We have some options:

Assume the user has the out-of-the-box _defaults settings, with autowire and autoconfigure. In that case, all code examples like this one are super simple

Don'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.

I personally prefer having it everywhere, this would limit situations hard to debug for new comers.

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?

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, remove public and autoconfigure). I'm interested in using the diff type for this (there's not a ton 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 :)

GuilhemN · 2017-05-07T10:42:55Z

controller/error_pages.rst

-    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%');


GuilhemN · 2017-05-07T10:43:51Z

controller/soap_web_service.rst

-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:


I would precise that it's "enabled by default in the standard edition"

GuilhemN · 2017-05-07T10:45:53Z

controller/soap_web_service.rst

-WSDL document can be retrieved via ``/soap?wsdl``.
-
-.. code-block:: php
+request. BEcause ``indexAction()`` is accessible via ``/soap``, the WSDL document


Typo because

GuilhemN · 2017-05-07T10:46:52Z

controller/upload_file.rst

@@ -221,8 +221,8 @@ Creating an Uploader Service
 To avoid logic in controllers, making them big, you can extract the upload
 logic to a separate service::

-    // src/AppBundle/FileUploader.php
-    namespace AppBundle;
+    // src/AppBundle/Service\FileUploader.php


GuilhemN · 2017-05-07T10:49:56Z

controller/upload_file.rst


    // ...
-    public function newAction(Request $request)
+    public function newAction(Request $request, FileUploader $fileUploader)


What about adding a use statement for Request too?

GuilhemN · 2017-05-07T11:58:54Z

controller/soap_web_service.rst

-                class: Acme\SoapBundle\Services\HelloService
-                arguments: ['@mailer']
+            _defaults:
+                # be sure autowire is enabled


autowiring?

GuilhemN · 2017-05-07T12:04:22Z

doctrine/associations.rst

-    public function findOneByIdJoinedToCategory($productId)
+    use Doctrine\ORM\EntityManagerInterface;
+
+    public function findOneByIdJoinedToCategory($productId, EntityManagerInterface $em)


This one should be reverted, it's a repository 😉

GuilhemN · 2017-05-07T12:05:43Z

doctrine/event_listeners_subscribers.rst

            <services>
-                <service id="my.listener" class="AppBundle\EventListener\SearchIndexer">
+                <!-- ... --


missing closing >

GuilhemN · 2017-05-07T12:07:02Z

doctrine/multiple_entity_managers.rst

@@ -204,27 +208,25 @@ entity manager to persist and fetch its entities.

 The same applies to repository calls::

+    use Doctrine\Common\Persistence\ManagerRegistry;
+
    class UserController extends Controller


Shouldn't we begin using AbstractController?

I've been using Controller 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 using AbstractController.

GuilhemN · 2017-05-07T12:07:38Z

doctrine/pdo_session_storage.rst

@@ -110,11 +110,10 @@ a second array argument to ``PdoSessionHandler``:
        use Symfony\Component\HttpFoundation\Session\Storage\Handler\PdoSessionHandler;
        // ...

-        $storageDefinition = new Definition(PdoSessionHandler::class, array(
+        $container->register(PdoSessionHandler::class)->setArguments(array(


I would add a line break here

GuilhemN · 2017-05-07T12:07:51Z

doctrine/pdo_session_storage.rst

@@ -170,7 +170,7 @@ of your project's data, you can use the connection settings from the
    .. code-block:: php

        // ...
-        $storageDefinition = new Definition(PdoSessionHandler::class, array(
+        $container->register(PdoSessionHandler::class)->setArguments(array(


GuilhemN · 2017-05-07T12:08:01Z

doctrine/registration_form.rst

@@ -225,13 +225,15 @@ into the database::
    use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route;
    use Symfony\Bundle\FrameworkBundle\Controller\Controller;
    use Symfony\Component\HttpFoundation\Request;
+    use Symfony\Component\Security\Core\Encoder\UserPasswordEncoderInterface;
+    use Doctrine\ORM\EntityManagerInterface;


javiereguiluz · 2017-05-07T17:38:57Z

controller/soap_web_service.rst

+                # be sure autowire is enabled
+                autowire: true
+
+            # load services from the Service directory


Not sure, but:

# load services from the Service directory

-->

# add Service/ to the list of directories to load services from

javiereguiluz · 2017-05-07T17:40:50Z

event_dispatcher.rst

+    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``.


[...] 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.

javiereguiluz · 2017-05-07T17:44:25Z

controller/argument_value_resolver.rst

+            # ...
+
+            AppBundle\ArgumentResolver\UserValueResolver:
+                # arguments is not needed if you have autowire above under _defaults


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?

weaverryan · 2017-05-08T09:55:55Z

controller/soap_web_service.rst

+                # be sure autowire is enabled
+                autowire: true
+
+            # load services from the Service directory


I like your's better... at the very least because it has the important word - Service earlier.

GuilhemN · 2017-05-08T10:01:06Z

doctrine/associations.rst

@@ -369,7 +369,7 @@ following method to the ``ProductRepository`` class::
    // src/AppBundle/Repository/ProductRepository.php
    use Doctrine\ORM\EntityManagerInterface;

-    public function findOneByIdJoinedToCategory($productId, EntityManagerInterface $em)
+    public function findOneByIdJoinedToCategory($productId)
    {
        $query = $em->createQuery(


Should be fixed now 😉

This PR was merged into the master branch. Discussion ---------- More "id" => type hints / autowiring changes Hi 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

carsonbot added the Status: Needs Review label May 6, 2017

GuilhemN reviewed May 7, 2017

View reviewed changes

javiereguiluz reviewed May 7, 2017

View reviewed changes

weaverryan commented May 8, 2017

View reviewed changes

GuilhemN reviewed May 8, 2017

View reviewed changes

weaverryan mentioned this pull request May 10, 2017

Example of the Doctrine doc with types as the first-class citizen #7876

Closed

Going through more chapters to use types and autowiring

e1d36e6

weaverryan merged commit e1d36e6 into symfony:master May 10, 2017

weaverryan deleted the more-types branch May 10, 2017 09:57

robfrawley mentioned this pull request Jun 7, 2017

[WIP] Document "framework.assets.json_manifest_path" option #8004

Closed

Uh oh!

More "id" => type hints / autowiring changes #7883

More "id" => type hints / autowiring changes #7883

Uh oh!

Conversation

weaverryan commented May 6, 2017

Uh oh!

GuilhemN left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!