Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Console] Additional validation of mode inInputArgument andInputOption#52055
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
base:7.4
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Also automatically set OPTIONAL whenever REQUIRED is not specified.
Tests failing due to issues fixed in#52058; I'll finish this after that is merged. |
…OptionAlso automatically set NONE whenever REQUIRED is not specified.
…tArgument and InputOption
b1e241c to251b242CompareThere 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.
Thank you for this suggestion, that will avoid mistakes. I don't see any legitimate case of using these contradictory options together.
My comments are proposals, to be discussed.
| { | ||
| $this->expectDeprecation('Since symfony/console 6.4: InputOption mode should be either none, required or optional.'); | ||
| new InputOption('foo', 'f', InputOption::VALUE_NONE | InputOption::VALUE_REQUIRED); |
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.
Have you considered using a data provider to reduce repetition?
| throw new InvalidArgumentException(sprintf('Option mode "%s" is not valid.', $mode)); | ||
| } | ||
| $requirementSpecifier = ($mode & (self::VALUE_NONE | self::VALUE_REQUIRED | self::VALUE_OPTIONAL)); | ||
| if (!(self::VALUE_NONE === $requirementSpecifier || self::VALUE_REQUIRED === $requirementSpecifier || self::VALUE_OPTIONAL === $requirementSpecifier)) { | ||
| trigger_deprecation('symfony/console', '6.4', 'InputOption mode should be either none, required or optional.'); |
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.
We can add the option name to the deprecation message. That would help to find where the change need to be done when our tools don't provide the backtrace of the trigger.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Jérôme Tamarelle <jerome@tamarelle.net>
InputArgument andInputOptionWhat is the status of this PR? If you still want to work on it, please rebase on top of |
Awaiting the results of#52058, which addresses the test failing here. Once a decision is reached there, I'll be rebasing and adjusting this further. |
Uh oh!
There was an error while loading.Please reload this page.
Currently it is allowed to to combine various combinations of input value requirement specifiers, which are logically exclusive behavior-wise. To prevent mistakes or possibly ambiguous behavior, this PR adds additional validation to ensure there's always exactly one specifier part of the mode after finishing the constructor.
This is done by adding a deprecation message for these cases, which can be replaced with an
InvalidArgumentExceptionin a future major version. The tests added are currently checking for the deprecation message and could be updated to check for this exception instead.