Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] ChoiceQuestion must have choices#22847
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
javiereguiluz commentedMay 22, 2017
What is the behavior proposed here? If you pass zero choices to a choice question, shouldn't we throw an exception? |
ro0NL commentedMay 22, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 commentedMay 22, 2017
Updated. |
| publicfunction__construct($question,array$choices,$default =null) | ||
| { | ||
| if (!$choices) { | ||
| thrownew \LogicException('Choice question must have at least 1 choice available.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
maybeInvalidArgumentException would be a better fit here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@SpacePossum I totally agree with u.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not sure if we really care :) imo. the argument is valid (an array), but it's domain is invalid (empty array). SoDomainException actually (imo.) but i thoughtLogicException is a bit more common in SF :)
Im fine either way.. as long as it's extends from logic i dont mind :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 commentedMay 28, 2017
Thank you@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--><!---->Commits-------96e307f [Console] ChoiceQuestion must have choices
Uh oh!
There was an error while loading.Please reload this page.