fix(logger): accept any Monolog handler as process manager log writer - #238
Open
jremmurd wants to merge 1 commit into
Open
fix(logger): accept any Monolog handler as process manager log writer#238jremmurd wants to merge 1 commit into
jremmurd wants to merge 1 commit into
Conversation
The return value of AbstractLogger::createStreamHandler() is passed straight to ApplicationLogger::addWriter(), which accepts every HandlerInterface implementation. The narrower union StreamHandler|ApplicationLoggerDb|null therefore rules out valid handlers for no reason, in particular every handler that does not write to a stream: the AsyncAws CloudWatch handler, Sentry, Slack, Redis. That the union already contains ApplicationLoggerDb, which is not a StreamHandler, shows the intent was "a log writer", not "a stream". Projects hitting this cannot return their handler at all, because the return type cannot be widened in a child class. Widening it here to ?HandlerInterface is backwards compatible: all existing implementations (File, Console, Application, EmailSummary) keep their narrower return types, which covariance allows.
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.
Problem
AbstractLogger::createStreamHandler()is typedStreamHandler | ApplicationLoggerDb | null, but its only consumer isMonitoringItem::getLogger():ApplicationLogger::addWriter(object $writer)accepts everyHandlerInterfaceimplementation. The declared union is therefore narrower than what the bundle actually supports, and it rules out every Monolog handler that does not happen to extendStreamHandler: the AsyncAws CloudWatch handler, Sentry, Slack, Redis, and so on.That
ApplicationLoggerDbis already part of the union, and is not aStreamHandler, shows the intent was "a log writer", not "a stream".A project cannot work around this. The return type of an overriding method may only be narrowed, never widened, so returning a non-stream handler is impossible. In our case the custom CloudWatch logger had to return
nullto satisfy the signature, which silently disabled process manager logging to CloudWatch:getLogger()drops anullwriter without a warning.Change
Widen the abstract return type to
?HandlerInterface, drop the two imports that become unused.Backwards compatible. All four shipped implementations keep their current, narrower return types, which covariance allows:
File—?StreamHandlerConsole—?StreamHandlerEmailSummary—?StreamHandlerApplication—ApplicationLoggerDbProject implementations that already return one of those types are unaffected.
The method name stays
createStreamHandler()to avoid a breaking rename.Branches
The same commit is proposed for
pimcore12in a separate PR, the file is identical on both branches.