Skip to content

Conversation

@pforesi
Copy link
Contributor

@pforesi pforesi commented Nov 28, 2025

This pull request refactors the handling of OS signals for daemon commands by introducing a dedicated ConsoleCommandListener that responds to Symfony's ConsoleSignalEvent. The signal handling logic is moved out of DaemonCommand, and comprehensive unit and integration tests are added to ensure correct behavior. The documentation is also updated to describe the new testing strategy and best practices.

Event-driven signal handling and listener introduction:

  • Added a new ConsoleCommandListener class that listens for ConsoleSignalEvent and triggers shutdown on DaemonCommand instances when receiving SIGTERM or SIGINT, using Symfony's event system and PHP 8 attributes. (src/EventListener/ConsoleCommandListener.php)
  • Removed the direct handleSignal method and related signal handler registration from DaemonCommand, making signal handling fully event-driven. (src/Command/DaemonCommand.php) [1] [2]

Testing improvements:

  • Added comprehensive unit tests for the new listener, covering all signal and command-type combinations, attribute configuration, and edge cases. (tests/Unit/EventListener/ConsoleCommandListenerTest.php)
  • Added integration tests to verify the listener's behavior within Symfony's event system, including handling multiple listeners and signals. (tests/Integration/EventListener/ConsoleCommandListenerIntegrationTest.php)
  • Updated the test fixture DaemonCommandConcrete to include an isShutdownRequested() helper for easier assertions in tests. (tests/Fixtures/Command/DaemonCommandConcrete.php)
  • Removed obsolete signal handling tests from the old DaemonCommandTest. (tests/Unit/Command/DaemonCommandTest.php)

Documentation updates:

  • Added a detailed markdown document describing the new listener's tests, structure, best practices, and coverage statistics. (docs/CONSOLE_COMMAND_LISTENER_TESTS.md)

Copy link
Member

@Oliboy50 Oliboy50 left a comment

Choose a reason for hiding this comment

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

🤞 👏 🤷

- php-version: '8.1'
symfony-version: '^7.0'
php-version: ['8.2', '8.3', '8.4']
symfony-version: ['^6.4', '^7.0']
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand the purpose of this PR, because you talked about Symfony 7.3, but this bundle seemed to already be tested against Symfony ^7.0, so:

did our tests were not covering enough things and were not proving that Symfony 7.0 was not supported correctly? 😢
or did Symfony introduced a breaking change between 7.0 and 7.4? 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As explain in the opened issue, the bug appears from symfony 7.3:
When updating to sf >= 7.3 projects based on DaemonBundle fails since symfony Command implements the SignalableCommandInterface from its 7.3 version.
This interface define the function:
public function handleSignal(int $signal, int|false $previousExitCode = 0): int|false;

that conflict with the one defined into DaemonCommand

To resolve this issue I propose to handle signal by adding a src/EventListener/ConsoleCommandListener.php that hanlde the signal instead of to add signal handler directly in the DaemonCommand contructor:
// Add the signal handler pcntl_signal(SIGTERM, [$this, 'handleSignal']); pcntl_signal(SIGINT, [$this, 'handleSignal']);

Concerning the ci.yml I just updated the matrix by adding php 8.3, php 8.4 and removing 8.1 as long as It will be no more supported at the end of the month of december

@pforesi pforesi force-pushed the chore/compability_symfo74 branch from 928dddc to 3288fba Compare December 11, 2025 16:11
@pforesi
Copy link
Contributor Author

pforesi commented Dec 11, 2025

Just squashed my commits

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants