Problem / motivation

Loading /admin/config/system/audit-trail/segments on a fresh container crashes with:

Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException:
Circular reference detected for service "audit_trail.logger",
path: "audit_trail.chain_archiver -> audit_trail.logger -> logger.factory".

Root cause is a service-wiring design smell: AuditTrailLogger wears two hats. It implements LoggerInterface (tagged logger, so the container's RegisterLoggersPass calls addLogger(@audit_trail.logger) on logger.factory), AND it implements AuditTrailInternalWriterInterface (so ChainArchiver, ChainTimestamper et al. inject it as their chain-event emitter). On top of that, it injects @logger.factory for fallback warning emission. The three together form a cycle: logger.factory cannot finish initialization without audit_trail.logger, which cannot finish construction without logger.factory.

Kernel tests do not catch this because the test harness warms logger.factory before any audit_trail service is requested. Production hits the cycle when a controller (the segments admin page) is the first cold thing to resolve chain_archiver.

Proposed resolution

A logger should be just a logger. Split into two classes with clearly separated responsibilities:

  • AuditTrailLogger (refactored): thin LoggerInterface entry point. Tagged logger. Inspects the PSR-3 context, forwards chain: TRUE calls to the internal writer, no-ops the rest. NO logger.factory dependency. Catastrophic failures fall back to error_log() because PSR-3 forbids throwing from log() and we cannot re-enter logger.factory without re-creating the cycle.
  • AuditTrailInternalWriter (new class): all the existing row-build, canonicalize, HMAC, INSERT, chain-event-mint logic moves here. Implements AuditTrailInternalWriterInterface. NOT tagged logger. Two public entry points: logChained(level, message, context) for the PSR-3 path (called by the logger above) and chainedWrite(chain, action, resource, buckets) for module internals (called by ChainArchiver, ChainTimestamper, etc.). On failure: throws a typed exception. Callers handle.

After the split, AuditTrailInternalWriter takes the heavy dependency list (database, secret repository, lock, current_user, datetime.time, config.factory, request_stack, entity_type_manager, state) without @logger.factory. AuditTrailLogger takes only AuditTrailInternalWriter. The service graph becomes a DAG. Cold-resolve of any audit_trail service no longer trips a cycle.

Replacing the five existing loggerFactory warning calls

  • Lock-contention drop (AuditTrailLogger:188): audit_trail.dropped_under_contention state counter already tracks this; the supplemental warning is redundant. Drop it.
  • Chain event mint / secret repository failures (lines 363, 397, 560, 581): the new AuditTrailInternalWriter throws typed exceptions on these failure modes. ChainArchiver already wraps its writes in try/catch and reports through its own paths. The thin AuditTrailLogger wraps the writer call in try/catch and falls back to error_log() for the PSR-3 path that cannot throw.

Regression test

A new kernel test that builds a fresh container per service and resolves each write-bearing audit_trail service cold:

public function testColdResolveCriticalServices(): void {
  foreach ([
    'audit_trail.logger',
    'audit_trail.internal_writer',
    'audit_trail.chain_archiver',
    // ...every service a controller injects
  ] as $id) {
    $container = $this->rebootKernel();
    $this->assertNotNull($container->get($id), $id);
  }
}

This test would have failed against the pre-split code. It will pass after the refactor and fail again if anyone re-introduces a cycle.

Remaining tasks

  • Create src/Writer/AuditTrailInternalWriter.php with the bulk of today's AuditTrailLogger.
  • Slim AuditTrailLogger down to the entry-point shape.
  • Update audit_trail.services.yml wiring + alias @Drupal\audit_trail\AuditTrailInternalWriterInterface to the new service.
  • Convert the five loggerFactory->warning(...) callsites to typed exceptions / state counters / error_log() per the table above.
  • Add the cold-resolve regression test.
  • Run the full suite + verify no regression.
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mably created an issue. See original summary.

  • mably committed d97e7d27 on 1.x
    fix: #3591791 Split AuditTrailLogger into a thin LoggerInterface entry...
mably’s picture

Status: Active » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.