Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

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

 Sign up
Appearance settings

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

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

Merged

Conversation

@GwendolenLynch
Copy link
Contributor

QA
Branch?5.4
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#45332
LicenseMIT

@derrabus
Copy link
Member

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

GwendolenLynch reacted with thumbs up emoji

@GwendolenLynch
Copy link
ContributorAuthor

GwendolenLynch commentedFeb 9, 2022•
edited
Loading

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, useSIGUSRx instead ofSIGALRM 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.

derrabus reacted with thumbs up emoji

@GwendolenLynchGwendolenLynchforce-pushed thecommand-signals branch 2 times, most recently from44c9186 to0b7cd1dCompareFebruary 9, 2022 20:46
Copy link
Member

@lyrixxlyrixx 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 👍🏼

@nicolas-grekas
Copy link
Member

Thank you@GwendolenLynch.

GwendolenLynch reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas merged commitdb7c211 intosymfony:5.4Aug 2, 2022
@GwendolenLynchGwendolenLynch deleted the command-signals branchAugust 2, 2022 10:19
}
if ($this->signalsToDispatchEvent) {
$commandSignals =$commandinstanceof 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, buthasListeners 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

@GwendolenLynch@nicolas-grekas I added a comment regarding usinghasListeners. 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?

fabpot added a commit that referenced this pull requestAug 9, 2022
…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 referencedAug 26, 2022
}
}

foreach ($commandSignalsas$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
ContributorAuthor

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.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot left review comments

@GromNaNGromNaNGromNaN left review comments

@derrabusderrabusderrabus left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@lyrixxlyrixxlyrixx approved these changes

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

+1 more reviewer

@helhumhelhumhelhum left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

8 participants

@GwendolenLynch@derrabus@nicolas-grekas@helhum@fabpot@GromNaN@lyrixx@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp