Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] Add signal event#33729
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
lyrixx commentedSep 28, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks@marie for your first contribution. This will be not so simple:
|
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
marie commentedSep 29, 2019
Hello,@lyrixx. Thank you for your code review! |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
0118e9c to1797ac7Compare3b2c4a8 tof5ab3f6CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
dillingham commentedNov 11, 2019
Ps thanks@marie for putting time towards this. Much appreciated! 😄 |
marie commentedNov 12, 2019
@dillingham, I'm sorry I don't have time to finish the task now. I'll write in the related issue that it is unassigned. |
dillingham commentedNov 12, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I understand. Thanks for trying Seems like a waste to close this when the reviews only seem to be cosmetic If someone wants to take this over, I'll happily tip via paypal |
noniagriconomie commentedNov 12, 2019
yes it's too bad, could be a great addition for 4.4 |
nicolas-grekas commentedNov 12, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
If anyone is interested in pursuing the work, please check it out, squash all commits in one, add yours after, then submit a PR. |
lyrixx commentedNov 12, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Please, feel free to take it. if nobody wants to work on it, I may finish it (ping me in 1 month) @noniagriconomie It will be for SF 5.1 or more :) |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/FrameworkBundle/Tests/Command/RouterMatchCommandTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
063f49d to1574956Comparemarie commentedJun 29, 2020
Hello! The merge request is in status "needs work", but there are no discussions. What do I need to do to make it accepted? :) |
lyrixx commentedJun 30, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hello, Sorry for the delay. I did not test it, but If the code in the PR description works as expected, it seems good to me 👍 But (for another PR) :
|
chalasr left a comment
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.
We've got time to improve it. 👍
1574956 to859b692Comparefabpot commentedJul 31, 2020
Thank you@marie. |
fabpot commentedJul 31, 2020
@lyrixx it's now merged, do you have time to implement the 2 items you mentioned above? |
lyrixx commentedAug 4, 2020
@fabpot Yes :) I'll work on this feature on 13th august |
dillingham commentedAug 4, 2020
@marie I offered a bounty earlier, feel free to email me to collect. Thanks everyone for the effort on this! |
This PR was merged into the 5.2-dev branch.Discussion----------[Console] Rework the signal integration| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| Deprecations? | no| Tickets | refs#33729| License | MIT| Doc PR |I updated the code to have a better design and DX:* `SignalRegistry::$handlingSignals` is gone. This was a temporary data store to bind signal to symfony event. This does not belong to the `SignalRegistry`. It has been replaced by `Application::$signalsToDispatchEvent`. A method has been added to edit this list `Application::setSignalsToDispatchEvent()`* The default value for `Application::$signalsToDispatchEvent` is `SIGINT, SIGTERM, SIGUSR1, SIGUSR2`. Theses defaults seems good enough for most of application. for recall: * `SIGINT`: CTRL+C * `SIGTERM`: `kill PID`, this is what occurs when stopping a process with SystemD, upstart & co * `SIGUSR1` and `SIGUSR2`: Signal for user space* `Application::setSignalRegistry()` is gone. Now the Application always owns a signal registry. Since it's a CLI, it's legit to always have support for signals. A method has been added for convenience, and to register signal handler manually from the command: ```php $application->getSignalRegistry()->register(SIGINT, function ($signal) { dump("Signal ($signal) caught"); }); ```* The interface `SignalableCommandInterface` has been added. When a command implements this interface, the command is automatically called when a registered signal has been caughtA note about the BC:* If one register an handler before the Symfony ones, the Signal Registry keeps existing registered handlers. The BC is kept ✅* If one register an handler after the Symfony ones, it overrides the Symfony behavior. Since the feature is new. the BC is kept ✅---So now, If one want to listen a signal, they have few options depending on the context:* A global action is common to all commands (such as logging, enabling a profiler (👋 blackfire.io)): Use a listener. With autoconfigure, the following code is enough: ```php class SignalSubscriber implements EventSubscriberInterface { private $logger; public function __construct(LoggerInterface $logger = null) { $this->logger = $logger ?: new NullLogger(); } public function handleSignal(ConsoleSignalEvent $event) { $signal = $event->getHandlingSignal(); $this->logger->debug('The application has been signaled', [ 'signal' => $signal, ]); } public static function getSubscribedEvents() { return [ ConsoleEvents::SIGNAL => 'handleSignal', ]; } } ```* The command should react to a signal: Implements the interface: ```php class SignalCommand extends Command implements SignalableCommandInterface { protected static $defaultName = 'signal'; private $shouldStop = false; protected function execute(InputInterface $input, OutputInterface $output): int { $output->writeln('hello, I am '. getmypid()); for ($i=0; $i < 60; $i++) { if ($this->shouldStop) { break; } $output->write('.'); sleep(1); } $output->writeln(''); $output->writeln('bye'); return 0; } public function handleSignal(int $signal) { dump([__METHOD__, $signal]); $this->shouldStop = true; } public function getSubscribedSignals(): array { return [SIGINT]; } } ```* The command should react differently to many event and/or one wants a full control on name of the method called ```php class SignalCommand extends Command { protected static $defaultName = 'signal'; private $shouldStop = false; protected function execute(InputInterface $input, OutputInterface $output): int { $this->getApplication()->getSignalRegistry()->register(SIGINT, [$this, 'stop']); $this->getApplication()->getSignalRegistry()->register(SIGUSR1, [$this, 'enableBlackfire']); $output->writeln('hello, I am '. getmypid()); for ($i=0; $i < 60; $i++) { if ($this->shouldStop) { break; } $output->write('.'); sleep(1); } $output->writeln(''); $output->writeln('bye'); return 0; } public function stop() { $this->shouldStop = true; } public function enableBlackfire() { // ... } } ```ping@marieCommits-------df57119 [Console] Rework the signal integration
Uh oh!
There was an error while loading.Please reload this page.
This new feature allows to set a listener for performing some actions after the console command get a signal.
Usage: