Skip to content

fix(DatabaseManager): make setLogger() reach every driver - #273

Merged
roxblnfk merged 1 commit into
2.xfrom
test/orm-60-set-logger
Oct 5, 2026
Merged

roxblnfk merged 1 commit into
2.xfrom
test/orm-60-set-logger

Conversation

@roxblnfk

@roxblnfk roxblnfk commented Oct 4, 2026

Copy link
Copy Markdown
Member

🔍 What was changed

  • DatabaseManager::setLogger() now reaches every driver it knows about: drivers created before or after the call, drivers registered via addDriver(), and the drivers of databases registered via addDatabase().
  • Behaviour change: a logger set explicitly via setLogger() now takes precedence over the LoggerFactory, for existing and future drivers. Before this, every driver created after the call got the factory's logger and the explicit one was ignored. This affected every Spiral application, because spiral/cycle-bridge always passes a LoggerFactory. The factory is still used while no logger is set.
  • An explicitly set NullLogger is now passed to drivers as is. A NullLogger returned by the factory is still skipped, so drivers without a logger don't build the log context for every query.

Why?

The scenarios where the logger was lost are covered by DatabaseManagerLoggerTest, 21 cases. Before this fix, 7 of them failed on 2.x.

Review notes

  • Database::withoutCache() clones its drivers, and the manager does not track those clones. A clone made before setLogger() keeps the logger it had when it was cloned. Tracking clones would need a shared logger holder between a driver and its clones, which is out of scope here. The limitation is documented on setLogger() and pinned by a test.
  • testDatabaseManagerWithLoggerAndWithLoggerFactoryShouldReturnLoggerFromFactory asserted the old priority (the factory wins over an explicit logger). It is replaced by the opposite assertion, plus a test that the factory is still used when no logger is set.

Checklist

@roxblnfk
roxblnfk requested review from a team as code owners October 4, 2026 17:24
@github-actions github-actions Bot added the type: test Test label Oct 4, 2026
@codecov

codecov Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.66%. Comparing base (75fb8f2) to head (a00cf3e).

Additional details and impacted files
@@             Coverage Diff              @@
##                2.x     #273      +/-   ##
============================================
+ Coverage     95.61%   95.66%   +0.05%     
- Complexity     2230     2234       +4     
============================================
  Files           142      142              
  Lines          6341     6351      +10     
============================================
+ Hits           6063     6076      +13     
+ Misses          278      275       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

test(DatabaseManager): cover setLogger() with factory, added drivers and databases, clones, reconnect and transactions

A logger set explicitly now takes precedence over the LoggerFactory for existing and future drivers, and is passed to drivers registered via addDriver() and addDatabase(). Drivers cloned by Database::withoutCache() are not tracked by the manager and keep the logger they had when cloned.

Refs cycle/orm#60

Assisted-By: Claude Opus 5.5 <noreply@anthropic.com>
@roxblnfk
roxblnfk force-pushed the test/orm-60-set-logger branch from 23a64ec to a00cf3e Compare October 4, 2026 17:32
@roxblnfk
roxblnfk merged commit 73a8306 into 2.x Oct 5, 2026
30 of 31 checks passed
@roxblnfk
roxblnfk deleted the test/orm-60-set-logger branch October 5, 2026 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant