feat(logging): [3–4/4] PHPStan logging gate + DB connection-error logging (re-target of #477/#478)#487
Merged
Conversation
Add three project-local PHPStan rules (ADR 011 / docs/logging.md) and resolve every site they flag in the same change — zero baseline entries: - NoSilentCatchRule (logging.silentCatch): flags a catch that neither rethrows, logs via PSR-3, calls error_log/trigger_error, nor reports the failure to the console (SymfonyStyle/OutputInterface). The console-output exemption is in the rule, not a src/Command/ path exclusion, so a genuinely empty catch in a command (unattended cron/Messenger) is still flagged. - NoInterpolatedLogMessageRule (logging.interpolatedLogMessage): log message must be a static string; variable data goes in the context array. - ExceptionInContextMustUseExceptionKeyRule (logging.exceptionContextKey): a Throwable in the context array must use the "exception" key. Silent-catch migration: inject channel-bound loggers and log the swallowed exception (AuthOidc/AuthScreenBind, ColiboFeedType, InstantBook, FeedService, Feed/Theme/UserProvider), convert interpolated feed-type messages to static message + exception context, and annotate the genuinely-intentional silences (optional OIDC end-session URL, per-booking time parse, read-degradation in FeedSourceProvider, the log-enrichment guard, validator violation, fixtures). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…hannel Add the `database` Monolog channel and a DBAL driver middleware that logs connection failures so operators without database access get a signal (ADR 011). Logging-only — no reconnect/retry. - ConnectionErrorDriver wraps connect() and reads the raw driver error code (a middleware sits below DBAL's exception conversion, which does not classify 1040/1203/2003 as ConnectionException). Connection-pressure / unreachability codes log at critical, other connect failures at error. The original exception is rethrown unchanged; only the host is logged, never the password or DSN. - Register the middleware with the explicit doctrine.middleware tag (the bundle collects tagged services; implementing the interface is not enough) and bind the database channel. - Add a prod per-channel handler for `database` (LOG_LEVEL_DATABASE with the LOG_LEVEL fallback) and document the event=db.connection_error shape. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nv' into feature/logging-phpstan-gate # Conflicts: # CHANGELOG.md
…to feature/db-connection-error-logging # Conflicts: # CHANGELOG.md
tests/PhpStan/data/silent-catch.php deliberately leaves the catch variable unused in the swallow-the-exception scenarios that exercise the logging.silentCatch rule. Rector's RemoveUnusedVariableInCatchRector rewrote those to non-capturing catches, failing the Rector CI gate. PHPStan rule fixtures are hand-crafted inputs and must stay verbatim, so skip the whole tests/PhpStan/data directory. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…to feature/db-connection-error-logging
The new RuleTestCase-based PHPStan rule tests boot full PHPStan, whose phar bundles phpdoc-parser 2.x and calls PhpDocNode::getSealedTagValues(). The composer-installed phpdoc-parser was pinned to ^1.24 (1.33.0), which lacks that method; under CI's optimized/authoritative autoloader the 1.x PhpDocNode shadowed the phar's bundled 2.x copy, erroring the rule tests. Bump to ^2.0 (resolves to 2.3.2) so both copies expose the method. Compatible with phpdocumentor/reflection-docblock (^1.7|^2.0); our own code does not use the parser directly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…to feature/db-connection-error-logging
…nv' into feature/logging-phpstan-gate
…to feature/db-connection-error-logging
…nv' into feature/logging-phpstan-gate
…to feature/db-connection-error-logging
…nv' into feature/logging-phpstan-gate
Rename the injected $logger to its channel ($authLogger, $screenLogger, $feedLogger, $interactiveLogger) in the classes wired to a non-default Monolog channel, so each constructor self-documents where it logs. Explicit @monolog.logger.<channel> bindings stay (keys updated to match) and remain the authoritative wiring. Covers AuthOidcController, AuthScreenBindController, Colibo/Notified/Rss FeedType, FeedService, FeedProvider and InstantBook. EventDatabaseApiV2FeedType and CalendarApiFeedType are intentionally left as $logger: they have no explicit channel binding (they autowire the default channel), so renaming them would only create a silent name-convention dependency. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…to feature/db-connection-error-logging
… channel Rename ConnectionErrorMiddleware's injected logger $logger -> $databaseLogger so the constructor self-documents its channel; binding key updated to match. ConnectionErrorDriver keeps its positional $logger (not a channel-bound service). No behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… db.response.status_code The database connection-error record now carries an OTel-compliant categorisation: error.type holds a stable low-cardinality token (too_many_connections, access_denied, …) mapped from the driver code, falling back to the stringified code when unmapped. The verbose driver text remains in exception.message. Also rename the bespoke db.error_code to the OTel database semantic-convention attribute db.response.status_code (emitted as a string). Doc table and ConnectionErrorDriver test updated accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nv' into feature/logging-phpstan-gate
…to feature/db-connection-error-logging
The database channel has its own always-on handler; add it to the main handler's channel blacklist so a critical connection error isn't emitted twice (see the per-channel duplicate-emission fix). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ion host There is no db.host OTel attribute; connection host/port are the general server.address / server.port attributes. Rename db.host -> server.address (host only, never password/DSN) and emit server.port conditionally (omit-when-absent, per OTel). Doc table and driver test updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nv' into feature/logging-phpstan-gate
…to feature/db-connection-error-logging
…nv' into feature/logging-phpstan-gate
…to feature/db-connection-error-logging
A NonUniqueResultException on a unique-constrained by-id lookup is a can't-happen data-integrity violation, so error is the correct severity. It also matters for ThemeProvider/UserProvider, which log on the default `app` channel: a lone warning there is buffered and dropped by the fingers_crossed gate, whereas an error trips the gate and is emitted. FeedProvider's identical case bumped too for consistency. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…to feature/db-connection-error-logging
These catches log on always-on channels but their key identifier lives in the request body or local scope, not the URL — so ambient url.path can't recover it. Add explicit context: - InstantBook compute-options catch: resource + slide_id - InstantBook duration-interval catch: resource (slide not in scope) - ColiboFeedType malformed-gallery catch: feed_id + feed_source_id AuthOidcController's auth-failure catch is intentionally left ambient-only — no domain resource id exists (no user resolved yet); url.path + the exception message are the right detail there. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…to feature/db-connection-error-logging
Our code never uses PHPStan\PhpDocParser directly; the direct ^2.0 constraint exists only so PHPStan's RuleTestCase resolves the same parser major as the bundled phpstan.phar (a dev-tooling concern). It stays installed in prod as a transitive of phpdocumentor/reflection-docblock, so production is unchanged — only the redundant direct prod declaration moves to require-dev. Addresses review feedback on #477. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…to feature/db-connection-error-logging
feat(logging): [3/4] PHPStan logging gate + silent-catch migration
…ogging feat(logging): [4/4] DB connection-error logging middleware (database channel)
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
1 participant
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.
Why this PR exists
#477 (PHPStan logging gate + silent-catch migration) and #478 (DB
connection-error middleware) were merged into
feature/structured-logging-semconvinstead of
release/3.0.0: when #479 merged, GitHub auto-retargeted the twostacked PRs onto the (merged-but-undeleted) semconv branch, so merging them
landed their content there rather than on
release/3.0.0.release/3.0.0therefore has only #475 + #479. This PR brings the remaining,already-reviewed #477 + #478 work onto
release/3.0.0. The diff is exactly thatdelta (43 files) — no new work.
What's included
logging.silentCatch,logging.interpolatedLogMessage,logging.exceptionContextKey) + migration ofthe silent catch sites.
ConnectionErrorMiddleware/Driveron thedatabasechannel.(fingers_crossed channel exclusions),
user.id(wasenduser.id),server.address/server.port(wasdb.host), exceptiontype(wasclass),error.type+db.response.status_code.phpstan/phpdoc-parser→require-dev; non-unique-by-idquery results logged at
error; domain ids added to instant-book / Colibocatches.
Content was reviewed and approved as #477 and #478; this is purely a
merge-target correction.
🤖 Generated with Claude Code