Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[TwigBundle] Commands as a service#23519
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
chalasr left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we keep a legacy test for theLintCommand?
| /** | ||
| *{@inheritdoc} | ||
| *@param Environment|string $twig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would not make the old type an accepted one, onlyEnvironment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's for bc :) we can keep it out of phpdoc though..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes guessed it, I think the doc should reflect the new behavior only (I might be wrong)
| <serviceid="console.command.symfony_bundle_twigbundle_command_debugcommand"class="Symfony\Bridge\Twig\Command\DebugCommand"> | ||
| <argumenttype="service"id="twig"/> | ||
| <tagname="console.command"command="debug:twig" /> | ||
| </service> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
deprecated="..."?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's done to satisfythis 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about makingTwigBundle::registerCommands() a noop instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hehe 👍 could be a final resort. Shouldnt we add service aliases to the command map instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Addingconsole.command.symfony_bundle_twigbundle_command_debugcommand => false to theconsole.command.ids parameter? That's probably better regarding BC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed!<defaults public="false" /> bite me :). The alias is now public and picked up. See console.xml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 commentedJul 18, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Legacy test added.@chalasr seems we donthave 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 :) |
| /** | ||
| *{@inheritdoc} | ||
| *@param Environment $name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this should be future proof, ie the arg should be named $twig
| /** | ||
| *{@inheritdoc} | ||
| *@param Environment $name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$twig
chalasr left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
CHANGELOG/UPGRADE files need to be updated
ro0NL commentedJul 19, 2017
Done. Tedious job, that is :) |
| *{@inheritdoc} | ||
| *@param Environment $twig | ||
| */ | ||
| publicfunction__construct($name ='debug:twig') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
- public function __construct($name = 'debug:twig')+ public function __construct($twig = 'debug:twig')
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
i preserved the sig for BC.. but thats from a reflection sniffing perspective. Which is nonsense i guess :)
__construct($twig = null) it is then 👍
| *{@inheritdoc} | ||
| *@param Environment $twig | ||
| */ | ||
| publicfunction__construct($name ='lint:twig') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$twig
| /** | ||
| *{@inheritdoc} | ||
| *@param Environment $twig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Patched.
UPGRADE-4.0.md Outdated
| * The`ContainerAwareRuntimeLoader` class has been removed. Use the | ||
| Twig`Twig_ContainerRuntimeLoader` class instead. | ||
| * Removed`ContainerAwareInterface` implementation in`Symfony\Bundle\TwigBundle\Command\LintCommand` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is missing theDebugCommand class removal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Right! nice catch.
| if (!$twiginstanceof 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); |
ogizanagiJul 21, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Minor, but it looks safe to me to simply use$this->setName($twig ?: 'debug:twig'); here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
?: !== ?? :) passing'0' previously worked as well (https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/Command/Command.php#L59)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure, but who would name this command0? 😄
It won't even work with the symfony baseApplication and will juste execute the default command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah.. to me that should benull !== $name as well :)
| * @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 commentedJul 22, 2017
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 | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->tiny step towards#23488Commits-------9839140 [TwigBundle] Commands as a service
| <services> | ||
| <defaultspublic="false" /> | ||
| <serviceid="twig.command.debug"class="Symfony\Bridge\Twig\Command\DebugCommand"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
Uh oh!
There was an error while loading.Please reload this page.
tiny step towards#23488