New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FrameworkBundle] Deprecate the Templating component integration #21035

Open
wants to merge 1 commit into
base: master
from

Conversation

@dunglas
Member

dunglas commented Dec 23, 2016

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

This PR deprecates the Templating component integration in FrameworkBundle. Only a few people use it because almost everybody use Twig or the serializer to output data. Removing this component will facilitate the maintenance.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Dec 23, 2016

Member

you also need to deprecate the TwigEngine (in the bridge and the bundle), as they are about integrating Twig in this component.

Member

stof commented Dec 23, 2016

you also need to deprecate the TwigEngine (in the bridge and the bundle), as they are about integrating Twig in this component.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Dec 23, 2016

Member

And this requires deprecating the AppBundle:foo:bar.html.twig syntax too, as it comes from the Templating system (and this one is used)

Member

stof commented Dec 23, 2016

And this requires deprecating the AppBundle:foo:bar.html.twig syntax too, as it comes from the Templating system (and this one is used)

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Dec 23, 2016

Member

Do we want to deprecate this syntax or not?

Member

dunglas commented Dec 23, 2016

Do we want to deprecate this syntax or not?

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Dec 23, 2016

Member

Well, if we don't want to deprecate it, you need to undeprecate the TemplateNameParser system in your PR.

Member

stof commented Dec 23, 2016

Well, if we don't want to deprecate it, you need to undeprecate the TemplateNameParser system in your PR.

@mvrhov

This comment has been minimized.

Show comment
Hide comment
@mvrhov

mvrhov Dec 23, 2016

Contributor

The @AppBundle/foo/bar.html.twig syntax is recommended since 2.5 or something like that.
But deprecating the symfony specific syntax might show some resistance :P

Contributor

mvrhov commented Dec 23, 2016

The @AppBundle/foo/bar.html.twig syntax is recommended since 2.5 or something like that.
But deprecating the symfony specific syntax might show some resistance :P

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Dec 23, 2016

Member

So let's deprecate this notation.

Member

dunglas commented Dec 23, 2016

So let's deprecate this notation.

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Dec 23, 2016

Member

@dunglas last time we tried to deprecate that syntax (in June 2014) they almost killed us 😁 See #11051

By the way, in #20130 we discussed about a big inconsistency in the Twig syntax. Maybe we should take care of that too?

Member

javiereguiluz commented Dec 23, 2016

@dunglas last time we tried to deprecate that syntax (in June 2014) they almost killed us 😁 See #11051

By the way, in #20130 we discussed about a big inconsistency in the Twig syntax. Maybe we should take care of that too?

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Dec 23, 2016

Member

@javiereguiluz but we have an advantage this time, people already using Twig alone (without the templating bridge) can only use the @AppBundle syntax. We're deprecating the Templating component, other syntaxes - not supported by the Twig Bundle - will be deprecated with it. Does it looks reasonable?

Member

dunglas commented Dec 23, 2016

@javiereguiluz but we have an advantage this time, people already using Twig alone (without the templating bridge) can only use the @AppBundle syntax. We're deprecating the Templating component, other syntaxes - not supported by the Twig Bundle - will be deprecated with it. Does it looks reasonable?

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Dec 23, 2016

Member

Indeed, #11051 is very different. Here we are deprecating everything related to the Templating component, including the way we reference templates. There is no need to discuss how to reference templates as we are just using "pure" Twig, nothing more.

Member

fabpot commented Dec 23, 2016

Indeed, #11051 is very different. Here we are deprecating everything related to the Templating component, including the way we reference templates. There is no need to discuss how to reference templates as we are just using "pure" Twig, nothing more.

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Dec 23, 2016

Member

But I think we should indeed look into #20130 and try to find a way to resolve the inconsistency if we force everyone to use the Twig schema to reference templates.

Member

xabbuh commented Dec 23, 2016

But I think we should indeed look into #20130 and try to find a way to resolve the inconsistency if we force everyone to use the Twig schema to reference templates.

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Dec 26, 2016

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Dec 28, 2016

Member

One even more important thing: The native Twig namespace syntax as it is currently integrated in the framework fails on bundle inheritance (see #6919). Please take a look at #19586 which provides a fix.

Member

xabbuh commented Dec 28, 2016

One even more important thing: The native Twig namespace syntax as it is currently integrated in the framework fails on bundle inheritance (see #6919). Please take a look at #19586 which provides a fix.

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Jan 13, 2017

Member

Should the TemplatingPass be deprecated?
Maybe should it trigger deprecations for each service in the loop too (i.e. tagged templating.*)?

Member

chalasr commented Jan 13, 2017

Should the TemplatingPass be deprecated?
Maybe should it trigger deprecations for each service in the loop too (i.e. tagged templating.*)?

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Jan 28, 2017

Member

@chalasr good catch, done.

Member

dunglas commented Jan 28, 2017

@chalasr good catch, done.

@dunglas dunglas moved this from In Progress to In Review in Lower entry bar Feb 13, 2017

@adrienrn

This comment has been minimized.

Show comment
Hide comment
@adrienrn

adrienrn Feb 24, 2017

One question remains on my end : templating was giving a layer of abstraction to twig and if @templating goes away, do we need to inject @twig directly or some other component will provide an abstraction ?

adrienrn commented Feb 24, 2017

One question remains on my end : templating was giving a layer of abstraction to twig and if @templating goes away, do we need to inject @twig directly or some other component will provide an abstraction ?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Mar 6, 2017

Member

Status: needs work

Member

nicolas-grekas commented Mar 6, 2017

Status: needs work

@robfrawley

This comment has been minimized.

Show comment
Hide comment
@robfrawley

robfrawley Mar 7, 2017

Contributor

As I've expressed on other issues on this matter, I know people that use this component and I am 👎 on moving to a forced-Twig environment (yes, I know people can create services and use them to perform their rendering using alternate templating engines quite easily, but it just seems unnessissary to remove the ability to use your own engine via the core).

Contributor

robfrawley commented Mar 7, 2017

As I've expressed on other issues on this matter, I know people that use this component and I am 👎 on moving to a forced-Twig environment (yes, I know people can create services and use them to perform their rendering using alternate templating engines quite easily, but it just seems unnessissary to remove the ability to use your own engine via the core).

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 23, 2017

Member

One alternative would be to move everything related to the Templating component from FrameworkBundle into a new bundle. Would that work?

Member

fabpot commented Mar 23, 2017

One alternative would be to move everything related to the Templating component from FrameworkBundle into a new bundle. Would that work?

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 23, 2017

Member

And move most of the classes back to the Templating component (something we do anyway as much as possible).

Member

fabpot commented Mar 23, 2017

And move most of the classes back to the Templating component (something we do anyway as much as possible).

@robfrawley

This comment has been minimized.

Show comment
Hide comment
@robfrawley

robfrawley Mar 23, 2017

Contributor

@fabpot Yeah, that sounds like a great alternative.

Contributor

robfrawley commented Mar 23, 2017

@fabpot Yeah, that sounds like a great alternative.

@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 3.3 Apr 3, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 2, 2017

Member

rebase needed

Member

nicolas-grekas commented Oct 2, 2017

rebase needed

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 8, 2017

Member

Moving to 4.1. Rebase on master needed, where PHP 7.1 features can be used btw.

Member

nicolas-grekas commented Oct 8, 2017

Moving to 4.1. Rebase on master needed, where PHP 7.1 features can be used btw.

@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 4.1 Oct 8, 2017

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Oct 16, 2017

Member

Remaining comments fixed.

Member

dunglas commented Oct 16, 2017

Remaining comments fixed.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Oct 16, 2017

Member

test are failing. I assume you need to add TwigBundle to require-dev of framework-bundle.

Member

Tobion commented Oct 16, 2017

test are failing. I assume you need to add TwigBundle to require-dev of framework-bundle.

@Tobion

Tobion approved these changes Oct 16, 2017

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Oct 16, 2017

Member

@Tobion right I was just pushing the commit

Member

dunglas commented Oct 16, 2017

@Tobion right I was just pushing the commit

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Oct 16, 2017

Member

The remaining failure is that TwigBridge >=3.2 is incompatible with TwigBundle < 3.2 since 812fbb4 because HttpKernelExtension requires HttpKernelRuntime. This would need a conflict rule in TwigBridge I assume.

The solution here could be to just require TwigBundle ~2.8|~3.2|~4.0

Member

Tobion commented Oct 16, 2017

The remaining failure is that TwigBridge >=3.2 is incompatible with TwigBundle < 3.2 since 812fbb4 because HttpKernelExtension requires HttpKernelRuntime. This would need a conflict rule in TwigBridge I assume.

The solution here could be to just require TwigBundle ~2.8|~3.2|~4.0

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Oct 17, 2017

Member

The services are only loaded if templating component is installed (https://github.com/symfony/symfony/blame/master/src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php#L50). That would be good enough for Flex but breaks for people using symfony/symfony. It needs to check for the templating services instead.

Member

Tobion commented Oct 17, 2017

The services are only loaded if templating component is installed (https://github.com/symfony/symfony/blame/master/src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php#L50). That would be good enough for Flex but breaks for people using symfony/symfony. It needs to check for the templating services instead.

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Oct 17, 2017

Member

Changing the check in the bundle extension won't work as it can't know if templating exists.
The twig.loader.filesystem service should be removed from ExtensionPass where we already remove templating.engine.twig https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/ExtensionPass.php#L80. it is already removed.
Both services need to be marked as deprecated in templating.xml though.

Member

chalasr commented Oct 17, 2017

Changing the check in the bundle extension won't work as it can't know if templating exists.
The twig.loader.filesystem service should be removed from ExtensionPass where we already remove templating.engine.twig https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/ExtensionPass.php#L80. it is already removed.
Both services need to be marked as deprecated in templating.xml though.

@javiereguiluz javiereguiluz referenced this pull request Oct 23, 2017

Closed

Extract the Templating component #15028

0 of 4 tasks complete
@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Nov 28, 2017

Contributor

Symfony\Bundle\SecurityBundle\Templating\ NS should be deprecated too right?

Hope to see this happening soon.

Status: needs work (outdated deprecation messages :))

Contributor

ro0NL commented Nov 28, 2017

Symfony\Bundle\SecurityBundle\Templating\ NS should be deprecated too right?

Hope to see this happening soon.

Status: needs work (outdated deprecation messages :))

@dunglas dunglas changed the base branch from 3.4 to master Dec 1, 2017

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Dec 1, 2017

Member

Rebased, message updated.

Member

dunglas commented Dec 1, 2017

Rebased, message updated.

@covex-nn covex-nn referenced this pull request Dec 25, 2017

Closed

Switch from Templating Component to Twig #456

3 of 3 tasks complete
@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas
Member

dunglas commented Dec 31, 2017

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Jan 30, 2018

Member

Btw, what this deprecation also means is that PHP templates for the Form component are deprecated as well. The php templating logic for forms is implemented in the framework bundle and the templates are located there as well.

So

  • Symfony\Component\Form\Extension\Templating\TemplatingExtension and Symfony\Component\Form\Extension\Templating\TemplatingRendererEngine should be deprecated as well. Those classes have no value without the stuff in the frameworkbundle.

* Support for the Templating component is provided by TemplatingExtension.
* This extension needs a PhpEngine object for rendering forms. As second
* argument you should pass the names of the default themes. Here is an
* example for using the default layout with "<div>" tags:
*
* <code>
* use Symfony\Component\Form\Extension\Templating\TemplatingExtension;
*
* $formFactory = Forms::createFormFactoryBuilder()
* ->addExtension(new TemplatingExtension($engine, null, array(
* 'FrameworkBundle:Form',
* )))
* ->getFormFactory();
* </code>
*
* The next example shows how to include the "<table>" layout:
*
* <code>
* use Symfony\Component\Form\Extension\Templating\TemplatingExtension;
*
* $formFactory = Forms::createFormFactoryBuilder()
* ->addExtension(new TemplatingExtension($engine, null, array(
* 'FrameworkBundle:Form',
* 'FrameworkBundle:FormTable',
* )))
* ->getFormFactory();
* </code>
can be removed

Member

Tobion commented Jan 30, 2018

Btw, what this deprecation also means is that PHP templates for the Form component are deprecated as well. The php templating logic for forms is implemented in the framework bundle and the templates are located there as well.

So

  • Symfony\Component\Form\Extension\Templating\TemplatingExtension and Symfony\Component\Form\Extension\Templating\TemplatingRendererEngine should be deprecated as well. Those classes have no value without the stuff in the frameworkbundle.

* Support for the Templating component is provided by TemplatingExtension.
* This extension needs a PhpEngine object for rendering forms. As second
* argument you should pass the names of the default themes. Here is an
* example for using the default layout with "<div>" tags:
*
* <code>
* use Symfony\Component\Form\Extension\Templating\TemplatingExtension;
*
* $formFactory = Forms::createFormFactoryBuilder()
* ->addExtension(new TemplatingExtension($engine, null, array(
* 'FrameworkBundle:Form',
* )))
* ->getFormFactory();
* </code>
*
* The next example shows how to include the "<table>" layout:
*
* <code>
* use Symfony\Component\Form\Extension\Templating\TemplatingExtension;
*
* $formFactory = Forms::createFormFactoryBuilder()
* ->addExtension(new TemplatingExtension($engine, null, array(
* 'FrameworkBundle:Form',
* 'FrameworkBundle:FormTable',
* )))
* ->getFormFactory();
* </code>
can be removed

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jan 30, 2018

Member

I would not deprecate templating support but move everything (the integration, not the component) to its own independent bundle (let say PHPTemplatingBundle) like we did for ACLs. The component itself is stable, so I don't see a need to move it out of core.

Member

fabpot commented Jan 30, 2018

I would not deprecate templating support but move everything (the integration, not the component) to its own independent bundle (let say PHPTemplatingBundle) like we did for ACLs. The component itself is stable, so I don't see a need to move it out of core.

@QuentinCurtet

This comment has been minimized.

Show comment
Hide comment
@QuentinCurtet

QuentinCurtet Feb 26, 2018

I have one question, does the use PHP templates will be deprecated in Symfony 4 ?

Thanks in advance.

QuentinCurtet commented Feb 26, 2018

I have one question, does the use PHP templates will be deprecated in Symfony 4 ?

Thanks in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment