use twig loader to validate template's existence #397

ElectricMaxxx · 2017-09-25T04:47:11Z

Q	A
Bug fix?	[no]
New feature?	[no]
BC breaks?	[no]
Deprecations?	[no]
Tests pass?	[yes]
Fixed tickets	[#396]
License	MIT

fix #396

I replaced templating by a twig loader based on the comment of @fabpot (maybe you can give us a internal in review)
symfony/recipes-contrib#91 (comment)

@dbu @wouterj I have no clue if we can release that on a patch release or if it is an BC-Break. From my POV we didn't change any external API, so a patch release would be ok.

ElectricMaxxx · 2017-09-25T04:50:25Z

The build fail is based on a parameter kernel.project_dir, which exists on Symfony 3.3 and not before. I think we can set the twig.path on whatever we whant in testing, in an app the user should set it to a value of its needs.

Edit:
We do not need to set the twig.paths configuration at all. But we should document to set it or is it set on %kernel.project_dir%/templates by default?

dbu

thanks for working on this. i feel that we should try to be BC for now, and keep supporting the templating service if its available. either with expression language or in a compiler pass that changes the route_defaults validator service depending on whether templating is available in the container or not.

dbu · 2017-09-25T07:23:31Z

composer.json

@@ -14,7 +14,8 @@
    "require": {
        "php": "^5.6|^7.0",
        "symfony-cmf/routing": "^2.0",
-        "symfony/framework-bundle": "^2.8|^3.0"
+        "symfony/framework-bundle": "^2.8|^3.0",
+        "symfony/twig-bundle": "^2.8|^3.0"


are you sure we need the twig bundle? would twig/twig not be enough?

ah sorry, we need the bundle because we want the twig.loader service. this change is fine imho.

dbu · 2017-09-25T07:25:01Z

src/Validator/Constraints/RouteDefaultsValidator.php

+     */
+    private $twig;
+
+    public function __construct(ControllerResolverInterface $controllerResolver, LoaderInterface $twig)


if we would omit the typehint here, and validate the class in the constructor to be either loader or engine, we could be BC. not sure what the best practice is for that.

dbu · 2017-09-25T07:26:01Z

src/Resources/config/validators.xml

@@ -7,7 +7,7 @@
    <services>
        <service id="cmf_routing.validator.route_defaults" class="Symfony\Cmf\Bundle\RoutingBundle\Validator\Constraints\RouteDefaultsValidator">
            <argument id="controller_resolver" type="service"/>
-            <argument id="templating" type="service"/>
+            <argument id="twig.loader" type="service"/>


i guess the real BC break is this. what if people use a non-twig template? then things will suddenly stop working. could we find a way to use templating if it exists, but twig.loader otherwise? maybe with an expression language check?

I would suggest to do it like here:
https://github.com/symfony/framework-bundle/blob/master/Controller/ControllerTrait.php#L211
in a kind of compiler pass. We would have two validator classes then. If the user does a composer require symfony/templating a validator using templating would overwrite the default validator using twig in our container.

But we have to document that symfony/templating should be added manually from Symfony 3.2 on, cause it was removed from frameworkbundle.

+1 for the compiler pass to update the service definition for the validator to use the templating service if available. i would just remove the typehint and say it must be one or the other class.

we never required the templating here, so explaining how to update is "out of scope" and part of upgrading symfony itself. we can just explain what happened in the changelog, like " RoutingBundle can now also directly work with Twig. Symfony FrameworkBundle no longer requires symfony/templating since 3.2 and the component is deprecated. If the templating component is available in your application, it is however still used for BC."

I would avoid to remove the type hint, cause the fact that both services got the same method is random. So yes, we should have two services, which can be changed by a compiler pass.

to the templating problem as a dependency:
When having a Symfony App with version >= 3.2 and doing a simple composer require symfony-cmf/routing-bundle to add routing functions, you will get an error that templating service isn't available. The reason is: templating was removed from framework-bundle. So have to do a composer require symfony/templating before for those apps. So requiring templating is not a question of which templating engine to chose, it is a question of the symfony version.

ElectricMaxxx · 2017-09-25T14:06:44Z

I think we will get the same trouble in content-bundle, where our Controller uses the engine.

ElectricMaxxx · 2017-09-25T14:38:29Z

src/DependencyInjection/Compiler/TemplatingValidatorPass.php

+     */
+    public function process(ContainerBuilder $container)
+    {
+        if (class_exists(EngineInterface::class) && $container->has('templating')) {


templating could exists in FrameworkBundle (before Symfony 3.2) and in a single namespace (3.2 and later). The one in FrameworkBundle is the one we need to use, as it is the one we used before. The one in the new component looks same but got an other namespace and would break the contructor then. So i will cover with this check the former templating to avoid a BC-Break. From Symfony 3.2 the user should use TwigBundle. But i have no clue how to test that one except from checking it out in sandbox with different symfony versions, any other idea?

dbu

appart from the changelog, this looks fine to me. i would not have separated the validator class and relied on the fact that the method is called the same, but it indeed is cleaner like this. we should delete the templating support (compiler class and validator and its test) for the next major version. lets see if symfony 4 pushes us to do a major version change.

dbu · 2017-09-25T15:31:46Z

CHANGELOG.md

@@ -1,6 +1,9 @@
 Changelog
 =========

+* **2017-02-17**: Remove usage of `templating` component to validate template existence,
+  use twig loader instead.


can we change this to "This bundle can now also directly use the Twig loader instead of the deprecated templating component. Symfony FrameworkBundle no longer requires symfony/templating since 3.2. If the templating component is available in your application, it is however still used for BC."

and please check the date, i'd guess 2017-09-25 would be correct

I should also add the path to the bundlle ;-)

When removing it later, we should deprecate it.

ElectricMaxxx

just a question of grammar and writing :-)

ElectricMaxxx · 2017-09-26T03:23:11Z

src/Validator/Constraints/RouteDefaultsTemplatingValidator.php

@@ -17,9 +17,18 @@
 use Symfony\Component\Validator\Constraint;
 use Symfony\Component\Validator\ConstraintValidator;

-class RouteDefaultsValidator extends ConstraintValidator
+/**
+ * @deprecated Will be removed in 3.0. Use RouteDefaultsTwigValidator with twig as template engine instead.


@dbu is that notice got enough?

dbu · 2017-09-26T07:31:50Z

src/DependencyInjection/Compiler/TemplatingValidatorPass.php

+            $templatingDefinition = $container->getDefinition('templating');
+            $validatorDefinition = $container->getDefinition('cmf_routing.validator.route_defaults');
+            $validatorDefinition->setClass(RouteDefaultsTemplatingValidator::class);
+            $validatorDefinition->setArgument('$templating', $templatingDefinition);


does this syntax work with all symfony versions or was that added in 3.2 or so?

you can be right, That syntax is new, so i need the index of the argument. Is the counting 0-based?

yes, its 0 based. can you check in the sandbox or so if it actually works? i think building a test is a lot of hassle, i am ok if we know it worked at the time we add it...

we do not need to define a twig path at all remove use statement really remove use statement Apply fixes from StyleCI [ci skip] [skip ci] use two services and use templating one, when FrameworkBundle's templating exists use the correct class add compiler pass to container add deprecation notice use argument index instead of parameters name get it running in sandbox use replace method remove debug messages

ElectricMaxxx · 2017-09-26T20:42:05Z

uhh, this was a hard thing at the end, but now it is green and it works.

stof · 2017-09-28T08:18:02Z

src/DependencyInjection/Compiler/TemplatingValidatorPass.php

+            $templatingDefinition = $container->findDefinition('templating');
+            $validatorDefinition = $container->getDefinition('cmf_routing.validator.route_defaults');
+            $validatorDefinition->setClass(RouteDefaultsTemplatingValidator::class);
+            $validatorDefinition->replaceArgument(1, $templatingDefinition);


this is wrong. You must not copy the definition in the argument. This would create a separate instance of the service. You must use new Reference('templating') as value of the argument, to reference the existing service

@stof ~~but i would then inject it by replaceArgument() right?~~ (you wrote it g) but thanks, had some trouble how to use aliases with service definitions.

ElectricMaxxx requested a review from dbu September 25, 2017 04:57

dbu reviewed Sep 25, 2017

View reviewed changes

ElectricMaxxx commented Sep 25, 2017

View reviewed changes

dbu reviewed Sep 25, 2017

View reviewed changes

ElectricMaxxx mentioned this pull request Sep 26, 2017

templating to be BC symfony-cmf/content-bundle#177

Closed

ElectricMaxxx commented Sep 26, 2017

View reviewed changes

dbu reviewed Sep 26, 2017

View reviewed changes

ElectricMaxxx force-pushed the remove_templating_component branch from b80d9d5 to 36ea55c Compare September 26, 2017 20:41

ElectricMaxxx merged commit 3ded693 into master Sep 26, 2017

ElectricMaxxx deleted the remove_templating_component branch September 26, 2017 20:43

stof reviewed Sep 28, 2017

View reviewed changes

use twig loader to validate template's existence #397

use twig loader to validate template's existence #397

Uh oh!

Conversation

ElectricMaxxx commented Sep 25, 2017

Uh oh!

ElectricMaxxx commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElectricMaxxx commented Sep 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElectricMaxxx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElectricMaxxx commented Sep 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElectricMaxxx Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ElectricMaxxx commented Sep 25, 2017 •

edited

Loading

ElectricMaxxx Sep 28, 2017 •

edited

Loading