[Console] enable describing commands in ways that make the `list` command lazy #39851

nicolas-grekas · 2021-01-15T12:48:15Z

Q	A
Branch?	5.x
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #33804
License	MIT
Doc PR	-

This PR improves the way one can describe a command so that the list command can be made lazy:

when provided using the $defaultName property or the console.command tag, the name of a command is now exploded using the | character. The first name in the list defines the name of the command, the other ones its aliases. When the first name is the empty string, the second name is used instead, and the command is declared as hidden.
a new $defaultDescription static property and a new description tag attribute allow for defining the commands' description while registering them.

Together, this is enough to make the list command lazy, because this command only accesses each command's name, aliases, hidden-status, and description.

On the implementation side, this PR adds a LazyCommand class that proxies regular commands to make them lazy for the target purpose.

This PR will enable support for attributes for configuring a command name+description+etc.
e.g. using the concepts in #39804:
#[CommandAutoTag(name: 'foo:bar', desc: 'boo', hidden: true)]#

The attribute could very well split the hidden and aliases settings apart - while the underlying code and pre-PHP8 apps would use the compact form, because dealing with many static properties + methods would be a maintenance pain imho.

chalasr · 2021-01-15T13:30:35Z

Looks really nice, thank you.
I'm going to challenge the new convention for hidden commands as I had to stare at it for a while before getting what it means. I'm not sure that the command visibility should be defined through its name, at least it's not obvious that an empty string before the first separator should be the way to control it.
Could we use a special char in front of the name (. ?) or add another static prop+tag attr?

nicolas-grekas · 2021-01-15T13:32:23Z

Not hidden:
$defaultName = 'app:foo|foo'

Hidden:
$defaultName = '|app:foo|goo'

I don't think this is worth adding another static property.

javiereguiluz · 2021-01-15T14:26:23Z

I don't like much the proposed convention to define a command as hidden because it's not self-explanatory.

As an alternative, would it be possible to store the list of commands in the cache dir while "compiling" the app? Same as we do with routes. Thanks.

nicolas-grekas · 2021-01-15T14:30:36Z

IMHO, there is no need to optimize for setting the "hidden" flag, as that's not a mainstream feature. It's readable enough as is.

would it be possible to store the list of commands in the cache dir while "compiling" the app? Same as we do with routes. Thanks.

Sorry, I don't get what you mean, can you please describe your idea a bit more? What's the DX that goes with this?

noniagriconomie · 2021-01-15T14:57:45Z

nice to have this lazy feature to be added :)

my 2cts, why not .app:foo|foo for hidden? as a hidden Unix file for example

that way:

the | for separating command name first and aliases next
the . to hide the command

jderusse · 2021-01-15T15:05:48Z

Instead of adding static property for everything, what's about making the method configure static?.

class AppCommand extends Command implements StaticCommandDescribedInterface
{
  public static function configure(): CommandDescription
  {
    return new CommandDescription('app:test')
      ->setDefinition()
      ->setHelp()
      ->addInput(...)
    ;
  }
}

class that can't be static and need internal service to resolve could implement another interface (that is incompatible with the previous)

class AppCommand extends Command implements InstanceCommandDescribedInterface
{
  public function configure(): CommandDescription
  {
    return new CommandDescription('app:test')
      ->setDefinition()
      ->setHelp($this->getHint())
      ->addInput(...)
    ;
  }
}

nicolas-grekas · 2021-01-15T15:10:06Z

@jderusse that would kill the possibility to make the command configuration sensitive to constructor argument. Aka that could kill a lot of flexibility in the class. Yes, we don't use/need that in typical Symfony DI, but the console component is very much used standalone. We should be sure to not degrade it's DX when used outside of a typical Symfony stack.

From a SOLID pov, static methods are just bad.

nicolas-grekas · 2021-01-15T15:11:05Z

why not .app:foo|foo for hidden

because it's not better than the current proposal?

jderusse · 2021-01-15T15:26:43Z

@jderusse that would kill the possibility to make the command configuration sensitive to constructor argument. Aka that could kill a lot of flexibility in the class. Yes, we don't use/need that in typical Symfony DI, but the console component is very much used standalone. We should be sure to not degrade it's DX when used outside of a typical Symfony stack.

From a SOLID pov, static methods are just bad.

Isn't it addressed by the second part of my comment?

nicolas-grekas · 2021-01-15T15:33:24Z

@jderusse to not break BC, we should rather make static version opt-in. I don't know where this could lead, in terms of BC impact for example. Of course, feel free to give it a try, as I did here with my proposal.

natewiebe13 · 2021-01-15T15:37:45Z

why not .app:foo|foo for hidden

because it's not better than the current proposal?

I think it's more clear as to what the intention is. Prefixing with a dot is common convention for hiding a file/directory in Linux and Mac OS. Whereas using the separator as a prefix, it isn't clear without first reading documentation that it's hiding the command instead of just a typo.

stof · 2021-01-15T15:41:01Z

@natewiebe13 but this reserves 2 chars (. and |) which cannot be used anymore in command names. | is fine as a reserved char is fine, as no one would use that char in their command name: | is a special char in all shells, and so using the command would require escaping it all the time.
But . has no special meaning and could totally be used in command names (even though that's not the recommended Symfony naming convention).

stof · 2021-01-15T15:43:11Z

Isn't it addressed by the second part of my comment?

but that forbids lazy-loading these commands then, while the use case for relying in the constructor arguments in the configuration is much more likely to be related to configuring options or arguments than the description.

natewiebe13 · 2021-01-15T15:44:44Z

@natewiebe13 but this reserves 2 chars (. and |) which cannot be used anymore in command names. | is fine as a reserved char is fine, as no one would use that char in their command name: | is a special char in all shells, and so using the command would require escaping it all the time.
But . has no special meaning and could totally be used in command names (even though that's not the recommended Symfony naming convention).

But wouldn't that just be for the first char in a command name? I wouldn't expect any periods in the command name to cause issues, just if the name was prefixed by a period.

Edit
For example, I'd still think .app.do-something would still be valid as app.do-something. It would just be hidden.

stof · 2021-01-15T15:46:06Z

@nicolas-grekas there is still the issue of Command::isEnabled() though. It impacts list as well.

nicolas-grekas · 2021-01-15T15:53:01Z

there is still the issue of Command::isEnabled() though. It impacts list as well.

Isn't this covered? See the implementation of isEnabled(). Otherwise, I may have missed what you meant.

nicolas-grekas · 2021-01-15T15:55:03Z

I wouldn't expect any periods in the command name to cause issues, just if the name was prefixed by a period.

Until one will report a BC break :) This happens all the time when we do such assumptions.

Honestly, the leading pipe is fine, it's as readable. Everything needs to be taught anyway.

stof · 2021-01-15T16:01:11Z

Isn't this covered? See the implementation of isEnabled(). Otherwise, I may have missed what you meant.

disabled commands are excluded by list

ro0NL · 2021-01-15T16:22:25Z

@stof pipe char may be quoted 😅 im fine with it, otherwise static $defaultAliases & co

@wouterj

…nicolas-grekas) This PR was merged into the 5.3-dev branch. Discussion ---------- [Console] add option `--short` to the `list` command | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - This PR is a follow up of symfony/symfony#39851, triggered by @wouterj's comment at symfony/symfony#39851 (review). This new option should enable creating fast shell auto-completion, by allowing the `list` command to run fast. Commits ------- 81d5728f4a [Console] add option `--short` to the `list` command

With the Symfony PR symfony/symfony#39851 new depndency injection console.command tag properties are supported, which allow to inject the description and the hidden state into console commands. In order to maintain a consistent console.command DI tag syntax between Symfony and TYPO3 core, we now add forward compatibility for this change. Releases: master, 10.4 Resolves: #93425 Related: #93174 Change-Id: Ie5dac65deaede2099f2d337466295bd2815ce918 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/67639 Tested-by: TYPO3com <noreply@typo3.com> Tested-by: Helmut Hummel <typo3@helhum.io> Tested-by: Benni Mack <benni@typo3.org> Tested-by: Benjamin Franzke <bfr@qbus.de> Reviewed-by: Helmut Hummel <typo3@helhum.io> Reviewed-by: Benni Mack <benni@typo3.org> Reviewed-by: Benjamin Franzke <bfr@qbus.de>

With the Symfony PR symfony/symfony#39851 new depndency injection console.command tag properties are supported, which allow to inject the description and the hidden state into console commands. In order to maintain a consistent console.command DI tag syntax between Symfony and TYPO3 core, we now add forward compatibility for this change. Releases: master, 10.4 Resolves: #93425 Related: #93174 Change-Id: Ie5dac65deaede2099f2d337466295bd2815ce918 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/67600 Tested-by: TYPO3com <noreply@typo3.com> Tested-by: Benjamin Franzke <bfr@qbus.de> Reviewed-by: Benjamin Franzke <bfr@qbus.de>

Based on the configuration syntax of the Symfony Console feature symfony/symfony#39851 …but implemented differently, using a registry pattern rather then a lazy-object pattern (like symfony does). Main motiviation for the registry pattern is following: Symfony LazyCommand wrappers add quite some complexity only for the sake of the list command, we already got lazy commands (in terms of execution) as our CommandRegistry implements the ConfigurationLoaderInterface that has been introduced by 2017 to add support for lazy commands. Now, that means we already got a registry for lazy commands, so it is logical to add lazy description handling there as well. We want to assure that the command list will never instantiate any commands. This is in constrast to the Symfony core LazyCommand approach, where legacy commands, that do not provide a compile time description, would still be instantiated during console command list. Also commands that return false in `isEnabled()` are now listed. That means enabled state is only evaluated during runtime. Therefore the special `dumpautoload` command is transformed into a lowlevel command in order to be hidden dependending on being run in composer-mode or not. Releases: master Resolves: #93174 Change-Id: Ifa68404cc81c64a335be30f2263a7eb17de0624d

Based on the configuration syntax of the Symfony Console feature symfony/symfony#39851 …but implemented differently, using a registry pattern rather then a lazy-object pattern (like symfony does). Main motiviation for the registry pattern is following: Symfony LazyCommand wrappers add quite some complexity only for the sake of the list command, we already got lazy commands (in terms of execution) as our CommandRegistry implements the ConfigurationLoaderInterface that has been introduced by 2017 to add support for lazy commands. Now, that means we already got a registry for lazy commands, so it is logical to add lazy description handling there as well. We want to assure that the command list will never instantiate any commands. This is in constrast to the Symfony core LazyCommand approach, where legacy commands, that do not provide a compile time description, would still be instantiated during console command list. Also commands that return false in `isEnabled()` are now listed. That means enabled state is only evaluated during runtime. Therefore the special `dumpautoload` command is transformed into a lowlevel command in order to be hidden dependending on being run in composer-mode or not. Releases: master Resolves: #93174 Change-Id: Ifa68404cc81c64a335be30f2263a7eb17de0624d Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/67635 Tested-by: TYPO3com <noreply@typo3.com> Tested-by: core-ci <typo3@b13.com> Tested-by: Benni Mack <benni@typo3.org> Tested-by: Helmut Hummel <typo3@helhum.io> Reviewed-by: Benni Mack <benni@typo3.org> Reviewed-by: Helmut Hummel <typo3@helhum.io>

… commands on PHP 8 (nicolas-grekas) This PR was merged into the 5.3-dev branch. Discussion ---------- [Console] Add `ConsoleCommand` attribute for declaring commands on PHP 8 | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Builds on #39851 On PHP8, this PR will allow using an attribute instead of the public static properties for the name and the description. ```php #[ConsoleCommand( name: 'app:my-command', description: '🌈', hidden: true, aliases: ['🌈'], )] class MyCommand extends Command { } ``` Commits ------- 0cbc9cc [Console] Add `ConsoleCommand` attribute for declaring commands on PHP 8

…nd in console (DemigodCode) This PR was merged into the 5.3-dev branch. Discussion ---------- [DebugBundle] Remove warning of ServerDumpPlaceholderCommand in console | Q | A | ------------- | --- | Branch? | 5.2.5 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #40495 | License | MIT | Doc PR | In 5.2.5 the console commands are lazy. (#39851) With this change the ServerDumpCommand::$defaultName is used which isn't set in the placeholder command. If no vardump-server in debug.dump_destination is defined, this will lead to a warning and not adding the command to the console list. Commits ------- 37b2a19 [DebugBundle] Add $defaultName to PlaceholderCommand

This allow Symfony to make the command lazy-loadable and thus avoiding instantiation of every processor just to list applications command. See symfony/symfony#39851 for more info

This allows Symfony to make the command lazy-loadable and thus avoiding instantiation of every processors only to list application's commands. See symfony/symfony#39851 for more info

nicolas-grekas added this to the 5.x milestone Jan 15, 2021

nicolas-grekas requested a review from chalasr as a code owner January 15, 2021 12:48

carsonbot added Status: Needs Review Console Feature labels Jan 15, 2021

nicolas-grekas mentioned this pull request Jan 15, 2021

[Console] Add LazyCommandTrait to help writing lazy commands #39726

Closed

chalasr approved these changes Jan 15, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 15, 2021

nicolas-grekas changed the title ~~[Console] enable describing commands in ways that makes the list command lazy~~ [Console] enable describing commands in ways that make the list command lazy Jan 15, 2021

nicolas-grekas force-pushed the console-attr branch from 39fab1f to a340975 Compare January 15, 2021 13:55

This was referenced Feb 17, 2021

[Uid] Add Generate and Inspect commands #39883

Merged

[Console] Add ConsoleCommand attribute for declaring commands on PHP 8 #40234

Merged

DemigodCode mentioned this pull request Mar 17, 2021

[DebugBundle] Remove warning of ServerDumpPlaceholderCommand in console #40498

Merged

fabpot mentioned this pull request Apr 18, 2021

Release v5.3.0-BETA1 #40853

Merged

mabar mentioned this pull request May 13, 2021

Lazy console command list nextras/migrations#116

Merged

Ocramius mentioned this pull request Nov 5, 2021

Upgrade CI setup, configure Dependabot with auto-merge Roave/BackwardCompatibilityCheck#320

Merged

tucksaun mentioned this pull request Mar 2, 2022

Define command description as static property swarrot/SwarrotBundle#220

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] enable describing commands in ways that make the `list` command lazy #39851

[Console] enable describing commands in ways that make the `list` command lazy #39851

nicolas-grekas commented Jan 15, 2021 •

edited

Loading

chalasr commented Jan 15, 2021

nicolas-grekas commented Jan 15, 2021

javiereguiluz commented Jan 15, 2021

nicolas-grekas commented Jan 15, 2021

noniagriconomie commented Jan 15, 2021

jderusse commented Jan 15, 2021

nicolas-grekas commented Jan 15, 2021

nicolas-grekas commented Jan 15, 2021

jderusse commented Jan 15, 2021

nicolas-grekas commented Jan 15, 2021

natewiebe13 commented Jan 15, 2021

stof commented Jan 15, 2021

stof commented Jan 15, 2021

natewiebe13 commented Jan 15, 2021 •

edited

Loading

stof commented Jan 15, 2021

nicolas-grekas commented Jan 15, 2021

nicolas-grekas commented Jan 15, 2021

stof commented Jan 15, 2021

ro0NL commented Jan 15, 2021

[Console] enable describing commands in ways that make the list command lazy #39851

[Console] enable describing commands in ways that make the list command lazy #39851

Conversation

nicolas-grekas commented Jan 15, 2021 • edited Loading

chalasr commented Jan 15, 2021

nicolas-grekas commented Jan 15, 2021

javiereguiluz commented Jan 15, 2021

nicolas-grekas commented Jan 15, 2021

noniagriconomie commented Jan 15, 2021

jderusse commented Jan 15, 2021

nicolas-grekas commented Jan 15, 2021

nicolas-grekas commented Jan 15, 2021

jderusse commented Jan 15, 2021

nicolas-grekas commented Jan 15, 2021

natewiebe13 commented Jan 15, 2021

stof commented Jan 15, 2021

stof commented Jan 15, 2021

natewiebe13 commented Jan 15, 2021 • edited Loading

stof commented Jan 15, 2021

nicolas-grekas commented Jan 15, 2021

nicolas-grekas commented Jan 15, 2021

stof commented Jan 15, 2021

ro0NL commented Jan 15, 2021

[Console] enable describing commands in ways that make the `list` command lazy #39851

[Console] enable describing commands in ways that make the `list` command lazy #39851

nicolas-grekas commented Jan 15, 2021 •

edited

Loading

natewiebe13 commented Jan 15, 2021 •

edited

Loading