-
Notifications
You must be signed in to change notification settings - Fork 2k
feat: signals #9690
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
feat: signals #9690
Conversation
Great work! I'm not sure if |
I believe we're good. Besides, this is just for development purposes anyway. |
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.
Please add ext-pcntl
and ext-posix
to the suggest
in composer.json.
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.
Thanks. Looks good overall!
I'm wondering if we should offer translations for some messages, such as: "PCNTL extension not available. Signal handling disabled." Or is it acceptable to leave them untranslatable, since they are general error messages? |
I think yes to be consistent with the rest of the framework. |
Is it possible to ping a "translation" repository? To add new phrases |
Not sure about the procedure here... @paulbalandan, are we waiting for the |
Unfortunately, translations repo only picks up the latest strings from develop branch. These won't be picked up until 4.7 is released and merged to develop. |
Okay, thanks for confirming. I don't think it's a problem if translations only become available after |
1309186
to
89c0192
Compare
89c0192
to
8e17a72
Compare
If someone has an idea why Psalm started failing, please let me know. I cannot reproduce this locally. |
I have error. Arch Linux $ utils/vendor/bin/psalm
Running on PHP 8.4.12, Psalm 6.13.1@1e3b7f0a8ab32b23197b91107adc0a7ed8a05b51.
JIT acceleration: ON
JIT compilation in progress... Done.
Process plugin adjustments...
Target PHP version: 8.1 (inferred from composer.json).
Scanning files...
[Error]
Class "Tests\Support\Commands\SignalCommand" not found
at ROOTPATH/tests/_support/Commands/SignalCommandNoPcntl.php:19 |
It looks like a bug. Check out the autoload for psalm: CodeIgniter4/psalm-autoload.php Lines 21 to 28 in bc62d40
But sorting the files does not allow us to find the class because it has not loaded yet.
It follows that there may be some errors in other code. @michalsn try: // TODO: Temporary solution.
$files = iterator_to_array($iterator, false);
unset($iterator);
usort($files, function(SplFileInfo $a, SplFileInfo $b) {
return strcmp($a->getPathname(), $b->getPathname());
});
/** @var SplFileInfo $file */
foreach ($files as $file) { |
Thanks, @neznaika0. I've already pushed a fix. Not sure if this is the ideal long-term solution, but it works for now. Maybe @paulbalandan has a better approach? |
Yes, it's temporary. In fact, you need an autoloader as in the system: calling the class >> reading the class file. |
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.
I do not know either why files are sorted differently across OS but the fix looks good at the moment.
The problem is not sorting. Files: Even if the order is correct, the class will load later |
Thank you @paulbalandan and @neznaika0 |
Description
This PR adds
SignalTrait
that provides unified handling of operating system signals (such as SIGINT and SIGTERM, etc.) in CLI commands. This ensures that long-running or sensitive CLI processes can gracefully handle interruptions before exiting.In addition, this PR updates the way migration commands are handled to prevent interruptions during critical changes in the database, reducing the risk of leaving the schema in an inconsistent state.
Further details, usage examples, and guidelines are available in the updated user guide.
Checklist: