Use Templating Component without "templating" service #457

covex-nn · 2017-12-25T20:31:28Z

I am targeting this branch, because this is backward compatible feature.

Changelog

### Added
- Added `symfony/asset` and `symfony/templating` dependencies
- Added new service `sonata.templating` for use in place of `templating`

### Changed
- Referencing templates using Twig namespaced syntax

### Removed
- Removed tag `templating.helper` from `sonata.block.templating.helper` service

Subject

This is first step to switching from Templating Component to using Twig directly. The following two steps are described in #456. This PR will allow to run SonataBlockBundle without activating templating extension in framework.

If this PR will be accepted, all other sonata bundles, that use SonataBlockBundle, could be updated too: services will have to use sonata.templating service instead of templating and templates must be referenced using Twig namespaced syntax

SonataCI · 2017-12-27T06:50:20Z

Could you please rebase your PR and fix merge conflicts?

SonataCI · 2017-12-27T07:10:21Z

Could you please rebase your PR and fix merge conflicts?

SonataCI · 2018-01-07T17:50:19Z

Could you please rebase your PR and fix merge conflicts?

… extension

OskarStark · 2018-01-08T06:15:05Z

src/Templating/TemplateLocator.php

+use Symfony\Bundle\FrameworkBundle\Templating\Loader\TemplateLocator as FrameworkTemplateLocator;
+
+/**
+ * @deprecated since 3.9, to be removed with 4.0.


why? its new

I've created new service sonata.templating, to start migrating from templating to twig service without BC breaks in 3.x branch. sonata.templating, like templating service, has 3 arguments: twig, sonata.templating.name_parser and sonata.templating.locator. This new class is sonata.templating.locator service.

It is a new class, yes. It is needed in 3.x branch to be sure, that everything will be fine after deprecating of symfony/templating component. But this new class should not be used by other services, because it will be deleted in master branch (if PR will be merged, of course) accordingly with stage 2 of a plan, described in #456

So, it is new and deprecated at the same time.

OskarStark · 2018-01-08T06:15:13Z

src/Templating/TemplateNameParser.php

+use Symfony\Bundle\FrameworkBundle\Templating\TemplateNameParser as FrameworkTemplateNameParser;
+
+/**
+ * @deprecated since 3.9, to be removed with 4.0.


why? its new

Same as above. But this class is sonata.templating.name_parser service

kricha · 2018-01-08T14:29:04Z

Maybe this need to be done in CoreBundle? And then can be reused in other sonata bundles?
@OskarStark @greg0ire @core23 @jordisala1991

greg0ire · 2018-01-09T17:18:32Z

@aLkRicha what do you mean by reused? This migrations needs to be done on all bundles, right?

covex-nn · 2018-01-09T20:07:53Z

@jordisala1991 the second argument of Sonata\AdminBundle\Block\AdminListBlockService (abstract parent class for all Blocks) is Symfony\Bundle\FrameworkBundle\Templating\EngineInterface $templating.

So, we cannot just change EngineInterface to Twig\Environment in 3.x branch - this is BC break. And there is nothing to deprecate here, except may be AbstractBlockService itself.

kricha · 2018-01-09T21:01:43Z

@covex-nn please, answer my question ☺️ #457 (comment)

covex-nn · 2018-01-09T21:33:16Z

@aLkRicha CoreBundle does not use templating service, BlockBundle does. The main purpose
of sonata.templating is to replace templating as a second argument of AdminListBlockService class in 3.x branch without BC breaks. If some other bundle uses templating, then there could be another way how to decouple from symfony/templating and how to use twig service instead of templating.

So, BlockBundle is a right place for a new sonata.templating service. May be sonata.templating should be in separate repository (not in SonataCoreBundle and not in SonataBlockBundle), but i am not sure about it (it is too complicated)

jordisala1991 · 2018-01-09T21:37:13Z

@covex-nn but you can remove the type hint, and check if the instance passed is a EngineInterface or TwigEnv.. and if not throw an exception.. and that is not BC break

covex-nn · 2018-01-09T22:30:37Z

@jordisala1991 i used http://symfony.com/doc/current/contributing/code/bc.html to determine what change is allowed and what is not. Removing type hint of an argument of a public method is not allowed there. I thought it is a BC break.

Anyway, removing type hint could be done only after all template references in all bundles will be guaranteed switched from "templating syntax" to "twig namespaced syntax", i.e. only in master branch, in next major release. But i hoped to run SonataAdminBundle with Flex with in 3.x branch.

If "removing type hint of an argument" is not BC break, than my plan is not good enough, and there will be 2 stages, not 3.

jordisala1991 · 2018-01-10T06:41:06Z

Wow, they really consider it bc break, althought we have done it several times, and I am not sure why they consider a bc break, because it does not break anything (if done correctly) . Wdyt @greg0ire?

greg0ire · 2018-01-10T08:18:04Z

Removing the type hint of a method is a BC break for extending classes (they may only widen it).

jordisala1991 · 2018-01-10T08:37:01Z

https://3v4l.org/s6f92

It throws a warning but it works, anyway if we consider it BC break we cannot do what I said.

greg0ire · 2018-01-10T09:16:37Z

Warning are BC breaks... Even strict standards are. That's why classes / methods should be final

covex-nn · 2018-01-10T15:48:29Z

I've updated issue #456, i added removing type hint for second argument of Sonata\AdminBundle\Block\AdminListBlockService in current master. Both sonata.templating and twig services will be used in blocks. Future master branch will not contain sonata.templating service.

greg0ire · 2018-01-16T07:47:20Z

The build fails on php 7.0

OskarStark · 2018-01-16T08:14:55Z

The build fails on php 7.0

I restarted the build, as it was a race condition 👍

kricha · 2018-01-16T08:52:26Z

do we really need there symfony/templating dep?

covex-nn · 2018-01-16T12:40:37Z

@aLkRicha we need it in 3.x branch, because Templating Component is still used. It will be removed in master branch, if this PR will be accepted

jordisala1991 · 2018-01-17T06:34:03Z

src/Test/FakeTemplating.php

@@ -11,7 +11,7 @@

 namespace Sonata\BlockBundle\Test;

-use Symfony\Bundle\FrameworkBundle\Templating\EngineInterface;
+use Sonata\BlockBundle\Templating\EngineInterface;


This is the only “use” that you are replacing even though there are more references to EngineInterface on the project. Dont we need to replace all of them?

We will replace all "use"s in master branch, if this PR will be accepted. Tests, that use Sonata\BlockBundle\Test\FakeTemplating show us, that when we use templating service, implementing Sonata\BlockBundle\Templating\EngineInterface, everything is fine

And we use sonata.templating instead of templating !

sonata.templating is Sonata\BlockBundle\Templating\TwigEngine, it implements Sonata\BlockBundle\Templating\EngineInterface.

jordisala1991 · 2018-01-17T18:38:19Z

Lets pull the trigger, great work @covex-nn

covex-nn · 2018-01-17T18:42:50Z

@jordisala1991 please do not release it right now. I realized, that i should not remove tag from templating helper. Functions ~~{{ sonata_block.includeStylesheets(...) }}~~ are not available in 3.x. I will create PR to fix that. Sorry

covex-nn · 2018-01-19T10:31:08Z

I've found, that it is not so important, but anyway removing templating.helper tag is BC break: templating.helper tag is used for php templating engine:

<?php echo $view['sonata_block']->includeStylesheets('css/style.css'); ?>

covex-nn force-pushed the twig branch from 3f37ade to 046f6ea Compare December 25, 2017 20:49

SonataCI added the pending author label Dec 27, 2017

OskarStark added the minor label Dec 27, 2017

covex-nn force-pushed the twig branch from 046f6ea to b960f73 Compare December 27, 2017 07:01

SonataCI removed the pending author label Dec 27, 2017

SonataCI added the pending author label Dec 27, 2017

covex-nn changed the title ~~Use Templating Component without "templating" service~~ [WIP] Use Templating Component without "templating" service Dec 27, 2017

covex-nn force-pushed the twig branch from b960f73 to a947cc9 Compare December 27, 2017 08:01

SonataCI removed the pending author label Dec 27, 2017

covex-nn changed the title ~~[WIP] Use Templating Component without "templating" service~~ Use Templating Component without "templating" service Dec 27, 2017

SonataCI added the pending author label Jan 7, 2018

covex-nn added 6 commits January 7, 2018 23:54

Update composer.json deps

4a7c178

Referencing templates using Twig namespaceв syntax

1b7d7a6

Use sonata templating service without activating framework.templating…

15ab935

… extension

Make sonata.block.templating.helper a regular service

325a58f

Deprecation comments

d57dc07

Fix CS errors

ef82b8d

covex-nn force-pushed the twig branch from a947cc9 to ef82b8d Compare January 7, 2018 20:54

SonataCI removed the pending author label Jan 7, 2018

OskarStark reviewed Jan 8, 2018

View reviewed changes

OskarStark requested review from greg0ire, jordisala1991 and core23 January 8, 2018 06:15

OskarStark mentioned this pull request Jan 8, 2018

add one more dependency in composer for creating symfony-recipe #463

Closed

kricha mentioned this pull request Jan 16, 2018

add SonataBlockBundle symfony/recipes-contrib#235

Closed

OskarStark approved these changes Jan 16, 2018

View reviewed changes

greg0ire approved these changes Jan 16, 2018

View reviewed changes

This was referenced Jan 16, 2018

Switch from Templating Component to Twig #456

Closed

Decouple sonata-project bundles from Templating Component, Stage 1 sonata-project/dev-kit#380

Closed

jordisala1991 approved these changes Jan 17, 2018

View reviewed changes

jordisala1991 reviewed Jan 17, 2018

View reviewed changes

core23 approved these changes Jan 17, 2018

View reviewed changes

jordisala1991 merged commit 5acea15 into sonata-project:3.x Jan 17, 2018

covex-nn deleted the twig branch January 17, 2018 18:45

covex-nn mentioned this pull request Jan 17, 2018

Add tag templating.helper back to sonata.block.templating.helper service #466

Merged

This was referenced Apr 22, 2018

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

Merged

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Templating Component without "templating" service #457

Use Templating Component without "templating" service #457

covex-nn commented Dec 25, 2017 •

edited by OskarStark

Loading

SonataCI commented Dec 27, 2017

SonataCI commented Dec 27, 2017

SonataCI commented Jan 7, 2018

OskarStark Jan 8, 2018

covex-nn Jan 8, 2018

OskarStark Jan 8, 2018

covex-nn Jan 8, 2018

kricha commented Jan 8, 2018

greg0ire commented Jan 9, 2018

covex-nn commented Jan 9, 2018

kricha commented Jan 9, 2018

covex-nn commented Jan 9, 2018

jordisala1991 commented Jan 9, 2018 •

edited

Loading

covex-nn commented Jan 9, 2018

jordisala1991 commented Jan 10, 2018

greg0ire commented Jan 10, 2018

jordisala1991 commented Jan 10, 2018

greg0ire commented Jan 10, 2018

covex-nn commented Jan 10, 2018 •

edited

Loading

greg0ire commented Jan 16, 2018

OskarStark commented Jan 16, 2018

kricha commented Jan 16, 2018

covex-nn commented Jan 16, 2018

jordisala1991 Jan 17, 2018 •

edited

Loading

covex-nn Jan 17, 2018

covex-nn Jan 17, 2018

jordisala1991 commented Jan 17, 2018

covex-nn commented Jan 17, 2018 •

edited

Loading

covex-nn commented Jan 19, 2018 •

edited

Loading

Use Templating Component without "templating" service #457

Use Templating Component without "templating" service #457

Conversation

covex-nn commented Dec 25, 2017 • edited by OskarStark Loading

Changelog

Subject

SonataCI commented Dec 27, 2017

SonataCI commented Dec 27, 2017

SonataCI commented Jan 7, 2018

OskarStark Jan 8, 2018

Choose a reason for hiding this comment

covex-nn Jan 8, 2018

Choose a reason for hiding this comment

OskarStark Jan 8, 2018

Choose a reason for hiding this comment

covex-nn Jan 8, 2018

Choose a reason for hiding this comment

kricha commented Jan 8, 2018

greg0ire commented Jan 9, 2018

covex-nn commented Jan 9, 2018

kricha commented Jan 9, 2018

covex-nn commented Jan 9, 2018

jordisala1991 commented Jan 9, 2018 • edited Loading

covex-nn commented Jan 9, 2018

jordisala1991 commented Jan 10, 2018

greg0ire commented Jan 10, 2018

jordisala1991 commented Jan 10, 2018

greg0ire commented Jan 10, 2018

covex-nn commented Jan 10, 2018 • edited Loading

greg0ire commented Jan 16, 2018

OskarStark commented Jan 16, 2018

kricha commented Jan 16, 2018

covex-nn commented Jan 16, 2018

jordisala1991 Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

covex-nn Jan 17, 2018

Choose a reason for hiding this comment

covex-nn Jan 17, 2018

Choose a reason for hiding this comment

jordisala1991 commented Jan 17, 2018

covex-nn commented Jan 17, 2018 • edited Loading

covex-nn commented Jan 19, 2018 • edited Loading

covex-nn commented Dec 25, 2017 •

edited by OskarStark

Loading

jordisala1991 commented Jan 9, 2018 •

edited

Loading

covex-nn commented Jan 10, 2018 •

edited

Loading

jordisala1991 Jan 17, 2018 •

edited

Loading

covex-nn commented Jan 17, 2018 •

edited

Loading

covex-nn commented Jan 19, 2018 •

edited

Loading