Apply Symfony best practices #126

emodric · 2017-12-16T14:41:36Z

Remove class parameters
use kernel.project_dir instead of kernel.root_dir
Use Twig namespaces for templates
Marked all services as private/public

andrerom · 2017-12-17T20:31:57Z

crevillo · 2017-12-17T20:33:30Z

bundle/Controller/LegacySetupController.php

+     * @param \eZ\Bundle\EzPublishLegacyBundle\SetupWizard\ConfigurationDumper $configDumper
+     * @param \eZ\Bundle\EzPublishLegacyBundle\SetupWizard\ConfigurationConverter $configConverter
+     * @param \Symfony\Component\HttpFoundation\RequestStack $requestStack
+     * @param \Symfony\Component\HttpKernel\KernelInterface


Missed var name in this line?

I think the whole PHPDoc could be removed

Yep, thanks. Will fix it tomorrow, when I'm at my laptop :)

lolautruche

A BC note should be added somewhere as some of those changes could be considered as BC breaks (e.g. class parameters if one were relying on that to override)

lolautruche · 2017-12-17T20:39:06Z

bundle/Controller/LegacySetupController.php

+     * @param \eZ\Bundle\EzPublishLegacyBundle\SetupWizard\ConfigurationDumper $configDumper
+     * @param \eZ\Bundle\EzPublishLegacyBundle\SetupWizard\ConfigurationConverter $configConverter
+     * @param \Symfony\Component\HttpFoundation\RequestStack $requestStack
+     * @param \Symfony\Component\HttpKernel\KernelInterface


I think the whole PHPDoc could be removed

lolautruche · 2017-12-17T20:41:01Z

bundle/Resources/config/default_settings.yml

@@ -5,7 +5,7 @@ parameters:
    ezpublish_legacy.default.module_default_layout: ~

    # Pagelayout to use while rendering a content view in legacy
-    ezpublish_legacy.default.view_default_layout: EzPublishLegacyBundle::legacy_view_default_pagelayout.html.twig
+    ezpublish_legacy.default.view_default_layout: "@@EzPublishLegacy/legacy_view_default_pagelayout.html.twig"


Why a double @ here?

Without it, Symfony crashes with unknown service error or something similar. I can't find the doc about it ATM, but I have it working like this on some of our other projects, after I've read it in the docs 🙊

Ha, found it!

http://symfony.com/doc/current/service_container/parameters.html

If you use a string that starts with @ or has % anywhere in it, you need to escape it by adding another @ or %

# config/services.yaml parameters: # This will be parsed as string '@securepass' mailer_password: '@@securepass' # Parsed as http://symfony.com/?foo=%s&bar=%d url_pattern: 'http://symfony.com/?foo=%%s&bar=%%d'

emodric · 2017-12-17T20:52:21Z

@lolautruche I'll write up some 2.0 upgrade docs, thanks 👍

emodric · 2017-12-17T21:04:26Z

Note to self: Need to rebase on master to get the tests running once #122 is merged.

andrerom · 2017-12-18T06:48:03Z

Note to self: Need to rebase on master to get the tests running once #122 is merged.

merged

blankse · 2017-12-18T08:59:50Z

mvc/Kernel/Loader.php

@@ -263,7 +264,7 @@ public function setCLIHandler(ezpKernelHandler $kernelHandler)
    public function buildLegacyKernelHandlerTreeMenu()
    {
        return $this->buildLegacyKernelHandlerWeb(
-            $this->container->getParameter('ezpublish_legacy.kernel_handler.treemenu.class'),
+            ezpKernelWeb::class,


Shouldn't it be ezpKernelTreeMenu?

Eeeek, you're right!!

Fixed, thanks!

emodric · 2017-12-18T09:25:37Z

@andrerom @crevillo @blankse @lolautruche Ready for final review and merge :)

Will writeup upgrade docs in a separate PR.

blankse

LGTM

emodric · 2017-12-18T15:39:27Z

Thanks everyone!

wizhippo · 2018-11-05T19:46:51Z

bundle/Resources/config/templating.yml

        calls:
            - [setLegacyHelper, ["@ezpublish_legacy.templating.legacy_helper"]]

    # Overriding core helper
    ezpublish.templating.global_helper:
        alias: ezpublish.templating.global_helper.legacy
+        public: false


Is EzPublishCoreBundle\Controller deprecated? The breaks it's 'getGlobalHelper' as it needs the service to be public or an exception is thrown and it is private now.

My guess: helper should be adapted to inject the service(s) instead of relying on contianer.

I don't believe when aliasing a service we should change its viability either which is causing me the issue I see.

That could be as well.

@emodric as you're the author of this change, any opinions on the matter :)

I'm fine with it being public, no problem :)

Another approach would be to use the service locator feature in EzPublishCoreBundle\Controller (Symfony 3.3+)

See implementation in Symfony AbstractController: https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php

Hi guys, I've found this issue after upgrading from an old 5.4 to the new 2.5.6, solved by overriding service definition on my own services.yml file.

Someone willing to do PR to drop this? I agree we should not change this from kernel.

emodric changed the title ~~Apply Symfony best practices~~ [WIP] Apply Symfony best practices Dec 16, 2017

emodric force-pushed the sf_best_practices branch 2 times, most recently from 237c856 to cc2f0d7 Compare December 16, 2017 17:13

emodric changed the title ~~[WIP] Apply Symfony best practices~~ Apply Symfony best practices Dec 16, 2017

crevillo reviewed Dec 17, 2017

View reviewed changes

lolautruche approved these changes Dec 17, 2017

View reviewed changes

emodric changed the base branch from 2.0 to master December 18, 2017 08:48

emodric force-pushed the sf_best_practices branch 2 times, most recently from 371d616 to 6b9323a Compare December 18, 2017 08:55

blankse reviewed Dec 18, 2017

View reviewed changes

emodric added 4 commits December 18, 2017 10:06

Remove class parameters, use kernel.project_dir

e649875

Use Twig namespaces for templates

5135ba8

Mark all services as private/public, as needed

f020990

Fix some PHPDoc

a212141

emodric force-pushed the sf_best_practices branch from 6b9323a to a212141 Compare December 18, 2017 09:06

Use ::class pseudo-constant

6eeec35

blankse approved these changes Dec 18, 2017

View reviewed changes

andrerom approved these changes Dec 18, 2017

View reviewed changes

emodric merged commit 4551926 into master Dec 18, 2017

andrerom deleted the sf_best_practices branch December 18, 2017 15:45

emodric mentioned this pull request Dec 19, 2017

Add 2.0 upgrade docs #128

Merged

wizhippo reviewed Nov 5, 2018

View reviewed changes

xserna mentioned this pull request Dec 3, 2019

Remove visibility change on global helper core service override. #175

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply Symfony best practices #126

Apply Symfony best practices #126

emodric commented Dec 16, 2017 •

edited

Loading

andrerom commented Dec 17, 2017

crevillo Dec 17, 2017

lolautruche Dec 17, 2017

emodric Dec 17, 2017

lolautruche left a comment

lolautruche Dec 17, 2017

lolautruche Dec 17, 2017

emodric Dec 17, 2017

emodric Dec 17, 2017

emodric commented Dec 17, 2017

emodric commented Dec 17, 2017

andrerom commented Dec 18, 2017

blankse Dec 18, 2017

emodric Dec 18, 2017

emodric commented Dec 18, 2017

blankse left a comment

emodric commented Dec 18, 2017

wizhippo Nov 5, 2018

andrerom Nov 5, 2018 •

edited

Loading

wizhippo Nov 5, 2018

andrerom Nov 6, 2018

emodric Nov 6, 2018

lolautruche Nov 6, 2018

lolautruche Nov 6, 2018

xserna Dec 3, 2019

andrerom Dec 3, 2019 •

edited

Loading

xserna Dec 3, 2019

Apply Symfony best practices #126

Apply Symfony best practices #126

Conversation

emodric commented Dec 16, 2017 • edited Loading

andrerom commented Dec 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lolautruche left a 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

emodric commented Dec 17, 2017

emodric commented Dec 17, 2017

andrerom commented Dec 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emodric commented Dec 18, 2017

blankse left a comment

Choose a reason for hiding this comment

emodric commented Dec 18, 2017

Choose a reason for hiding this comment

andrerom Nov 5, 2018 • edited Loading

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

andrerom Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emodric commented Dec 16, 2017 •

edited

Loading

andrerom Nov 5, 2018 •

edited

Loading

andrerom Dec 3, 2019 •

edited

Loading