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
[Console] Add AlarmableCommandInterface
and console.alarm
event
#53533
base: 7.2
Are you sure you want to change the base?
Conversation
ecb03f9
to
baaf070
Compare
*/ | ||
public function getSubscribedSignals(): array; | ||
public function getSubscribedSignals(/* InputInterface $input, OutputInterface $output */): array; |
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.
you cannot add mandatory arguments in a method signature through such BC layer, because the child class is not allowed to add them as mandatory arguments.
Btw, changing the subscribed signals based on the output looks weird to me. What is the use case for it ? Is this worth a BC break ?
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.
The more I thought about it, the less sense it made to me as well. I initially had a use case for #53508, but I've since found a better approach, and now I can't really think of a good use case. Therefore, I've removed the entire commit. If an actual use case comes up in the future, it can be added then. I've also removed the $output
argument from getAlarmTime()
as it was also useless.
} | ||
|
||
$commandSignals = $command instanceof SignalableCommandInterface ? $command->getSubscribedSignals($input, $output) : []; | ||
if ($command instanceof AlarmableCommandInterface && \defined('SIGALRM') && SignalRegistry::isSupported() && !\in_array(\SIGALRM, $commandSignals, true)) { |
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.
Making the TraceableCommand implement AlarmableCommandInterface means that in dev, all commands will register this SIGALRM
(even when they are not alarmable). Is this expected ?
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.
No, good catch, I've totally missed that one. I've added a TraceableAlarmableCommand
, so this should no longer be an issue. I'm not sure if this is the best approach, but I couldn't think of any other.
// If the command is signalable, we call the handleSignal() method | ||
if (\in_array($signal, $commandSignals, true)) { | ||
elseif (\in_array($signal, $commandSignals, true)) { |
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.
this will never be called in dev when a SignalableCommand asks for SIGALRM
because of the TraceableCommand wrapping it and implementing AlarmableCommandInterface. This means this PR is currently a BC break.
$this->signalRegistry->register($signal, function (int $signal) use ($command): void { | ||
if (false !== $exitCode = $command->handleSignal($signal)) { | ||
$this->signalRegistry->register($signal, function (int $signal) use ($command, $input, $output): void { | ||
if (\SIGALRM === $signal && $command instanceof AlarmableCommandInterface) { |
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.
Same issue here regarding TraceableCommand.
use Symfony\Component\Console\Output\OutputInterface; | ||
|
||
/** | ||
* Interface for command that dispatches SIGALRM signal. |
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.
Is the command actually dispatching SIGALRM signals ? The rest of the code gives me the impression that it listens to those signals.
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.
Fixed
a11d0da
to
93f7c17
Compare
93f7c17
to
e42a38a
Compare
src/Symfony/Component/Console/Command/AlarmableCommandInterface.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Console/Command/AlarmableCommandInterface.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Console/Tests/phpt/alarm/command_exit.phpt
Outdated
Show resolved
Hide resolved
3496c6f
to
26d9c59
Compare
26d9c59
to
4ff3662
Compare
Btw, So if a command logic f.e. includes Another (old) thing I found that using |
from the original issue, it was taken into account:
i think this is a reasonable workaround (but needs documentation), even if it's not supported on windows (that's a PHP API issue/bug), and should not block this PR for Linux, we could even fix/abstract it in |
Part of #53508
This PR introduces a new
AlarmableCommandInterface
and aconsole.alarm
event. A command implementing this interface will automatically subscribe to theSIGALRM
signal and dispatch an alarm at the specified interval:When a command implements both the
AlarmableCommandInterface
and theSignalableCommandInterface
, thehandleSignal()
method will not be called for aSIGALRM
signal.The
console.alarm
event is dispatched on everySIGALRM
signal: