-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Console][FrameworkBundle] Fix missing profile
option for console commands
#52434
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
Conversation
keulinho
commented
Nov 3, 2023
•
edited by OskarStark
Loading
edited by OskarStark
Q | A |
---|---|
Branch? | 6.4 |
Bug fix? | yes |
New feature? | no |
Deprecations? | no |
Issues | Fix #52433 |
License | MIT |
Unit test failure seems unrelated to this change 🤔 |
Also the fail in the appveyor CI seems unrelated as it is some error in the redis integration test for the messenger component 🤔 |
friendly ping @HeahDude as you contributed this feature, thanks |
I could not reproduce the issue #52433. This listener is part of the |
Ok took a look again and seemed i messed something up with the command tester, that seems to work as it does not dispatch the event. However manually triggering events by dispatching the $commandEvent = new ConsoleCommandEvent($command, $input, $output);
$kernel->getContainer()->get('event_dispatcher')->dispatch($commandEvent, ConsoleEvents::COMMAND); We used something like that in an internal command tester, that's why i first assumed the issue is in the symfony command tester as well. However, I still think the fix makes sense, as you should not rely in the listener that the event was correctly configured in the application class. IMHO the benefit of using events is to decouple different parts of the system from each other, but this assumption (that there is always an |
I've no strong opinion here. I think I can live with that coupling, especially given it's part of a bundle |
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.
Given #52433 (comment), I think this fix looks sensible. Thanks!
Thank you @keulinho. |