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

[Console] ChoiceQuestion must have choices #22847

Closed
wants to merge 2 commits into
base: 2.7
from

Conversation

Projects
None yet
8 participants
@ro0NL
Contributor

ro0NL commented May 22, 2017

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #22842
License MIT
Doc PR symfony/symfony-docs#...

@ro0NL ro0NL changed the title from [Console] Derive choice from default to [Console] Bypass choice question without choices May 22, 2017

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone May 22, 2017

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz May 22, 2017

Member

What is the behavior proposed here? If you pass zero choices to a choice question, shouldn't we throw an exception?

Member

javiereguiluz commented May 22, 2017

What is the behavior proposed here? If you pass zero choices to a choice question, shouldn't we throw an exception?

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 22, 2017

Contributor

Perhaps easier yes. Went for safe, as i wasnt really sure about the default value behavior (without any further options); or a default value being an unknown option.

The first (actually the case from the issue) also crashes, the latter is already validated when the question is optional.

So 👍 for throwing in the constructor, unless im unaware of any side effects :) but im fine either way.

Contributor

ro0NL commented May 22, 2017

Perhaps easier yes. Went for safe, as i wasnt really sure about the default value behavior (without any further options); or a default value being an unknown option.

The first (actually the case from the issue) also crashes, the latter is already validated when the question is optional.

So 👍 for throwing in the constructor, unless im unaware of any side effects :) but im fine either way.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 22, 2017

Contributor

Updated.

Contributor

ro0NL commented May 22, 2017

Updated.

@ro0NL ro0NL changed the title from [Console] Bypass choice question without choices to [Console] ChoiceQuestion must have choices May 22, 2017

@@ -32,6 +32,10 @@ class ChoiceQuestion extends Question
*/
public function __construct($question, array $choices, $default = null)
{
if (!$choices) {
throw new \LogicException('Choice question must have at least 1 choice available.');

This comment has been minimized.

@SpacePossum

SpacePossum May 28, 2017

Contributor

maybe InvalidArgumentException would be a better fit here?

@SpacePossum

SpacePossum May 28, 2017

Contributor

maybe InvalidArgumentException would be a better fit here?

This comment has been minimized.

@nunomaduro

nunomaduro May 29, 2017

@SpacePossum I totally agree with u.

@nunomaduro

nunomaduro May 29, 2017

@SpacePossum I totally agree with u.

This comment has been minimized.

@ro0NL

ro0NL May 30, 2017

Contributor

not sure if we really care :) imo. the argument is valid (an array), but it's domain is invalid (empty array). So DomainException actually (imo.) but i thought LogicException is a bit more common in SF :)

Im fine either way.. as long as it's extends from logic i dont mind :)

@ro0NL

ro0NL May 30, 2017

Contributor

not sure if we really care :) imo. the argument is valid (an array), but it's domain is invalid (empty array). So DomainException actually (imo.) but i thought LogicException is a bit more common in SF :)

Im fine either way.. as long as it's extends from logic i dont mind :)

This comment has been minimized.

@SpacePossum

SpacePossum May 30, 2017

Contributor

changing it now would be BC break ;) anyway we should be good. If it is a domain level exception I would expect it to be thrown on count < 2 as one option does not leave anything to choose from either ;)

@SpacePossum

SpacePossum May 30, 2017

Contributor

changing it now would be BC break ;) anyway we should be good. If it is a domain level exception I would expect it to be thrown on count < 2 as one option does not leave anything to choose from either ;)

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot May 28, 2017

Member

Thank you @ro0NL.

Member

fabpot commented May 28, 2017

Thank you @ro0NL.

fabpot added a commit that referenced this pull request May 28, 2017

bug #22847 [Console] ChoiceQuestion must have choices (ro0NL)
This PR was squashed before being merged into the 2.7 branch (closes #22847).

Discussion
----------

[Console] ChoiceQuestion must have choices

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22842
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->

<!--
![image](https://cloud.githubusercontent.com/assets/1047696/26301309/1bfa52ca-3ee1-11e7-883b-f627f16e9d2f.png)
-->

Commits
-------

96e307f [Console] ChoiceQuestion must have choices

@fabpot fabpot closed this May 28, 2017

@ro0NL ro0NL deleted the ro0NL:console-choice branch May 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment