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.

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

[Console] Fix ConsoleEvents::SIGNAL subscriber dispatch #45333

Merged
merged 1 commit into from Aug 2, 2022
Merged

[Console] Fix ConsoleEvents::SIGNAL subscriber dispatch #45333

merged 1 commit into from Aug 2, 2022

Conversation

GwendolenLynch
Copy link
Contributor

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #45332
License MIT

@carsonbot carsonbot added this to the 5.4 milestone Feb 7, 2022
@derrabus
Copy link
Member

derrabus commented Feb 9, 2022

Thank you for your PR! Can you please add a test hat covers your change?

@GwendolenLynch
Copy link
Contributor Author

GwendolenLynch commented Feb 9, 2022 •
edited

Added @derrabus. The act of writing the tests showed up some missing bits, so thank you for the nudge.

Changed

  • Move extracted code back into method and change some of the selection logic.
  • For tests, use SIGUSRx instead of SIGALRM as the later will cause an exit when there are no more listeners, and we only need to test that the signal was processed by the listener.

Please let me know what changes you'd like made.

@GwendolenLynch GwendolenLynch force-pushed the command-signals branch 2 times, most recently from 44c9186 to 0b7cd1d Compare February 9, 2022 20:46
lyrixx
lyrixx approved these changes Aug 1, 2022
View reviewed changes
Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good 👍🏼

src/Symfony/Component/Console/Application.php Show resolved Hide resolved
@nicolas-grekas
Copy link
Member

Thank you @GwendolenLynch.

@nicolas-grekas nicolas-grekas merged commit db7c211 into symfony:5.4 Aug 2, 2022
@GwendolenLynch GwendolenLynch deleted the command-signals branch August 2, 2022 10:19
helhum
helhum reviewed Aug 5, 2022
View reviewed changes
src/Symfony/Component/Console/Application.php
}
if ($this->signalsToDispatchEvent) {
$commandSignals = $command instanceof SignalableCommandInterface ? $command->getSubscribedSignals() : [];
$dispatchSignals = $this->dispatcher && $this->dispatcher->hasListeners(ConsoleEvents::SIGNAL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$this->dispatcher is typed to be a \Symfony\Contracts\EventDispatcher\EventDispatcherInterface, but hasListeners is not part of that contract.

This leads to our package breaking with the current branch version (5.4.x-dev in our case), since it does not fulfill the \Symfony\Component\EventDispatcher\EventDispatcherInterface

@helhum
Copy link
Contributor

helhum commented Aug 5, 2022

@GwendolenLynch @nicolas-grekas I added a comment regarding using hasListeners. Not sure if this was an intentional change, but wanted to let you know, that it breaks our package, that implements an event dispatcher that fulfills the PSR and SF contract interfaces.

@nicolas-grekas
Copy link
Member

This should be fixed. Up for a PR?

@xabbuh xabbuh mentioned this pull request Aug 8, 2022
Merged
fabpot added a commit that referenced this pull request Aug 9, 2022
@fabpot
bug #47218 [Console] fix dispatch signal event check for compatibilit…
458b294 
…y with the contract interface (xabbuh)

This PR was merged into the 5.4 branch.

Discussion
----------

[Console] fix dispatch signal event check for compatibility with the contract interface

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #45333 (comment)
| License       | MIT
| Doc PR        |

Commits
-------

6b29591 fix dispatch signal event check for compatibility with the contract interface
This was referenced Aug 26, 2022
Merged
Merged
Merged
@fabpot fabpot mentioned this pull request Sep 30, 2022
Merged
@GromNaN GromNaN mentioned this pull request Nov 12, 2022
Closed
GromNaN
GromNaN reviewed Nov 13, 2022
View reviewed changes
src/Symfony/Component/Console/Application.php
}
}

foreach ($commandSignals as $signal) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any specific reason why the order of registration of signal handlers have been modified? This caused an issue #48205 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember my approach/reason at the time, but the test added in this PR should cover the use-case problem I had.

Your fix #48210 is most probably correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks you for reviewing.

@benmeynell benmeynell mentioned this pull request Jun 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot left review comments

@GromNaN GromNaN GromNaN left review comments

@helhum helhum helhum left review comments

@derrabus derrabus derrabus left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@lyrixx lyrixx lyrixx approved these changes

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

Assignees
No one assigned
Labels
Bug Console Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@GwendolenLynch @derrabus @nicolas-grekas @helhum @fabpot @GromNaN @lyrixx @carsonbot