Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.8k
[Console] don't discard existing aliases when constructing Command#62562
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
Conversation
carsonbot commentedNov 29, 2025
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution 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 |
| } | ||
| // we must not overwrite existing aliases, combine new ones with existing ones | ||
| $aliases =array_unique([ |
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.
I think I would rather guard thesetAliases() call below to not call it with an empty array.
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.
What's the intended behavior most likely?
Adding aliases from $name being split by| seems to be the latest change. Merging those into the array of existing ones feels most BC-safe over overwriting it if aliases here are not an empty array? 🤔
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.
I think I would rather guard the
setAliases()call below to not call it with an empty array.
if ($aliases !== []) {$this->setAliases($aliases); }
Like this? That alone wouldn't fix the issue of existing aliases, because if $name was 'name|newalias', it would still overwrite the existing ones.
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.
I thought about that too and then I wondered if it is really realistic that someone updates the name to make use of the new opportunity to define name and aliases at once and then additionally also callssetAlias(). 🤷
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.
Though if we say that's something we want to keep we can indeed remove theif again.
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.
Perhaps not realistic, but entirely possible.
08fe044 to2ae73e6Comparechalasr commentedDec 2, 2025
Thank you@henderkes. |
93ea5f6 intosymfony:7.4Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Contributor guidelines:
https://symfony.com/releases#maintained-symfony-branches
and must add an entry to the changelog file of the patched component:
https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
https://symfony.com/bc