fix: the CLI must attach the event bus - #865
Merged
Merged
Conversation
`ModuleBase::initEventHandlers()` is what attaches the four receivers that make the event bus do anything — the log handler, the database handler that writes the Eventlog table the security log reads, the mail alerts and the in-app notifications. Web/Init and Api/Init each call it from their own initialize(). Cli/Init never did. EventDispatcher is a container singleton and notify() is a plain loop over the attached receivers, so with none attached it is a silent no-op for the whole process. Nothing throws, nothing logs that an event was dropped. A master password rotation or a backup run from bin/cli.php therefore left no trace of having happened, while the identical operation through the web or the API was fully recorded — including run.backup.start, the rotation's per-stage events, and the exception events fired when one of them fails. An operator reading the security log to find out what had been done to the instance was reading a log with the CLI's work missing from it. That it is an oversight rather than a decision is visible in the constructor: Cli\Init already takes ProvidersHelper and hands it to ModuleBase, and nothing in the CLI ever used it — initEventHandlers() is its only consumer. It goes before initCli(), which is what runs the command. initEventHandlers() returns early after attaching the log handler when the instance is not installed, so sp:install is unaffected: it still gets file logging and nothing that would need a database. The test builds each module's real container and asserts the dispatcher has the log handler afterwards. For the CLI it calls the real initialize() (with an inert console application, so no command is dispatched); for web and api it invokes the shared initEventHandlers() directly, because reaching it through their initialize() means a real PHP session for one and a fully installed config with a live database for the other. Checked by removing the line: only the CLI test fails, and it fails because the handler is absent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ModuleBase::initEventHandlers()is what attaches the four receivers that make the eventbus do anything — the log handler, the database handler that writes the Eventlog table
the security log reads, the mail alerts and the in-app notifications. Web/Init and
Api/Init each call it from their own initialize(). Cli/Init never did.
EventDispatcher is a container singleton and notify() is a plain loop over the attached
receivers, so with none attached it is a silent no-op for the whole process. Nothing
throws, nothing logs that an event was dropped. A master password rotation or a backup
run from bin/cli.php therefore left no trace of having happened, while the identical
operation through the web or the API was fully recorded — including run.backup.start,
the rotation's per-stage events, and the exception events fired when one of them fails.
An operator reading the security log to find out what had been done to the instance was
reading a log with the CLI's work missing from it.
That it is an oversight rather than a decision is visible in the constructor: Cli\Init
already takes ProvidersHelper and hands it to ModuleBase, and nothing in the CLI ever
used it — initEventHandlers() is its only consumer.
It goes before initCli(), which is what runs the command. initEventHandlers() returns
early after attaching the log handler when the instance is not installed, so sp:install
is unaffected: it still gets file logging and nothing that would need a database.
The test builds each module's real container and asserts the dispatcher has the log
handler afterwards. For the CLI it calls the real initialize() (with an inert console
application, so no command is dispatched); for web and api it invokes the shared
initEventHandlers() directly, because reaching it through their initialize() means a real
PHP session for one and a fully installed config with a live database for the other.
Checked by removing the line: only the CLI test fails, and it fails because the handler
is absent.