Skip to content

Navigation Menu

Sign in

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: 7.3
Choose a base branch
Loading
from
Open

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

wants to merge 5 commits into from
+26 −7

Conversation

flkasper
Copy link

@flkasper flkasper commented May 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 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.

@flkasper flkasper requested a review from chalasr as a code owner May 1, 2024 11:16
@carsonbot carsonbot added this to the 7.1 milestone May 1, 2024
@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 •
edited by chalasr
Loading

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.

@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexandre-daubois alexandre-daubois alexandre-daubois left review comments

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

Assignees
No one assigned
Labels
Console DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature Status: Needs Review
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

Command: Adding argument called 'command' makes other args/options unreadable
5 participants
@flkasper @chalasr @fabpot @alexandre-daubois @carsonbot
Morty Proxy This is a proxified and sanitized view of the page, visit original site.