Skip to content
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] Fix issue with reserved keyword "command" as argument name #54795

Open
wants to merge 5 commits into
base: 7.2
Choose a base branch
from

Conversation

flkasper
Copy link

@flkasper flkasper commented May 1, 2024

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

In the InputArgument constructor, an InvalidArgumentException is thrown if the reserved keyword command is used as the name and is not the argument command name.

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

@chalasr
Copy link
Member

chalasr commented May 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 the command argument description and forces us to sync the description used in InputArgument.

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

@chalasr chalasr modified the milestones: 7.1, 7.2 May 1, 2024
@chalasr chalasr added DX DX = Developer eXperience (anything that improves the experience of using Symfony) and removed Bug labels May 1, 2024
@fabpot
Copy link
Member

fabpot commented May 2, 2024

An alternative could be to rename the internal command name to __command to avoid the potential clash.

@OskarStark 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
Copy link
Author

flkasper commented May 4, 2024

An alternative could be to rename the internal command 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Console DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature Status: Needs Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command: Adding argument called 'command' makes other args/options unreadable
5 participants