Remove dependency on symfony/templating #476

covex-nn · 2018-02-05T07:15:28Z

I am targeting this branch, because this is BC break.

Changelog

### Changed
- Use Twig directly in Blocks and Exception renderers
- `AbstractBlockService::getTemplating` method renamed to `getTwig`

### Removed
- Removed default null values for arguments of `AbstractBlockService` class
- Removed dependency on `symfony/templating` in `composer.json`
- Removed all temporary classes for templating
- Removed `FakeTemplating` class

Subject

Switch from Templating Component to Twig, Stage 2

greg0ire · 2018-02-05T07:18:21Z

src/Block/Service/AbstractBlockService.php

                E_USER_DEPRECATED
            );
+        } elseif (!$templating instanceof Environment) {
+            throw new \TypeError('Argument 2 passed to '.get_class($this).'::__construct() must be instance of Twig\Environment, instance of '.get_class($templating).' given');


Please linebreak this, use sprintf, __METHOD__ and Environment::class

greg0ire · 2018-02-05T07:19:17Z

src/Exception/Renderer/InlineRenderer.php

     */
-    protected $templating;
+    protected $twig;


Should be private

Fixed, $twig property is private in InlineRenderer and InlineDebugRenderer

greg0ire · 2018-02-05T07:19:50Z

src/Templating/EngineInterface.php

+     *
+     * @return string The evaluated template as a string
+     */
+    public function render($name, array $parameters = []);


Please add return type hints

Added type hints and updated classes, implementing this interface

kunicmarko20 · 2018-02-05T07:53:00Z

src/Block/Service/AbstractBlockService.php

@@ -63,7 +72,13 @@ public function __construct($name = null, EngineInterface $templating = null)
     */
    public function renderResponse($view, array $parameters = [], Response $response = null)
    {
-        return $this->getTemplating()->renderResponse($view, $parameters, $response);
+        if (null === $response) {


this can probably be changed to $response = $response ?? new Response(); but I am not sure that makes it better 😄

Fixed. But, i searched for ?? new string in master branches of all sonata branches. This will be for a first time, where $obj = $obj ?? new className() is used =)

jordisala1991 · 2018-02-05T09:11:31Z

src/Block/Service/AbstractBlockService.php

    {
-        if (null === $name || null === $templating) {
+        if ($templating instanceof EngineInterface) {


Do we need to add deprecation messages on master? can't we just move to twig direclty? wdyt @sonata-project/contributors @covex-nn ?

I think that is a better option.

At the begining i thought, that if i will remove type hint from __construct of AbstractBlockService, it will be easier to upgrade all other bundles, because they will be compatible with 3.x and with dev-master of BlockBundle.

I searched in src/Block directory inside each bundle and found, that there are 12 bundles, containing Blocks:

7 bundles contain at least one Block with extended __construct (with more than 2 parameters): AdminBundle, DoctrineORMAdminBundle, MediaBundle, NewsBundle, PageBundle, SeoBundle, TimelineBundle.

4 bundles have Blocks without __construct: CommentBundle: DashboardBundle, FormatterBundle, TranslationBundle

DoctrinePhpcrAdminBundle contain Block, that extends deprecated BaseBlockService class

So, first 7 bundles will not be compatible with new AbstractBlockService::__construct =(( May be it is too much and a should just change EngineInterface $templating to Environment $templating? And there will be no deprecation message.

@jordisala1991 @kunicmarko20 with type hint Environment for second constructor's parameter of AbstractBlockService there won't be Stage 3 =) and there won't be a message about deprecation, of course. I like it more, than current proposal.

But if we will leave second parameter without type hint, new Block classes, created after 3.11 and before 4.0 release could be compatible with new AbstractBlockService, if they will be without own __construct. And i guess, that all of new Blocks will use templating service, not new sonata.templating!

So, i think, that we will not reach BC. And if you do not mind, tomorrow i will add Environment type hint to a second parameter.

Well, 4.x does not have to be BC with 3.x? /cc @sonata-project/contributors

If I understand correctly, the issue is that some bundle need to be compatible with 2 versions of some other bundles, right?

kunicmarko20 · 2018-02-05T09:56:05Z

src/Block/Service/MenuBlockService.php

     * @param MenuProviderInterface      $menuProvider
     * @param MenuRegistryInterface|null $menuRegistry
     */
-    public function __construct($name, EngineInterface $templating, MenuProviderInterface $menuProvider, $menuRegistry = null)
+    public function __construct($name, $templating, MenuProviderInterface $menuProvider, $menuRegistry = null)


Can you replace parameters with typehint and remove the comment?

@kunicmarko20 i am sorry, i did not understans what you mean =( you say, that i have to replace $templating with Environment $templating? But exception renderers contain this type hint and your comment is the same there.

@covex-nn sorry, my bad, here you can skip it but on other places you can replace comment with typehint in constructor, right?

kunicmarko20 · 2018-02-05T09:56:17Z

src/Exception/Renderer/InlineDebugRenderer.php

     */
-    public function __construct(EngineInterface $templating, $template, $debug, $forceStyle = true)
+    public function __construct(Environment $twig, $template, $debug, $forceStyle = true)


Can you add typehint to parameters and remove the comment?

But type hint is already exists here. Removing type hint for AbstractBlockService was made for compability with other bundles (for easier upgrade). Should i remove type hint from Environment $twig or comment @param Environment $twig Templating engine ?

My comment was badly explained, I updated it. So we can typehint all parameters and remove the phpdoc comment.

Fixed. It is public function __construct(Environment $twig, string $template, bool $debug, bool $forceStyle = true) now without phpdoc comment

Great, thank you very much!

kunicmarko20 · 2018-02-05T09:56:28Z

src/Exception/Renderer/InlineRenderer.php

     */
-    public function __construct(EngineInterface $templating, $template)
+    public function __construct(Environment $twig, $template)


Can you add typehint to parameters and remove the comment?

Fixed. It is public function __construct(Environment $twig, string $template) now without phpdoc comment

covex-nn · 2018-02-07T07:32:10Z

src/Test/FakeTemplating.php

@@ -20,6 +20,8 @@
 * Mocking class for template usage.
 *
 * @author Thomas Rabaix <[email protected]>
+ *
+ * @deprecated since 4.0, to be removed with 5.0.
 */
 class FakeTemplating implements EngineInterface


This class is not used any more. Can i remove it instead of deprecating?

jordisala1991 · 2018-02-07T09:25:57Z

src/Block/Service/AbstractBlockService.php

-        return $this->getTemplating()->renderResponse($view, $parameters, $response);
+        $response = $response ?? new Response();
+
+        $response->setContent($this->getTemplating()->render($view, $parameters));


setContent returns the response, so you can return directly if you want :P

I do not like it =) with these two lines i can set breakpoint on a second line with return $response after setContent; with one line it won't be convenient

jordisala1991 · 2018-02-07T09:26:44Z

src/Templating/EngineInterface.php


-interface EngineInterface extends TemplatingEngineInterface
+/**
+ * @deprecated since 4.0, to be removed with 5.0.


If not used, you can remove it also

It is not used =)
If so, in next PR i will remove templating helper too! All methods will be copied to twig extension

SonataCI · 2018-02-08T08:30:22Z

Could you please rebase your PR and fix merge conflicts?

jordisala1991 · 2018-02-13T21:47:32Z

src/Block/Service/AbstractBlockService.php

-                E_USER_DEPRECATED
-            );
-        }
-
        $this->name = $name;
        $this->templating = $templating;


Can this be renamed to twig? And maybe the getTemplate method too? it will be a lof of changes maybe so I am not sure, but it seems wrong

I was afraid, that you would ask =) It is not a lot of changes here, in BlockBundle, but getTemplating could be used in other bundles.
I will rename it everywhere. Also i will make AbstractBlockService::$templating private.

@jordisala1991 done
Travis failed, because 41 is not 42 again =) Please, restart travis job https://travis-ci.org/sonata-project/SonataBlockBundle/jobs/341291098

SonataCI · 2018-02-14T05:20:19Z

Could you please rebase your PR and fix merge conflicts?

jordisala1991 · 2018-02-14T06:56:49Z

composer.json

@@ -34,7 +34,6 @@
        "symfony/http-foundation": "^2.8 || ^3.2 || ^4.0",
        "symfony/http-kernel": "^2.8 || ^3.2 || ^4.0",
        "symfony/options-resolver": "^2.8 || ^3.2 || ^4.0",
-        "symfony/templating": "^2.8 || ^3.2 || ^4.0",


Can you check if we can get rid of framework-bundle also? AFAIK it was used for the EngineInterface, not sure if we need it for anything else

symfony/framework-bundle is used only in Sonata\BlockBundle\Command\BaseCommand:

namespace Sonata\BlockBundle\Command; use Sonata\BlockBundle\Block\BlockServiceManagerInterface; use Symfony\Bundle\FrameworkBundle\Command\ContainerAwareCommand; abstract class BaseCommand extends ContainerAwareCommand { /* ... */ }

To remove dependency extends ContainerAwareCommand could be replaced with implements \Symfony\Component\DependencyInjection\ContainerAwareInterface here

jordisala1991 · 2018-02-14T06:58:18Z

tests/Exception/Renderer/InlineDebugRendererTest.php

@@ -33,7 +33,7 @@ public function testRenderWithoutDebug(): void
        $debug = false;
        $exception = $this->createMock('\Exception');
        $block = $this->createMock('Sonata\BlockBundle\Model\BlockInterface');
-        $templating = $this->createMock('Symfony\Bundle\FrameworkBundle\Templating\EngineInterface');
+        $templating = $this->createMock('Twig\Environment');


Can you use ::class notation? Only on the lines you changed. No need to fix the whole file

Done. Also i renamed all $templating to $twig in test classes

jordisala1991

Probably the docs could be updated, and an upgrade note to know what to do when migrating from 3.x to 4.x of this Bundle.

The changelog could be simplified.

Except for this minor comments, 👍

covex-nn · 2018-02-20T16:20:10Z

@jordisala1991 i've updated upgrading note and some docs. But this is the most difficult part of PR =)

greg0ire · 2018-02-20T17:19:36Z

UPGRADE-4.0.md

+
+## Block constructor
+
+Blocks are using `twig` service to render templates, so if you already overridden a constructor of a custom `AbstractBlockService`, you must update it.


SonataCI · 2018-02-21T01:10:19Z

Could you please rebase your PR and fix merge conflicts?

kunicmarko20 · 2018-03-13T09:58:58Z

ping @sonata-project/contributors can you review this and maybe get it merged.

OskarStark · 2018-03-14T08:29:55Z

IMO 3 approvals are enough 👍

OskarStark · 2018-03-14T08:30:28Z

Thank you very much @covex-nn 👍

greg0ire reviewed Feb 5, 2018

View reviewed changes

kunicmarko20 reviewed Feb 5, 2018

View reviewed changes

covex-nn force-pushed the twig branch 2 times, most recently from de0842e to 59eb208 Compare February 5, 2018 08:06

jordisala1991 reviewed Feb 5, 2018

View reviewed changes

kunicmarko20 reviewed Feb 5, 2018

View reviewed changes

jordisala1991 added the major label Feb 5, 2018

covex-nn changed the title ~~Remove dependency on symfony/templating~~ [WIP] Remove dependency on symfony/templating Feb 5, 2018

covex-nn force-pushed the twig branch from 3bebe2e to 403516f Compare February 5, 2018 14:55

covex-nn mentioned this pull request Feb 5, 2018

Use Twig directly in sonata-project bundles without proxy classes, Stage 2 sonata-project/dev-kit#390

Closed

15 tasks

covex-nn force-pushed the twig branch from 4650fba to 646c304 Compare February 7, 2018 06:56

covex-nn commented Feb 7, 2018

View reviewed changes

covex-nn mentioned this pull request Feb 7, 2018

[WIP] Remove dependency on symfony/templating sonata-project/SonataAdminBundle#4938

Closed

1 task

jordisala1991 reviewed Feb 7, 2018

View reviewed changes

SonataCI added the pending author label Feb 8, 2018

covex-nn force-pushed the twig branch from f4c271d to 3fab05c Compare February 8, 2018 09:24

SonataCI removed the pending author label Feb 8, 2018

covex-nn changed the title ~~[WIP] Remove dependency on symfony/templating~~ Remove dependency on symfony/templating Feb 8, 2018

covex-nn force-pushed the twig branch 2 times, most recently from b7c0fdb to 88fff71 Compare February 12, 2018 05:22

jordisala1991 reviewed Feb 13, 2018

View reviewed changes

SonataCI added pending author and removed pending author labels Feb 14, 2018

covex-nn force-pushed the twig branch from 88fff71 to fb38649 Compare February 14, 2018 05:47

jordisala1991 reviewed Feb 14, 2018

View reviewed changes

jordisala1991 added Needs: Documentation Needs: Upgrade note labels Feb 18, 2018

jordisala1991 reviewed Feb 18, 2018

View reviewed changes

greg0ire reviewed Feb 20, 2018

View reviewed changes

jordisala1991 removed Needs: Upgrade note Needs: Documentation labels Feb 20, 2018

SonataCI added the pending author label Feb 21, 2018

Remove dependency on symfony/templating

c7850e5

covex-nn force-pushed the twig branch from 8de3ca7 to c7850e5 Compare February 21, 2018 05:13

SonataCI removed the pending author label Feb 21, 2018

jordisala1991 approved these changes Feb 21, 2018

View reviewed changes

OskarStark approved these changes Mar 6, 2018

View reviewed changes

OskarStark requested review from greg0ire and core23 March 6, 2018 08:47

kunicmarko20 approved these changes Mar 6, 2018

View reviewed changes

OskarStark merged commit 160da4a into sonata-project:master Mar 14, 2018

covex-nn deleted the twig branch March 14, 2018 08:31

This was referenced Apr 23, 2018

Remove dependency on phpcr/phpcr-implementation doctrine/DoctrinePHPCRBundle#323

Merged

Use sonata.templating service from SonataBlockBundle symfony-cmf/block-bundle#282

Merged


		## Block constructor

		Blocks are using `twig` service to render templates, so if you already overridden a constructor of a custom `AbstractBlockService`, you must update it.

Remove dependency on symfony/templating #476

Remove dependency on symfony/templating #476

Conversation

covex-nn commented Feb 5, 2018 • edited by OskarStark Loading

Changelog

Subject

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

kunicmarko20 Feb 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

covex-nn Feb 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

kunicmarko20 Feb 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

kunicmarko20 Feb 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

covex-nn Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

SonataCI commented Feb 8, 2018

Choose a reason for hiding this comment

covex-nn Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

covex-nn Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

SonataCI commented Feb 14, 2018

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

jordisala1991 left a comment

Choose a reason for hiding this comment

covex-nn commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SonataCI commented Feb 21, 2018

kunicmarko20 commented Mar 13, 2018

OskarStark commented Mar 14, 2018

OskarStark commented Mar 14, 2018

covex-nn commented Feb 5, 2018 •

edited by OskarStark

Loading

kunicmarko20 Feb 5, 2018 •

edited

Loading

covex-nn Feb 5, 2018 •

edited

Loading

kunicmarko20 Feb 5, 2018 •

edited

Loading

kunicmarko20 Feb 5, 2018 •

edited

Loading

covex-nn Feb 7, 2018 •

edited

Loading

covex-nn Feb 14, 2018 •

edited

Loading

covex-nn Feb 14, 2018 •

edited

Loading