Skip to content

afterSaveCommit / afterDeleteCommit permanently suppressed inside Connection::transactional() #19382

@jamisonbryant

Description

@jamisonbryant

Summary

When Table::save() or Table::delete() is called inside an application-level Connection::transactional() block, afterSaveCommit and afterDeleteCommit events are permanently suppressed. The events are correctly held during the transaction, but are never deferred and re-dispatched when the outermost transaction commits.

$connection->transactional(function () {
    $this->OrdersTable->saveOrFail($order);          // afterSaveCommit suppressed
    $this->PaymentsTable->saveOrFail($payment);       // afterSaveCommit suppressed
});
// Transaction committed — but afterSaveCommit events are permanently lost

This pattern is common when multiple tables need to be saved atomically (cross-table transactions).

Root Cause

The gate is Table::_transactionCommitted():

protected function _transactionCommitted(bool $atomic, bool $primary): bool
{
    return !$this->getConnection()->inTransaction() && ($atomic || $primary);
}

When an outer transaction is active, inTransaction() returns true, the method returns false, and dispatchEvent() is never called. There is no mechanism to defer the event for later dispatch.

CakePHP's own saveManyOrFail() solves this internally — _saveMany() collects entities and dispatches afterSaveCommit for each one after its transaction commits (Table.php lines 2392-2398). This proves the intent is for these events to fire after commit. The gap is that application-level transactional() blocks have no access to an equivalent mechanism.

Failing Tests

Branch feature/connection-on-commit contains four tests demonstrating the issue:

Test Result What it shows
testAfterSaveCommitFiringAfterOuterTransactionCommits FAIL Single save inside transactional() — event lost after commit
testAfterSaveCommitForCrossTableTransactional FAIL Cross-table saves — both events lost
testAfterDeleteCommitFiringAfterOuterTransactionCommits FAIL Delete inside transactional() — event lost
testAfterSaveCommitNotFiredOnRollback PASS Control: events correctly discarded on rollback

Proposed Solution: Connection::onCommit() + deferred dispatch

A general-purpose post-commit callback mechanism on Connection, used by Table to defer suppressed events.

Connection changes (~20 lines)

protected array $afterCommitCallbacks = [];

public function onCommit(Closure $callback): void
{
    if (!$this->inTransaction()) {
        $callback();
        return;
    }
    $this->afterCommitCallbacks[] = $callback;
}

In commit(), after outermost commit succeeds, flush callbacks. In rollback(), discard them when outermost rolls back.

Table changes (~10 lines)

At each dispatch point gated by _transactionCommitted(), add deferral:

if ($this->_transactionCommitted($options['atomic'], $options['_primary'])) {
    $this->dispatchEvent('Model.afterSaveCommit', compact('entity', 'options'));
} elseif ($this->getConnection()->inTransaction()) {
    $this->getConnection()->onCommit(
        fn() => $this->dispatchEvent('Model.afterSaveCommit', compact('entity', 'options'))
    );
}

Applies to 5 dispatch points: save(), delete(), findOrCreate(), _saveMany(), deleteMany().

Industry precedent

This is a well-established pattern:

Design Decisions for Discussion

  1. Callback exceptions — The transaction is already committed when callbacks flush. Should exceptions propagate? Should remaining callbacks still execute (collect-and-throw)?

  2. Savepoint rollback semantics — When a nested savepoint rolls back, should callbacks registered after that savepoint be discarded? Django's on_commit() takes the simpler approach: callbacks always run after outermost commit regardless of intermediate savepoint rollbacks.

  3. Reentrancy — What if a callback calls transactional() again? The flush loop should drain the queue safely even if new callbacks are added during flushing.

  4. _saveMany() / deleteMany() deferred loops — These existing loops become redundant once individual save()/delete() handle deferral. They could be removed to avoid double dispatch, but this should be verified with targeted equivalence tests.

  5. ConnectionInterface — Should onCommit() be added to the interface?

CakePHP Version

5.x (tested against 5.3.1)

Metadata

Metadata

Assignees

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions