[Console] Fixes multiselect choice question defaults in non-interactive mode #27772

veewee · 2018-06-29T11:26:03Z

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

This commit introduced a warning in multiselect mode:

PHP Notice:  Undefined index:  in /tmp/vendor/symfony/console/Helper/QuestionHelper.php on line 52
PHP Stack trace:
PHP   1. {main}() /tmp/vendor/phpro/grumphp/bin/grumphp:0
PHP   2. GrumPHP\Console\Application->run() /tmp/vendor/phpro/grumphp/bin/grumphp:31
PHP   3. GrumPHP\Console\Application->run() /tmp/vendor/phpro/grumphp/src/Console/Application.php:240
PHP   4. GrumPHP\Console\Application->doRun() /tmp/vendor/symfony/console/Application.php:145
PHP   5. GrumPHP\Console\Application->doRunCommand() /tmp/vendor/symfony/console/Application.php:262
PHP   6. GrumPHP\Console\Command\ConfigureCommand->run() /tmp/vendor/symfony/console/Application.php:904
PHP   7. GrumPHP\Console\Command\ConfigureCommand->execute() /tmp/vendor/symfony/console/Command/Command.php:251
PHP   8. GrumPHP\Console\Command\ConfigureCommand->buildConfiguration() /tmp/vendor/phpro/grumphp/src/Console/Command/ConfigureCommand.php:95
PHP   9. Symfony\Component\Console\Helper\QuestionHelper->ask() /tmp/vendor/phpro/grumphp/src/Console/Command/ConfigureCommand.php:156

Notice: Undefined index:  in /tmp/vendor/symfony/console/Helper/QuestionHelper.php on line 52

This PR fixes this issue by parsing the default value using the built-in validator if available. (which most likely is ...)

chalasr

Bug fixes must be submitted against the lowest branch where they apply

For 2.8? :)

chalasr · 2018-06-29T20:40:44Z

src/Symfony/Component/Console/Helper/QuestionHelper.php


-                return $choices[$question->getDefault()];
+                return $question->getValidator() ? $question->getValidator()($default) : $default;


The validator wasn't applied here previously, this looks like a behavior change to me so cannot be part of the patch. Should be submitted as a new feature on master separately.

veewee · 2018-07-02T05:10:07Z

Hi @chalasr,

Thanks for the feedback!
I am not sure what you are expecting from me at this point. Do I need to create a patch for 2.8, another one for 4.1 (since the code has changed there) and a new feature request using the validator for master?

The way I see it:
2.8 until 4.1.1 worked "fine": it used the default value without any lookups in the choices.
From 4.1.1 on, the code selects an index from the choices list which breaks in non-interactive multiselect mode.

The only thing I did in this PR, is made the defaults parameter work in the same way it does in interactive mode. It is not really a behaviour change, but an inconsistency between operation modes which could be considered a bug.

Can you provide me some additional feedback? Thanks!

chalasr · 2018-07-03T09:57:07Z

@veewee The updated code looks the same in 2.8 and 4.1 to me (the PR that introduced the bug is part of 2.8). Am I missing something that makes 2.8 free of this bug? Otherwise, this PR needs to be rebased on 2.8 being the lowest branch where the bug applies.

About making use of the validator in non-interactive mode, it potentially changes the return value of the method. Could be considered as a bugfix but anyways I think it should be discussed in a dedicated PR as it is not related to the bug fixed here.

chalasr · 2018-09-08T12:53:36Z

ping @veewee :) should we close/take over?

veewee · 2018-09-28T07:11:48Z

Hi @chalasr,

Sorry for the delay.

To me, it is not very clear what the type of the 'default' parameter should be. For single options it makes sense to provide a key or name, but for multiple options it makes more sense to just provide the array of defaults instead of a comma separated string of indexes / names. Maybe the validator lookup is just overkill in this case... But that's up to you guys to decide.

Besides that, I also do not know which versions this affects and how it should be fixed in the different affected versions.

Since I have a lot of open questions about the subject, feel free to take this one over.

With kind regards.

fabpot · 2018-10-10T10:40:10Z

@chalasr Can you take over this one?

…default values

chalasr · 2018-10-12T15:59:36Z

@fabpot PR reworked, ready on my side.

chalasr · 2018-10-12T16:01:31Z

src/Symfony/Component/Console/Helper/QuestionHelper.php

+                foreach ($default as $k => $v) {
+                    $v = trim($v);
+                    $default[$k] = isset($choices[$v]) ? $choices[$v] : $v;
+                }


Borrowed from https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/Helper/SymfonyQuestionHelper.php#L47 so that it's consistent everywhere

fabpot · 2018-10-12T16:06:47Z

Thank you @veewee.

…n-interactive mode (veewee) This PR was merged into the 2.8 branch. Discussion ---------- [Console] Fixes multiselect choice question defaults in non-interactive mode | Q | A | ------------- | --- | Branch? | >4.1.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | [This commit](41ffc69) introduced a warning in multiselect mode: ``` PHP Notice: Undefined index: in /tmp/vendor/symfony/console/Helper/QuestionHelper.php on line 52 PHP Stack trace: PHP 1. {main}() /tmp/vendor/phpro/grumphp/bin/grumphp:0 PHP 2. GrumPHP\Console\Application->run() /tmp/vendor/phpro/grumphp/bin/grumphp:31 PHP 3. GrumPHP\Console\Application->run() /tmp/vendor/phpro/grumphp/src/Console/Application.php:240 PHP 4. GrumPHP\Console\Application->doRun() /tmp/vendor/symfony/console/Application.php:145 PHP 5. GrumPHP\Console\Application->doRunCommand() /tmp/vendor/symfony/console/Application.php:262 PHP 6. GrumPHP\Console\Command\ConfigureCommand->run() /tmp/vendor/symfony/console/Application.php:904 PHP 7. GrumPHP\Console\Command\ConfigureCommand->execute() /tmp/vendor/symfony/console/Command/Command.php:251 PHP 8. GrumPHP\Console\Command\ConfigureCommand->buildConfiguration() /tmp/vendor/phpro/grumphp/src/Console/Command/ConfigureCommand.php:95 PHP 9. Symfony\Component\Console\Helper\QuestionHelper->ask() /tmp/vendor/phpro/grumphp/src/Console/Command/ConfigureCommand.php:156 Notice: Undefined index: in /tmp/vendor/symfony/console/Helper/QuestionHelper.php on line 52 ``` This PR fixes this issue by parsing the default value using the built-in validator if available. (which most likely is ...) Commits ------- 099e265 [Console] Fixes multiselect choice question in interactive mode with default values

carsonbot added Status: Needs Review Console Bug labels Jun 29, 2018

veewee mentioned this pull request Jun 29, 2018

Warnings at installation phpro/grumphp#516

Closed

nicolas-grekas added this to the 4.1 milestone Jun 29, 2018

carsonbot removed the Status: Needs Review label Jun 29, 2018

Landerstraeten approved these changes Jun 29, 2018

View reviewed changes

carsonbot added Status: Reviewed Status: Needs Work and removed Status: Reviewed labels Jun 29, 2018

chalasr requested changes Jun 29, 2018

View reviewed changes

chalasr modified the milestones: 4.1, 2.8 Jun 29, 2018

chalasr changed the base branch from 4.1 to 2.8 October 11, 2018 22:55

[Console] Fixes multiselect choice question in interactive mode with …

099e265

…default values

chalasr added Status: Needs Review and removed Status: Needs Work labels Oct 12, 2018

chalasr changed the title ~~[Console] Fixes multiselect choice question defaults in interactive mode~~ [Console] Fixes multiselect choice question defaults in non-interactive mode Oct 12, 2018

chalasr reviewed Oct 12, 2018

View reviewed changes

fabpot approved these changes Oct 12, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 12, 2018

fabpot merged commit 099e265 into symfony:2.8 Oct 12, 2018

This was referenced Nov 3, 2018

Release v2.8.47 #29069

Merged

Release v3.4.18 #29070

Merged

Release v4.1.7 #29071

Merged

This was referenced Mar 12, 2021

Bump symfony/symfony from 2.8.27 to 2.8.52 lajoskvcs/symfony-final#2

Open

Bump symfony/symfony from 2.8.3 to 2.8.52 lajoskvcs/symfony-course#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] Fixes multiselect choice question defaults in non-interactive mode #27772

[Console] Fixes multiselect choice question defaults in non-interactive mode #27772

veewee commented Jun 29, 2018

chalasr left a comment

chalasr Jun 29, 2018

veewee commented Jul 2, 2018

chalasr commented Jul 3, 2018 •

edited

Loading

chalasr commented Sep 8, 2018

veewee commented Sep 28, 2018

fabpot commented Oct 10, 2018

chalasr commented Oct 12, 2018

chalasr Oct 12, 2018

fabpot commented Oct 12, 2018


		return $choices[$question->getDefault()];
		return $question->getValidator() ? $question->getValidator()($default) : $default;

[Console] Fixes multiselect choice question defaults in non-interactive mode #27772

[Console] Fixes multiselect choice question defaults in non-interactive mode #27772

Conversation

veewee commented Jun 29, 2018

chalasr left a comment

Choose a reason for hiding this comment

chalasr Jun 29, 2018

Choose a reason for hiding this comment

veewee commented Jul 2, 2018

chalasr commented Jul 3, 2018 • edited Loading

chalasr commented Sep 8, 2018

veewee commented Sep 28, 2018

fabpot commented Oct 10, 2018

chalasr commented Oct 12, 2018

chalasr Oct 12, 2018

Choose a reason for hiding this comment

fabpot commented Oct 12, 2018

chalasr commented Jul 3, 2018 •

edited

Loading