[TwigBundle] Commands as a service #23519

ro0NL · 2017-07-15T10:48:33Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#...
License	MIT
Doc PR	symfony/symfony-docs#...

tiny step towards #23488

chalasr

Can we keep a legacy test for the LintCommand?

chalasr · 2017-07-16T10:02:00Z

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

@@ -34,15 +34,27 @@ class LintCommand extends Command
    private $twig;

    /**
-     * {@inheritdoc}
+     * @param Environment|string $twig


I would not make the old type an accepted one, only Environment

That's for bc :) we can keep it out of phpdoc though..?

Yes guessed it, I think the doc should reflect the new behavior only (I might be wrong)

chalasr · 2017-07-16T10:06:39Z

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

+        <service id="console.command.symfony_bundle_twigbundle_command_debugcommand" class="Symfony\Bridge\Twig\Command\DebugCommand">
+            <argument type="service" id="twig"/>
+            <tag name="console.command" command="debug:twig" />
+        </service>


deprecated="..."?

Not sure.. it's still picked up as the ought to be command service.

Perhaps if we do it vice versa; we can deprecate the alias. However tried that and it didnt work out causing auto discovery to override i believe.

Wait, this service and alias don't exist in 3.3 and lower? Since the command wasn't registered as a service but through convention

It's done to satisfy this check.

The deprecated command (in the bundle) will override (by command name) otherwise, as we dont skip deprecated commands here ;-) Note the new service points to the command in the bridge.

What about making TwigBundle::registerCommands() a noop instead?

Hehe 👍 could be a final resort. Shouldnt we add service aliases to the command map instead?

Adding console.command.symfony_bundle_twigbundle_command_debugcommand => false to the console.command.ids parameter? That's probably better regarding BC

Fixed! <defaults public="false" /> bite me :). The alias is now public and picked up. See console.xml

Still, it means that the dependencies of this command won't be loaded lazily anymore until 4.0 (the twig environment), that might break some consoles :)

Right.. i also realized i deprecated the alias, which probably wont work. We cant deprecate aliases. Although current XML is happily parsed.

So we basically need to preserve the old service (not as an alias). It will point to the old class and its definition is deprecated.

I believe that will conflict with auto discovery (deprecated will override), perhaps make a special case there. But i havent followed the code path for this approach yet.. maybe it just works :)

edit:

What about making TwigBundle::registerCommands() a noop instead?

Is probably the easiest indeed 👍

ro0NL · 2017-07-18T17:31:39Z

Legacy test added. @chalasr seems we dont have to make registerCommands a noop. It works as expected, the legacy command is not auto-discovered due its public service id.

This should be good to go :)

nicolas-grekas · 2017-07-19T05:29:06Z

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

@@ -29,15 +29,27 @@ class DebugCommand extends Command
    private $twig;

    /**
-     * {@inheritdoc}
+     * @param Environment $name


this should be future proof, ie the arg should be named $twig

nicolas-grekas · 2017-07-19T05:33:38Z

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

@@ -34,15 +34,27 @@ class LintCommand extends Command
    private $twig;

    /**
-     * {@inheritdoc}
+     * @param Environment $name


chalasr

CHANGELOG/UPGRADE files need to be updated

ro0NL · 2017-07-19T11:37:49Z

Done. Tedious job, that is :)

ogizanagi · 2017-07-19T17:52:00Z

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

@@ -29,15 +29,27 @@ class DebugCommand extends Command
    private $twig;

    /**
-     * {@inheritdoc}
+     * @param Environment $twig
     */
    public function __construct($name = 'debug:twig')


- public function __construct($name = 'debug:twig') + public function __construct($twig = 'debug:twig')

?

Not sure.. did that initially. But given we dont have to change it, this would be safest. So for BC really :)

edit: might look weird compared to phpdoc; although intended to clarify 4.0 sig. Perhaps the variable naming should be consistent here.. being either $twig or $name.. i dont mind.

@nicolas-grekas wdyt?

doc block and arg must have the same name for sure,
then I suggest $twig because that's the most future oriented, even if the default value looks strange.
One way to make it better would be to have null as default, and internally turn null into this string, in the deprecated code path. Might be the best in fact. (Same for the other command of course).

i preserved the sig for BC.. but thats from a reflection sniffing perspective. Which is nonsense i guess :)

__construct($twig = null) it is then 👍

ogizanagi · 2017-07-19T17:52:25Z

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

@@ -34,15 +34,27 @@ class LintCommand extends Command
    private $twig;

    /**
-     * {@inheritdoc}
+     * @param Environment $twig
     */
    public function __construct($name = 'lint:twig')


dunglas · 2017-07-19T20:37:38Z

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

@@ -29,15 +29,27 @@ class DebugCommand extends Command
    private $twig;

    /**
-     * {@inheritdoc}
+     * @param Environment $twig


xabbuh · 2017-07-21T12:10:30Z

UPGRADE-4.0.md

@@ -511,6 +511,8 @@ TwigBundle
 * The `ContainerAwareRuntimeLoader` class has been removed. Use the
  Twig `Twig_ContainerRuntimeLoader` class instead.

+ * Removed `ContainerAwareInterface` implementation in `Symfony\Bundle\TwigBundle\Command\LintCommand`


this is missing the DebugCommand class removal

Right! nice catch.

ogizanagi · 2017-07-21T13:17:38Z

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

+        if (!$twig instanceof Environment) {
+            @trigger_error(sprintf('Passing a command name as the first argument of "%s" is deprecated since version 3.4 and will be removed in 4.0. If the command was registered by convention, make it a service instead.', __METHOD__), E_USER_DEPRECATED);
+
+            $this->setName(null === $twig ? 'debug:twig' : $twig);


Minor, but it looks safe to me to simply use $this->setName($twig ?: 'debug:twig'); here.

?: !== ?? :) passing '0' previously worked as well (https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/Command/Command.php#L59)

Sure, but who would name this command 0? 😄
It won't even work with the symfony base Application and will juste execute the default command.

Yeah.. to me that should be null !== $name as well :)

symfony/src/Symfony/Component/Console/Input/InputInterface.php

Line 27 in 5129c4c

* @return string The value of the first argument or null otherwise

(or null otherwise)

If you insist i change it, just saying im preserving previous behavior :)

fabpot · 2017-07-22T07:51:23Z

Thank you @ro0NL.

This PR was squashed before being merged into the 3.4 branch (closes #23519). Discussion ---------- [TwigBundle] Commands as a service | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #...  | License | MIT | Doc PR | symfony/symfony-docs#...  tiny step towards #23488 Commits ------- 9839140 [TwigBundle] Commands as a service

Tobion · 2017-08-05T01:06:09Z

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

+    <services>
+        <defaults public="false" />
+
+        <service id="twig.command.debug" class="Symfony\Bridge\Twig\Command\DebugCommand">


are we not using class name == service id convention for new code?

This PR was merged into the 3.4 branch. Discussion ---------- [TwigBundle] require twig-bridge ~3.4 | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Injecting "twig" as first arg of the DebugCommand works only in ^3.4 - as of #23519. Commits ------- 03816b7 [TwigBundle] require twig-bridge ~3.4

carsonbot added Status: Needs Review TwigBundle Feature Deprecation labels Jul 15, 2017

chalasr approved these changes Jul 16, 2017

View reviewed changes

nicolas-grekas added this to the 3.4 milestone Jul 17, 2017

nicolas-grekas reviewed Jul 19, 2017

View reviewed changes

chalasr reviewed Jul 19, 2017

View reviewed changes

[TwigBundle] Commands as a service

d796f95

upgrade/changelog

42b71b2

chalasr approved these changes Jul 19, 2017

View reviewed changes

ogizanagi reviewed Jul 19, 2017

View reviewed changes

dunglas reviewed Jul 19, 2017

View reviewed changes

ro0NL added 3 commits July 20, 2017 10:22

Update DebugCommand.php

552f368

Update LintCommand.php

a2a58f5

Update DebugCommand.php

58194cd

xabbuh reviewed Jul 21, 2017

View reviewed changes

Update UPGRADE-4.0.md

08feece

ogizanagi approved these changes Jul 21, 2017

View reviewed changes

ogizanagi reviewed Jul 21, 2017

View reviewed changes

fabpot approved these changes Jul 22, 2017

View reviewed changes

fabpot closed this Jul 22, 2017

ro0NL deleted the twig/commands branch July 22, 2017 08:51

Tobion reviewed Aug 5, 2017

View reviewed changes

nicolas-grekas mentioned this pull request Aug 30, 2017

[TwigBundle] require twig-bridge ~3.4 #24039

Merged

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TwigBundle] Commands as a service #23519

[TwigBundle] Commands as a service #23519

ro0NL commented Jul 15, 2017 •

edited

Loading

chalasr left a comment

chalasr Jul 16, 2017

ro0NL Jul 16, 2017

chalasr Jul 16, 2017 •

edited

Loading

chalasr Jul 16, 2017

ro0NL Jul 16, 2017

chalasr Jul 16, 2017

ro0NL Jul 16, 2017

chalasr Jul 16, 2017

ro0NL Jul 16, 2017

chalasr Jul 16, 2017

ro0NL Jul 16, 2017

chalasr Jul 17, 2017

ro0NL Jul 18, 2017 •

edited

Loading

ro0NL commented Jul 18, 2017 •

edited

Loading

nicolas-grekas Jul 19, 2017

nicolas-grekas Jul 19, 2017

chalasr left a comment

ro0NL commented Jul 19, 2017

ogizanagi Jul 19, 2017

ro0NL Jul 19, 2017 •

edited

Loading

nicolas-grekas Jul 19, 2017

ro0NL Jul 20, 2017 •

edited

Loading

ogizanagi Jul 19, 2017

dunglas Jul 19, 2017

ro0NL Jul 20, 2017

xabbuh Jul 21, 2017

ro0NL Jul 21, 2017

ogizanagi Jul 21, 2017 •

edited

Loading

ro0NL Jul 21, 2017

ogizanagi Jul 21, 2017

ro0NL Jul 21, 2017 •

edited

Loading

fabpot commented Jul 22, 2017

Tobion Aug 5, 2017

[TwigBundle] Commands as a service #23519

[TwigBundle] Commands as a service #23519

Conversation

ro0NL commented Jul 15, 2017 • edited Loading

chalasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Jul 16, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

ro0NL commented Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr left a comment

Choose a reason for hiding this comment

ro0NL commented Jul 19, 2017

Choose a reason for hiding this comment

ro0NL Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Jul 20, 2017 • 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

ogizanagi Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

fabpot commented Jul 22, 2017

Choose a reason for hiding this comment

ro0NL commented Jul 15, 2017 •

edited

Loading

chalasr Jul 16, 2017 •

edited

Loading

ro0NL Jul 18, 2017 •

edited

Loading

ro0NL commented Jul 18, 2017 •

edited

Loading

ro0NL Jul 19, 2017 •

edited

Loading

ro0NL Jul 20, 2017 •

edited

Loading

ogizanagi Jul 21, 2017 •

edited

Loading

ro0NL Jul 21, 2017 •

edited

Loading