Skip to content

[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

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

keulinho
Copy link
Contributor

@keulinho keulinho commented Nov 3, 2023

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

@keulinho
Copy link
Contributor Author

keulinho commented Nov 3, 2023

Unit test failure seems unrelated to this change 🤔

@keulinho
Copy link
Contributor Author

keulinho commented Nov 3, 2023

Also the fail in the appveyor CI seems unrelated as it is some error in the redis integration test for the messenger component 🤔

@OskarStark
Copy link
Contributor

friendly ping @HeahDude as you contributed this feature, thanks

@symfony symfony deleted a comment from carsonbot Nov 3, 2023
@GromNaN
Copy link
Member

GromNaN commented Nov 3, 2023

I could not reproduce the issue #52433.

This listener is part of the FrameworkBundle, and so it should be used with Symfony\Bundle\FrameworkBundle\Console \Application. Could you provide more details on your application or test setup?

@keulinho
Copy link
Contributor Author

keulinho commented Nov 3, 2023

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 ConsoleCommandEvent does not work anymore.
E.g.

$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 profile option) in the listener basically couples the listener to the application class-

@chalasr
Copy link
Member

chalasr commented Nov 4, 2023

I've no strong opinion here. I think I can live with that coupling, especially given it's part of a bundle

Copy link
Contributor

@HeahDude HeahDude left a 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!

@chalasr
Copy link
Member

chalasr commented Nov 7, 2023

Thank you @keulinho.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Console][FrameworkBundle] --profile option might not be set on every command
6 participants