Updated the application to Symfony 3.3.0 #562

javiereguiluz · 2017-05-22T07:54:47Z

This finishes #483 and uses the all the new 3.3 features for DI services.

javiereguiluz · 2017-05-22T07:55:20Z

Commands still extend from ContainerAwareCommand and get services with $container->get() What can we do about that? Thanks!

weaverryan · 2017-05-22T13:47:25Z

@javiereguiluz I'm a bit undecided about commands (what to officially recommend). In the docs, I've left the $this->getContainer()->get() in the commands, and just mentioned that you can use DI normally now that these are services. So, the answer would be, if you want, you can absolutely add a __construct() method to these commands and use normal DI.

yceruto · 2017-05-22T13:50:51Z

app/config/services.yml

+    AppBundle\Controller\:
+        resource: '../../src/AppBundle/Controller'
+        public: true
+        tags: ['controller.service_arguments']


IMO this is not needed if our controllers extend from Controller so it's already auto-tagged.

You're right... but it's nice to be consistent with the official project skeleton

yceruto · 2017-05-22T14:07:08Z

We could use AbstractController instead?

stof · 2017-05-22T14:02:33Z

app/config/services.yml

+        resource: '../../src/AppBundle/*'
+        # you can exclude directories or files
+        # but if a service is unused, it's removed anyway
+        exclude: '../../src/AppBundle/{Entity,Repository}'


you should also exclude Controller here IMO. It is a waste of resources to include them all here to overwrite them all just below

stof · 2017-05-22T14:03:13Z

app/config/services.yml

+            $locales: '%app_locales%'
+            $defaultLocale: '%locale%'
+
+    Twig_Extensions_Extension_Intl:
        public: false


public: false is useless due to the default

stof · 2017-05-22T14:05:09Z

app/config/services.yml

+            $locales: '%app_locales%'
+            $defaultLocale: '%locale%'
+
+    Twig_Extensions_Extension_Intl:
        public: false
        class:  Twig_Extensions_Extension_Intl
        tags:


the tag is useless to, as you have autoconfigure: true in the defaults of the files

yceruto · 2017-05-22T14:13:35Z

app/config/config.yml

@@ -42,7 +42,6 @@ framework:
        engines: ['twig']
    default_locale:  "%locale%"
    trusted_hosts:   ~
-    trusted_proxies: ~
    session:
        handler_id:  session.handler.native_file
        save_path:   "%kernel.root_dir%/../var/sessions/%kernel.environment%"


'%kernel.project_dir%/var/sessions/%kernel.environment%' ?

yceruto · 2017-05-22T14:15:07Z

app/config/config.yml

@@ -42,7 +42,6 @@ framework:
        engines: ['twig']
    default_locale:  "%locale%"


Single quote in yamls?

yceruto · 2017-05-22T14:22:01Z

src/AppBundle/Controller/Admin/BlogController.php

@@ -86,7 +87,7 @@ public function newAction(Request $request)
        // However, we explicitly add it to improve code readability.
        // See https://symfony.com/doc/current/best_practices/forms.html#handling-form-submits
        if ($form->isSubmitted() && $form->isValid()) {
-            $post->setSlug($this->get('slugger')->slugify($post->getTitle()));
+            $post->setSlug($slugger->slugify($post->getTitle()));

            $entityManager = $this->getDoctrine()->getManager();


Also we could inject em for each action if needed https://github.com/symfony/symfony-docs/pull/7933/files#diff-e530d0758aab398d19d2ed98bf9d6715R107 or maybe better if it's used frequently inject it into __constructor?

Or why not extend from AbstractController and keep this as it?

FYI - the recommended practice for getting the EM is being discussed here: symfony/symfony-docs#7933 (comment)

javiereguiluz · 2017-05-22T14:53:57Z

I've updated commands to use DI too. Please let me know other places where we can use the new features. Thanks!

yceruto · 2017-05-22T19:35:30Z

src/AppBundle/Controller/Admin/BlogController.php

-            $entityManager = $this->getDoctrine()->getManager();
-            $entityManager->persist($post);
-            $entityManager->flush();
+            $em->persist($post);


$em missing injection?

GuilhemN · 2017-05-22T19:43:55Z

src/AppBundle/Command/ListUsersCommand.php

    private $entityManager;
+    private $mailer;


missing $emailSender property

GuilhemN · 2017-05-22T19:46:12Z

src/AppBundle/Controller/Admin/BlogController.php

@@ -134,19 +135,16 @@ public function showAction(Post $post)
     * @Route("/{id}/edit", requirements={"id": "\d+"}, name="admin_post_edit")
     * @Method({"GET", "POST"})
     */
-    public function editAction(Post $post, Request $request)
+    public function editAction(Request $request, EntityManagerInterface $em, Post $post, Slugger $slugger)


So we order arguments this way: Request>$em>entities>other dependencies?

I don't know. To me Request and the Doctrine thing look better at the beginning. But this is just an opinion.

I see Doctrine as a regular dependency so I would put it at the end.
Anyway we can decide this later in symfony/symfony-docs#7933 (comment), let's keep it as is for now :)

javiereguiluz · 2017-05-25T09:43:41Z

Ready for the final review before merge. Thanks!

GuilhemN · 2017-05-25T13:30:02Z

src/AppBundle/DataFixtures/ORM/PostFixtures.php

@@ -47,6 +47,7 @@ public function load(ObjectManager $manager)

            $post->setTitle($title);
            $post->setSummary($this->getRandomPostSummary());
+            // TODO: fix this 'slugger' call


Planed for another pr ?

Good catch! Maybe I'm too naive, but I'm trying to do this and it doesn't work:

class PostFixtures extends AbstractFixture implements DependentFixtureInterface, ContainerAwareInterface { use ContainerAwareTrait; use FixturesTrait; private $slugger; public function __construct(Slugger $slugger) { $this->slugger = $slugger; } // ... }

Well, fixtures are instantiated by Doctrine, so you cannot do autowiring there

Yet another exception for where autowiring works. I don't like it.

Autowiring works for services. Fixture classes have nothing to do with the DI container

weaverryan · 2017-05-26T12:42:44Z

About fixtures, the fixtures bundle should probably be updated to allow for fixtures to be a tagged service. The fact that you can't use autowiring for them is due to this shortcoming, which is with the bundle imo

weaverryan · 2017-05-26T12:52:09Z

app/config/services.yml

+            $defaultLocale: '%locale%'
+
+    Twig_Extensions_Extension_Intl:
+        class:  Twig_Extensions_Extension_Intl


If you upgrade to version 1.5.0 of Twig extensions, you can now use Twig\Extensions\IntlExtension (and so you won't need the class key

So nice! Thanks.

weaverryan · 2017-05-26T12:54:58Z

src/AppBundle/Controller/Admin/BlogController.php

    {
-        $entityManager = $this->getDoctrine()->getManager();
-        $posts = $entityManager->getRepository(Post::class)->findBy(['author' => $this->getUser()], ['publishedAt' => 'DESC']);
+        $posts = $em->getRepository(Post::class)->findBy(['author' => $user], ['publishedAt' => 'DESC']);


Personally, I still prefer $this->getUser(). In fact, in the docs, I've really been trying to continue to use all of the Controller shortcuts, except for $this->get() and $this->container->get(). So even type-hinting EntityManagerInterface instead of $this->getDoctrine()->getManager() is debatable.... as we've been discussing here: symfony/symfony-docs#7933 (comment).

Also I prefer $this->getDoctrine()->getRepository() shortcut if $em is not needed :)

I reverted EntityManagerInterface and UserInterface autowiring.

weaverryan · 2017-05-26T12:56:17Z

src/AppBundle/Controller/Admin/BlogController.php

@@ -70,10 +72,10 @@ public function indexAction()
     * to constraint the HTTP methods each controller responds to (by default
     * it responds to all methods).
     */
-    public function newAction(Request $request)
+    public function newAction(Request $request, EntityManagerInterface $em, UserInterface $user = null, Slugger $slugger)


Here's a good reason why I don't prefer the UserInterface type-hint :). If anything, in the future, I'd prefer something like Security $security and then $security->getUser(). But I don't like the UserInterface type-hint at all. It's also not something that can be done in __construct() of services, adding to inconsistency.

weaverryan · 2017-05-26T13:03:49Z

src/AppBundle/Controller/BlogController.php

@@ -65,7 +68,7 @@ public function indexAction($page, $_format)
     * value given in the route.
     * See https://symfony.com/doc/current/bundles/SensioFrameworkExtraBundle/annotations/converters.html
     */
-    public function postShowAction(Post $post)
+    public function postShowAction(Post $post, UserInterface $user = null)


Remove this too

weaverryan · 2017-05-26T13:06:24Z

src/AppBundle/EventListener/CommentNotificationSubscriber.php

+    {
+        return [
+            'comment.created' => 'onCommentCreated',
+        ];


Do you want to use Events::COMMENT_CREATED?

weaverryan · 2017-05-26T13:07:18Z

src/AppBundle/Form/Type/DateTimePickerType.php

    {
-        $this->formatConverter = new MomentFormatConverter();
+        $this->formatConverter = $converter;


Hey, this is a cool benefit of the new system :). There is no longer any extra work to do things correctly :)

bocharsky-bw · 2017-05-26T20:28:22Z

.travis.yml

@@ -36,5 +36,7 @@ script:
  - ./bin/console lint:yaml @CodeExplorerBundle
  # this checks that the Twig template files contain no syntax errors
  - ./bin/console lint:twig app/Resources @CodeExplorerBundle
+  # this checks that the XLIFF translations contain no syntax errors
+  - ./bin/console lint:xliff app/Resources


What about more specific app/Resources/translations? We have translations only in that folder

bocharsky-bw · 2017-05-26T20:43:54Z

app/config/services.yml

+            $defaultLocale: '%locale%'
+
+
+    Twig\Extensions\IntlExtension: ~


Is it necessary? Where is it used?

If I remove this service definition, I see this error:

Ah, what about adding a small note to this service definition where say it's used by localizeddate filter in Twig templates?

bocharsky-bw · 2017-05-26T20:48:36Z

src/AppBundle/Command/ListUsersCommand.php

    private $entityManager;
+    private $mailer;
+    private $emailSender;


I'd add @param string annotation since $emailSender does not have type hint and can't be resolved by IDE.

You are right ... but at the same time, the type-hint is useful for objects or arrays of objects. In this case, even if the IDE knows this is a string ... there's nothing to complete or help us, right?

bocharsky-bw · 2017-05-30T06:45:56Z

composer.json

@@ -29,8 +29,8 @@
        "symfony/monolog-bundle"               : "^3.0",
        "symfony/polyfill-apcu"                : "^1.0",
        "symfony/swiftmailer-bundle"           : "^2.3",
-        "symfony/symfony"                      : "^3.2",
-        "twig/extensions"                      : "^1.3",
+        "symfony/symfony"                      : "3.3.0-RC1",


Now we can use stable 3.3.0

javiereguiluz · 2017-05-30T07:06:33Z

After upgrading to 3.3.0 final, I see these deprecations:

I've reported the issue on symfony/symfony#22951

Updated the application to Symfony 3.3.0 RC1

798a786

yceruto reviewed May 22, 2017

View reviewed changes

stof reviewed May 22, 2017

View reviewed changes

yceruto reviewed May 22, 2017

View reviewed changes

app/config/config.yml Outdated

@@ -42,7 +42,6 @@ framework:

engines: ['twig']

default_locale: "%locale%"

Copy link

Member

yceruto May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single quote in yamls?

Changes suggested by reviewers

e2b40f7

yceruto reviewed May 22, 2017

View reviewed changes

Use DI features for commands too

faf65d3

javiereguiluz added 2 commits May 22, 2017 16:55

Use single quotes in config.yml

a718f1f

Used DI features in controllers

8447d8f

yceruto reviewed May 22, 2017

View reviewed changes

GuilhemN reviewed May 22, 2017

View reviewed changes

javiereguiluz added 5 commits May 24, 2017 15:50

Added missing $em injection

73ec12f

Added a missing property declaration

6149f61

Enable the linter of translation files

5226a1e

Fixed tests

cec08f3

Fixed a CS issue

64b57bc

GuilhemN approved these changes May 25, 2017

View reviewed changes

Removed a TODO messages because this won't be changed

b046128

weaverryan reviewed May 26, 2017

View reviewed changes

weaverryan requested changes May 26, 2017

View reviewed changes

javiereguiluz added 2 commits May 26, 2017 17:15

Reverted some autowired arguments

573ddda

Used the event name constant

0d82f42

bocharsky-bw reviewed May 26, 2017

View reviewed changes

bocharsky-bw reviewed May 30, 2017

View reviewed changes

javiereguiluz changed the title ~~Updated the application to Symfony 3.3.0 RC1~~ Updated the application to Symfony 3.3.0 May 30, 2017

Updated to 3.3.0 final

a71756b

javiereguiluz added 2 commits May 30, 2017 09:27

Added a minor help note

927920a

Removed unused imports

c94a06a

javiereguiluz closed this in ac188e9 May 30, 2017

		@@ -42,7 +42,6 @@ framework:
		engines: ['twig']
		default_locale: "%locale%"

Updated the application to Symfony 3.3.0 #562

Updated the application to Symfony 3.3.0 #562

Conversation

javiereguiluz commented May 22, 2017

javiereguiluz commented May 22, 2017

weaverryan commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented May 30, 2017 • edited Loading

yceruto May 22, 2017 •

edited

Loading

yceruto May 22, 2017 •

edited

Loading

javiereguiluz commented May 30, 2017 •

edited

Loading