[Twig] Decouple Twig commands from the Famework by GromNaN · Pull Request #9855 · symfony/symfony

GromNaN · 2013-12-24T10:14:34Z

I want to use the command twig:lint in a Silex project.

In this PR, I've moved the class Symfony\Bundle\TwigBundle\Command\LintCommand to Symfony\Bridge\Twig\Command\LintCommand and removed dependency to the ContainerAwareCommand.

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	yes (renamed class)
Deprecations?	no
Tests pass?	yes
Fixed tickets	#9818 (comment)
License	MIT
Doc PR	n/a

Move command twig:debug once merged.
Lazy load twig service

cordoval · 2013-12-24T10:16:29Z

src/Symfony/Bridge/Twig/Command/LintCommand.php

why not just Twig_Environment or is the \ also needed here?

If you remove the \, it becomes a relative class name, and you need to add a use statement for it. and Symfony does not add use statements for classes of the global namespace

cordoval · 2013-12-24T10:19:01Z

nice, great move decoupling ftw! 👍

stof · 2013-12-24T10:21:56Z

src/Symfony/Bridge/Twig/Command/LintCommand.php

You don't need this getter IMO

My idea is that someone can extend the command class to customize how the twig instance is located.

then you should make $this->twig protected and remove this getter, following the Symfony standards

And I don't even think it make sense. Someone extending the command will still need to call the parent constructor, and so would still need to inject twig through the constructor

Method removed.

GromNaN · 2013-12-24T11:50:20Z

In addition, I've just added tests for the command.

ggam · 2013-12-24T12:22:19Z

I think you should add a note on the changelog about the BC break.

lyrixx · 2013-12-26T17:10:30Z

src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml

Happy to see this feature used.
But two little questions: did you try this in a symfony2 project ? And does this make the application slower ?
Because in SF2 project, twig depends of lot of things (like security...).

This is actually an issue with the commands defined as service (introduced in 2.4): commands are not loaded lazily, so all their dependency graph is instantiated for any unrelated console call

It would be better if the getDefinition method was static.

fabpot · 2013-12-28T12:02:57Z

This one cannot be merged before we find a solution for lazy-loading console command as services.

realityking · 2013-12-28T12:15:54Z

Couldn't this be done by moving it to the bridge as an abstract class with an abstract method getTwigEnvironment? This would mean no change for Symfony2 but make it easier reusable outside of it.

fabpot · 2013-12-28T13:36:39Z

Whatever we decide to do here should be done also in #9855

realityking · 2013-12-28T13:47:26Z

src/Symfony/Bridge/Twig/Command/LintCommand.php

Please allow passing the name trough, sometime one wants to rename a command for consistency.

Actually, it's not possible to set a name in the constructor as it will be overwritten by the method configure(). The only solution is to set the command name explicitly with setName().

It's very easy to implement: just add a $name parameter here with the standard name as default and pass it to parent::__construct(). After that, remove it from configure

could you please also add this feature?

wouterj · 2013-12-28T14:03:51Z

@fabbot I think something did go wrong, you linked to the same PR...

And I think this can be solved by overriding this command in the TwigBundle:

namespace Symfony\Bundle\TwigBundle\Command;

use Symfony\Bridge\Twig\Command\LintCommand as BaseLintCommand;
use Symfony\Component\DependencyInjection\ContainerAwareInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;

class LintCommand extends BaseLintCommand implements ContainerAwareInterface
{
    protected $container;

    public function __construct()
    {
        // override constructor to not set the twig environment yet
        parent::__construct(new \Twig_Environment());
    }

    public function setContainer(ContainerInterface $container)
    {
        $this->container = $container;
        $this->twig = $container->get('twig');
    }
}

This requires the twig property to be protected though. Or we can use the getTwig method to retrieve the environment and override that.

Then we can investigate more time into lazy loading service commands and after that, we can remove this "hack".

…d twig:lint

…itialized

GromNaN · 2013-12-30T00:28:57Z

Seeing that using "commands as services" requires initializing a deep dependency tree each time the application is run, I can't find a solution to lazy-inject the twig service into the command.

So, I think that's better not using the "commands as a service" feature; even if this is a very brilliant approach. In my last commit, I've followed the suggestions from @realityking and @wouterj and I've created a class in the bundle that extends the class in the bridge and retrieve the service from the container.

fabpot · 2013-12-30T06:31:15Z

src/Symfony/Bridge/Twig/CHANGELOG.md

I would say moved instead of added

fabpot · 2013-12-30T06:32:53Z

... and tests are broken.

wouterj · 2013-12-30T08:57:25Z

src/Symfony/Bundle/TwigBundle/Command/LintCommand.php

Afaik, container can't be null,as it's set by setContainer during initialization?

Exact, I took that portion of code from ContainerAwareCommand; but it looks like a legacy fallback.
I've removed it.

…ainerAwareInterface instances

GromNaN · 2013-12-30T16:43:14Z

@fabpot everything as been fixed.

realityking · 2013-12-30T16:43:57Z

src/Symfony/Bridge/Twig/Command/LintCommand.php

I'd make this abstract and remove the setter. This would prevent making the setter non-functioning in the Bundle Command.

As a developer, I don't want to be forced to create a class to use this command.

cordoval reviewed Dec 24, 2013
View reviewed changes

stof reviewed Dec 24, 2013
View reviewed changes

lyrixx reviewed Dec 26, 2013
View reviewed changes

realityking reviewed Dec 28, 2013
View reviewed changes

GromNaN added 5 commits December 30, 2013 00:55

Move the command "tiwg:lint" to the bridge

8861f9c

Updage changelog and minimum compatibility versions for moving comman…

b8bd4aa

…d twig:lint

Test command twig:lint

1124bb6

Removed getTwig for the command as it was unecessary

d500749

Remove command as a service as it requires too many services to be in…

678eddf

…itialized

fabpot reviewed Dec 30, 2013
View reviewed changes

Fix test, inline method and fix typo

a8fd92e

wouterj reviewed Dec 30, 2013
View reviewed changes

GromNaN added 2 commits December 30, 2013 17:33

Makes command name configurable in the constructor

b008f49

Remove useless fallback as the container is always injected into Cont…

3696d04

…ainerAwareInterface instances

realityking reviewed Dec 30, 2013
View reviewed changes

Uh oh!

Conversation

GromNaN commented Dec 24, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cordoval commented Dec 24, 2013

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!

GromNaN commented Dec 24, 2013

Uh oh!

ggam commented Dec 24, 2013

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!

fabpot commented Dec 28, 2013

Uh oh!

realityking commented Dec 28, 2013

Uh oh!

fabpot commented Dec 28, 2013

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!

wouterj commented Dec 28, 2013

Uh oh!

GromNaN commented Dec 30, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Dec 30, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GromNaN commented Dec 30, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants