Skip to content

[Console] Improve AsCommand DX for simple arguments and options #57225

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

alamirault
Copy link
Contributor

@alamirault alamirault commented May 29, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #...
License MIT

I really like to use AsCommand attribute to configure command name and description.
For most of my commands I only have one or two arguments/options.

Instead implementing configure method, what do you think to allow argument/inputs declarations inside the attribute ?

(IMO, I can be more focus on execute method because command configuration it's done at one place)

Before this PR

#[AsCommand(
    name: 'app:foobar',
    description: 'Add a short description for your command',
)]
class FoobarCommandBefore extends Command
{
    protected function configure(): void
    {
        $this
            ->addArgument('folder', InputArgument::REQUIRED, 'Argument description')
            ->addOption('symlink', null, InputOption::VALUE_NONE, 'Option description')
        ;
    }

    protected function execute(InputInterface $input, OutputInterface $output): int
    {
        // ...
    }
}

After this PR

#[AsCommand(
    name: 'app:foobar',
    description: 'Add a short description for your command',
    inputDefinition: [
        new InputArgument('folder', InputArgument::REQUIRED, 'Argument description'),
        new InputOption('symlink', null, InputOption::VALUE_NONE, 'Option description'),
    ],
)]
class FoobarCommand extends Command
{
    protected function execute(InputInterface $input, OutputInterface $output): int
    {
        // ...
    }
}

(You can, of course, continue to use configure method )

@alamirault alamirault requested a review from chalasr as a code owner May 29, 2024 10:37
@carsonbot carsonbot added this to the 7.2 milestone May 29, 2024
@alamirault alamirault changed the title Improve AsCommand DX for simple arguments and options Improve AsCommand DX for simple arguments and options May 29, 2024
@xabbuh xabbuh added the Console label May 29, 2024
@carsonbot carsonbot changed the title Improve AsCommand DX for simple arguments and options [Console] Improve AsCommand DX for simple arguments and options May 29, 2024
@94noni
Copy link
Contributor

94noni commented May 29, 2024

kind of reminds me #49522, wdyt?
perhaps dedicate attributes would be better ?

@alamirault
Copy link
Contributor Author

kind of reminds me #49522, wdyt? perhaps dedicate attributes would be better ?

I didn't see issue. Interesting suggestion, that way modes looks like constraints or can be implicitly extracted from type hint.

However, it's required to rewrite how command system works, right ?

@94noni
Copy link
Contributor

94noni commented Jun 2, 2024

However, it's required to rewrite how command system works, right ?

i do not know really 🤷🏻‍♂️, I was just commenting an issue of mine were I've seen that the console component will leverage new langage feature, so quite similar to this

I think it is worth a discuss/question to know the go to of modernizing such "low level", and massively used component :)

@chalasr
Copy link
Member

chalasr commented Jun 3, 2024

Thanks for the PR, happy to see traction on this topic.

I didn't see issue. Interesting suggestion, that way modes looks like constraints or can be implicitly extracted from type hint.

Indeed, although still up to be discussed, input options/arguments value resolvers and autowiring are something that the work in progress mentioned in the linked issue covers.

However, it's required to rewrite how command system works, right ?

Correct yes, that WIP has a broader scope and is a lot more involving than what you propose in this PR as it also includes getting rid of the fact commands must extend the base Command class, which means changing the way commands are registered.
I wouldn't reject proposals in favor of something that didn't show up yet, especially given that merging this wouldn't prevent revisiting it as part of the WIP mentioned above - but I can confirm it does address input definition differently than what's proposed here.
FYI, @theofidry and I do have planned iterations in the coming weeks to move forward on this topic, so expect a competitive PR open asap :)

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this but I'm wondering about "suggestedValues" - should we find a way to allow binding it to a method of the command class?

*/
public function __construct(
public string $name,
public ?string $description = null,
array $aliases = [],
bool $hidden = false,
public array|InputDefinition $inputDefinition = [],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the $inputDefinition name a bit long but I don't have a better idea at the moment

@chalasr
Copy link
Member

chalasr commented Jan 3, 2025

I'm closing this in favor of #59340 as it goes one step further which is desired.
Thanks for the PR @alamirault.

@chalasr chalasr closed this Jan 3, 2025
chalasr added a commit that referenced this pull request Jan 10, 2025
… attributes (yceruto)

This PR was merged into the 7.3 branch.

Discussion
----------

[Console] Add support for invokable commands and input attributes

| Q             | A
| ------------- | ---
| Branch?       | 7.3
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Issues        | -
| License       | MIT

Alternative to: #57225

This PR focuses on enhancing the DX for creating and defining console commands.

Key improvements include:
 * No Need to Extend the Command Class: When using an invokable class marked with `#[AsCommand]`, extending the `Command` class is no longer required.
 * Automatic Argument and Option Inference: Command arguments and options are now inferred directly from the parameters of the `__invoke()` method, thanks to the new `#[Argument]` and `#[Option]` attributes.
 * Flexible `__invoke()` Signature: The `__invoke()` method now has a flexible signature, allowing you to define only the helpers you need.

### Before
```php
#[AsCommand(name: 'lucky:number')]
class LuckyNumberCommand extends Command
{
    public function __construct(private LuckyNumberGenerator $generator)
    {
        parent::__construct();
    }

    protected function configure(): void
    {
        $this->addArgument('name', InputArgument::REQUIRED);
        $this->addOption('formal', null, InputOption::VALUE_NONE | InputOption::VALUE_NEGATABLE);
    }

    public function execute(InputInterface $input, OutputInterface $output): int
    {
        $io = new SymfonyStyle($input, $output);
        $formal = (bool) $input->getOption('formal');
        $name = $input->getArgument('name');

        $io->title(sprintf('%s %s!', $formal ? 'Hello' : 'Hey', ucfirst($name)));
        $io->success(sprintf('Today\'s Lucky Number: %d', $this->generator->random()));

        return 0;
    }
}
```
### After
```php
#[AsCommand(name: 'lucky:number')]
class LuckyNumberCommand
{
    public function __construct(private LuckyNumberGenerator $generator)
    {
    }

    public function __invoke(SymfonyStyle $io, #[Argument] string $name, #[Option] bool $formal): void
    {
        $io->title(sprintf('%s %s!', $formal ? 'Hello' : 'Hey', ucfirst($name)));
        $io->success(sprintf('Today\'s Lucky Number: %d', $this->generator->random()));
    }
}
```
However, you can still extend the `Command` class when necessary to use advanced methods, such as the `interact()` method and others.

Happy New Year! 🎉

Commits
-------

cf6c3aa Add support for invokable commands and input attributes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants