NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

[Console] Fix issue with reserved keyword "command" as argument name#54795

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

Jump to bottom

Open

flkasper wants to merge5 commits intosymfony:7.4

base:7.4

Choose a base branch

fromflkasper:feature/fix-issue-54729

Open

[Console] Fix issue with reserved keyword "command" as argument name#54795

flkasper wants to merge5 commits intosymfony:7.4fromflkasper:feature/fix-issue-54729

Conversation

Copy link

flkasper commentedMay 1, 2024•
edited by OskarStark
Loading

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	Fix#54729
License	MIT

In theInputArgument constructor, anInvalidArgumentException is thrown if the reserved keywordcommand is used as the name and is not the argumentcommand name.

The change to this PR is a new non-breaking feature, but could also be merged into older versions as a bugfix.

flkasper added2 commits

May 1, 2024 13:03

[Console] Fix issue 54729 of reserved keyword 'command' as argument name

6948564

[Console] Add changelog entry

719fa86

flkasper requested a review fromchalasr as acode owner

May 1, 2024 11:16

carsonbot added Status: Needs Review Bug Console Feature labels

May 1, 2024

carsonbot added this to the7.1 milestone

May 1, 2024

Fix tests, remove void return type in test

d67fa0c

Copy link

Member

chalasr commentedMay 1, 2024

Thanks for the PR. Although it is hack-ish, I'm ok with the argument description' string-based check.
To prevent regressions we need a test case that makes us able to catch a potential change to thecommand argument description and forces us to sync the description used inInputArgument.

Changing the milestone for 7.2 also as 7.1 is in feature-freeze and this is a DX improvement, not a bugfix.

chalasr modified the milestones:7.1,7.2

May 1, 2024

chalasr added DXDX = Developer eXperience (anything that improves the experience of using Symfony) and removed Bug labels

May 1, 2024

alexandre-daubois reviewed

May 2, 2024

View reviewed changes

src/Symfony/Component/Console/Input/InputArgument.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Console/Command/Command.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Console/Input/InputArgument.php OutdatedShow resolvedHide resolved

Copy link

Member

fabpot commentedMay 2, 2024

An alternative could be to rename the internalcommand name to__command to avoid the potential clash.

OskarStark changed the title~~[Console] Fix issue with reserved keyword 'command' as argument name~~[Console] Fix issue with reserved keyword "command" as argument name

May 2, 2024

flkasper added2 commits

May 4, 2024 19:15

Applied suggested wording changes

d23ba08

Replace string description of command argument by constant.

a162b40

Copy link

Author

flkasper commentedMay 4, 2024•
edited by chalasr
Loading

An alternative could be to rename the internalcommand name to__command to avoid the potential clash.

See discussion at#54729.
A collision would not be 100% avoidable with a renaming.

It would also be a major breaking change.
Many libraries that are based on Symfony would also have to be adapted; in Symfony's own code alone, this is used in many places to execute other commands.