[Console] Fix passing options with defaultCommand #22244

jakubsacha · Apr 2, 2017

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

Seems like overwriting input for the default command is not needed (anymore?). I don't know where the removed comment comes from originally.

Use case: i want to call default command and use options at the same time:
app/console --abc=true

nicolas-grekas · Apr 3, 2017

ping @lyrixx?

lyrixx · Apr 3, 2017

I don't know. But without test, It can not be merged.

chalasr · Apr 3, 2017

It never worked (tried 2.7.0 out).

Also what about arguments? Here, the first passed argument will just be considered as the command name. Just wondering if we should expect the command argument value to be automatically set instead? It would echo the behavior of single command applications since 3.2, which sounds good to me (as a feature on master). It can't work.

jakubsacha · Apr 3, 2017

I'll create a unit test for that use case later today. Could someone give me a hand with that "interact called" thing in the unit tests, that is failing?

@chalasr it can only work with options. The first argument is used as a command name, so it's never a case.

app/console --abc=true this invocation should call default command and pass abc as an option. This is the only use case i can think about at the moment.

jakubsacha · Apr 3, 2017

src/Symfony/Component/Console/Application.php

@@ -179,7 +179,9 @@ public function doRun(InputInterface $input, OutputInterface $output)

        if (!$name) {
            $name = $this->defaultCommand;
-            $input = new ArrayInput(array('command' => $this->defaultCommand));
+            $inputArgument = $this->definition->getArgument('command');
+            $inputArgument->setMode(InputArgument::OPTIONAL);


this is kind of ugly fix, that allows me to call setDefault() - it disallows setting a default value for the commands with REQUIRED value.

We could get rid of the setter by redefining the argument entirely e.g

- $inputArgument = $this->definition->getArgument('command'); - $inputArgument->setMode(InputArgument::OPTIONAL); - $inputArgument->setDefault($name); + $this->definition->setArguments(array_merge( + $this->definition->getArguments(), + array('command' => new InputArgument('command', InputArgument::OPTIONAL, this->definition->getArgument('command')->getDescription(), $name)) + ));

(not tested)

jakubsacha · Apr 3, 2017

ok seems much better to me now. I added test as well. I still have no clue why travis fails with:

2) Symfony\Component\Console\Tests\ApplicationTest::testSetRunCustomDefaultCommandWithOption
Application runs the default set command if different from 'list' command
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'interact called
-called
+'called
 opt
 '

Can anyone help me with that?

chalasr

👍 with minor comment

chalasr · Apr 3, 2017

src/Symfony/Component/Console/Application.php

+                        InputArgument::OPTIONAL,
+                        $this->definition->getArgument('command')->getDescription(),
+                        $name),
+                )


I would put the whole array on 1 line.

as requested.

jakubsacha · Apr 5, 2017

@chalasr is there anything else for me to do here? especially with that failing unit test.

chalasr · Apr 5, 2017

@jakubsacha I given a quick look at the build failure 2 days ago trying to reproduce, unfortunately I don't know what happen either, the test runs fine locally. I would suggest to rebase this PR against the current master since there is a lot of activity on it these days. I will give a deeper look into the failing test. Apart from that, this looks good to me.

fabpot · Apr 5, 2017

src/Symfony/Component/Console/Tests/Fixtures/FooOptCommand.php

+                'fo',
+                InputOption::VALUE_OPTIONAL,
+                'fooopt description'
+            )


This should be on one line as well

fabpot · Apr 5, 2017

So, we would allow options to work but not arguments, right?

chalasr · Apr 5, 2017

Arguments work already, but the first needs to be the command name (we could not guess it from the default command as we can't predict which value targets which argument in the raw input token).

Actually you can't do:

php console.php --foo=bar // default command executed,  getOption('foo') is NULL

You have to specify the command name to have options taken into account. This PR proposes to allow omitting it so that getOption('foo') is equal to bar using the previous example

jakubsacha · Apr 5, 2017

@fabpot correct. In case you have arguments the first one becomes command name, and you don't have that issue. The problem is only for defaultCommand.

chalasr · Apr 6, 2017

@jakubsacha this statement is reached in the build, thus interact() is not called. Dunno why it happens in your PR and not in the current 2.7 build.

jakubsacha · Apr 7, 2017

Ok I think I understand what was the problem:

Current Symfony 2.7 behaviour:

ArrayInput is created in the ApplicationTester
It's passed through Application::run to Application::configureIO, where interactive shell is disabled
ArrayInput is recreated in Application::doRun, and by default interactive shell is true. and its done after configureIO is called.

Unit tests were failing because I didn't create new Input but used one passed initially. Since my input is processed by configureIO and the shell is not interactive in the CI systems, it failed.

I forced non-interactive execution of the tests, but I'm not sure is it the best approach.

jakubsacha · Apr 10, 2017

ping @chalasr

jakubsacha · Apr 19, 2017

ping @chalasr @fabpot. could we do anything with that pull request? i think it's ready to merge.

chalasr

👍 from my side

jakubsacha · Apr 28, 2017

@fabpot kind ping again;-) any chance we get it through?

jakubsacha · Jul 20, 2017

@fabpot @Simperfit @chalasr @lyrixx it's been some time. It's still relevant to me. Can we merge it?

nicolas-grekas · Jul 28, 2017

ping @ogizanagi also :)

chalasr · Jul 28, 2017

Just checked this out again. The changes in ApplicationTest might look suspicious, but this fix just reveals that the fact we are currently creating a new ArrayInput instance makes interactive to true in the middle of a command execution even if not a tty.
This fixes a real bug: input options and arguments are lost when the default command is used.
Still 👍 for me.

ogizanagi

LGTM 👍

fabpot · Jul 29, 2017

Thank you @jakubsacha.

…acha) This PR was merged into the 2.7 branch. Discussion ---------- [Console] Fix passing options with defaultCommand | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Seems like overwriting input for the default command is not needed (anymore?). I don't know where the removed comment comes from originally. Use case: i want to call default command and use options at the same time: app/console --abc=true Commits ------- 761de99 Fix passing options with defaultCommand

* Yaml::parseFile() wasn't present untill 3.4 > use parse() with file_get_contents() * Process didn't accept array untill 3.3 > Update to 3.3+ as 4.0+ needs array * Console had a bug issue in with empty defaults prior to 3.3.^ & 3.2.13 See: symfony/symfony#22244 > Update minimum to 3.13 and add a conflict rune for between 3.3.0 and 3.3.5 Be sure to test against lowest versions on travis.

carsonbot added Status: Needs Review Bug labels Apr 2, 2017

jakubsacha changed the title ~~Fix passing options with defaultCommand~~ [Console] Fix passing options with defaultCommand Apr 2, 2017

jakubsacha force-pushed the default-command-options-arguments branch 5 times, most recently from 6a6565d to 00b1d79 Compare April 3, 2017 05:31

nicolas-grekas added this to the 2.7 milestone Apr 3, 2017

jakubsacha commented Apr 3, 2017

View reviewed changes

jakubsacha force-pushed the default-command-options-arguments branch from 6db5e8d to 065f3d5 Compare April 3, 2017 15:51

chalasr approved these changes Apr 3, 2017

View reviewed changes

jakubsacha force-pushed the default-command-options-arguments branch from 065f3d5 to 259de3c Compare April 3, 2017 16:43

jakubsacha mentioned this pull request Apr 4, 2017

Add run filtering trivago/rumi#73

Open

fabpot reviewed Apr 5, 2017

View reviewed changes

jakubsacha force-pushed the default-command-options-arguments branch from 9975d8e to 94ca088 Compare April 6, 2017 06:48

jakubsacha force-pushed the default-command-options-arguments branch from c861433 to 5977146 Compare April 7, 2017 21:47

Fix passing options with defaultCommand

761de99

jakubsacha force-pushed the default-command-options-arguments branch from 5977146 to 761de99 Compare April 7, 2017 21:55

chalasr approved these changes Apr 19, 2017

View reviewed changes

Simperfit approved these changes Apr 28, 2017

View reviewed changes

ogizanagi approved these changes Jul 28, 2017

View reviewed changes

fabpot merged commit 761de99 into symfony:2.7 Jul 29, 2017

This was referenced Aug 1, 2017

Release v2.7.33 #23734

Merged

Release v2.8.26 #23735

Merged

Release v3.2.13 #23736

Merged

Release v3.3.6 #23738

Merged

BackEndTea mentioned this pull request Aug 11, 2018

Fix symfony dependency issues infection/infection#446

Merged

asko-ohmann mentioned this pull request Sep 20, 2018

[Console] Error calling command from default command #27838

Closed

tobymackenzie mentioned this pull request Mar 9, 2022

Console: Running default command causes invalid hasAnArrayArgument state #25632

Closed

Search code, repositories, users, issues, pull requests...

Uh oh!

[Console] Fix passing options with defaultCommand #22244

[Console] Fix passing options with defaultCommand #22244

Uh oh!

Conversation

jakubsacha commented Apr 2, 2017

Uh oh!

nicolas-grekas commented Apr 3, 2017

Uh oh!

lyrixx commented Apr 3, 2017

Uh oh!

chalasr commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakubsacha commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakubsacha Apr 3, 2017

Choose a reason for hiding this comment

Uh oh!

chalasr Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakubsacha commented Apr 3, 2017

Uh oh!

chalasr left a comment

Choose a reason for hiding this comment

Uh oh!

chalasr Apr 3, 2017

Choose a reason for hiding this comment

Uh oh!

jakubsacha Apr 3, 2017

Choose a reason for hiding this comment

Uh oh!

jakubsacha commented Apr 5, 2017

Uh oh!

chalasr commented Apr 5, 2017

Uh oh!

fabpot Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

jakubsacha Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

fabpot commented Apr 5, 2017

Uh oh!

chalasr commented Apr 5, 2017

Uh oh!

jakubsacha commented Apr 5, 2017

Uh oh!

chalasr commented Apr 6, 2017

Uh oh!

jakubsacha commented Apr 7, 2017

Uh oh!

jakubsacha commented Apr 10, 2017

Uh oh!

jakubsacha commented Apr 19, 2017

Uh oh!

chalasr left a comment

Choose a reason for hiding this comment

Uh oh!

jakubsacha commented Apr 28, 2017

Uh oh!

jakubsacha commented Jul 20, 2017

Uh oh!

nicolas-grekas commented Jul 28, 2017

Uh oh!

chalasr commented Jul 28, 2017

Uh oh!

ogizanagi left a comment

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jul 29, 2017

Uh oh!

Uh oh!

chalasr commented Apr 3, 2017 •

edited

Loading

jakubsacha commented Apr 3, 2017 •

edited

Loading

chalasr Apr 3, 2017 •

edited

Loading