[Console] Better required argument check in InputArgument #46264
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
|
Works for me as a bugfix. Can you please add a test case that fails without your patch to prevent regressions? |
|
Done! Also I got slightly confused by the allowed mode combinations on both I'm willing to look into refactoring things and adding some additional validations, but this might be more of a problem with backwards compatibility, so maybe it is better to point such changes at |
|
Yes BC is especially important here, please do separate PRs/discussions for each bug-ish case. In the past, we have been merging bug fixes that turned into harmful BC breaks on this part of the code, which we had to revert. |
af9c97c to
89dd2bb
Compare
|
Thank you @jnoordsij. |
With the change symfony/symfony#46264 it's not longer possible to provide a default value for required arguments and the TYPO3 Console stops working with Symfony 6.0.9, 5.4.9 and 4.4.42.
With the change symfony/symfony#46264 it's not longer possible to provide a default value for required arguments and the TYPO3 Console stops working with Symfony 6.0.9, 5.4.9 and 4.4.42.
Use better check for required arguments in InputArgument when setting default value,
to correctly account for combining multiple mode options
In the current implementation, when combining multiple argument modes, e.g.
REQUIREDandIS_ARRAY, it is possible to also pass a default value, while this should raise an exception.This fix uses the
isRequiredmethod (which does a bitwise comparison) instead of a full equality match, to always reject a default value if the argument is required.Note: I am not sure if this is considered a bugfix or something that could be considered breaking backwards compatibility, as some configurations (e.g. combining the
REQUIREDandIS_ARRAYmodes and still passing a default value) that previously worked will break due to this.