Skip to content
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

[9.x] Improve input argument parsing for commands #44662

Merged
merged 2 commits into from
Nov 2, 2022

Conversation

bert-w
Copy link
Contributor

@bert-w bert-w commented Oct 19, 2022

I noticed that whenever I would listen to the CommandStarting/CommandFinished events, there was some functionality missing with the input property on this event (\Symfony\Component\Console\Input\InputInterface). It appears that the input definition is not loaded at all and it causes the getArguments()/getOptions() functions to be empty. So the following code would never work:

// Keep track of called commands and their arguments.
Event::listen(function (CommandStarting $event) {
    Log::info('starting ' . $event->command);
    Log::info($event->input->getOptions()); // Always `[]` even though the command has options.
    Log::info($event->input->getArguments()); // Always `[]` even though the command has arguments.
});

Therefore I've added a few lines of code to Illuminate\Console\Application to load the input definition directly from the command that is being called. The catch block around calling $input->bind() appears to be a common thing judging by the occurrences in the Symfony codebase, I'm not sure why this is necessary.

I've also just noticed by looking at this application structure, that Laravel is basically overriding some Symfony events (which DO work properly). Laravel calls its own CommandStarting/CommandFinished events while there already is existing code in Symfony that uses their own (for instance https://github.com/symfony/symfony/blob/6.2/src/Symfony/Component/Console/Application.php#L1006 and https://github.com/symfony/symfony/blob/6.2/src/Symfony/Component/Console/Application.php#L1034). However, the Symfony commands cannot be listened to because there apparently is no event dispatcher attached in the Laravel override so these events are never sent.

I'm not sure of the downsides of my addition so I've submitted this as a draft. Someone with more knowledge about the bootstrapping logic should have a look at this.

@driesvints
Copy link
Member

Hi there. We fixed the failing tests on 9.x which are causing this PR to fail as well. Can you please rebase your PR with 9.x? Thanks

@bert-w bert-w force-pushed the improve-command-input branch from f5933ae to ac087c0 Compare October 25, 2022 13:12
@driesvints
Copy link
Member

Please mark this as ready once you're done as draft PR's aren't reviewed.

@bert-w bert-w marked this pull request as ready for review October 27, 2022 12:53
@taylorotwell taylorotwell merged commit 0de8734 into laravel:9.x Nov 2, 2022
crynobone added a commit to crynobone/framework that referenced this pull request Nov 3, 2022
Calling `php artisan --version` now going to failed.

Signed-off-by: Mior Muhammad Zaki <[email protected]>
taylorotwell pushed a commit that referenced this pull request Nov 3, 2022
Calling `php artisan --version` now going to failed.

Signed-off-by: Mior Muhammad Zaki <[email protected]>

Signed-off-by: Mior Muhammad Zaki <[email protected]>
@bert-w bert-w deleted the improve-command-input branch November 4, 2022 19:01
@driesvints
Copy link
Member

I've decided to revert this PR. Please see #44888 for more info.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants