Skip to content

Pass CommandInterface to retry handler (fixes #1155, #1130)#1171

Open
bautrukevich wants to merge 3 commits intoyiisoft:masterfrom
bautrukevich:feature/pass-command-to-retry-handler
Open

Pass CommandInterface to retry handler (fixes #1155, #1130)#1171
bautrukevich wants to merge 3 commits intoyiisoft:masterfrom
bautrukevich:feature/pass-command-to-retry-handler

Conversation

@bautrukevich
Copy link
Copy Markdown

Q A
Is bugfix? ✔️
New feature? ✔️
Breaks BC? ✔️
Tests pass? ✔️
Fixed issues #1155, #1130

Summary

Implements feature request #1155 and #1130.

Allows retry handlers to access CommandInterface to implement connection renewal logic.

Changes

  • Pass CommandInterface as third parameter to retry handler callback
  • Updated docblocks in CommandInterface and AbstractCommand
  • Retry handler signature changed from (Exception, int): bool to (Exception, int, CommandInterface): bool

Breaking Change

Yes - existing retry handlers need to be updated to accept the new parameter (can be ignored with _).

Usage Example

$command->setRetryHandler(function (Exception $e, int $attempt, CommandInterface $cmd) {
    if ($attempt === 0 && strpos($e->getMessage(), 'no connection') !== false) {
        $cmd->getDb()->close();
        $cmd->getDb()->open();
        return true; // Retry
    }
    return false;
});

Migration Guide

Update existing retry handlers:

// Before
->setRetryHandler(fn(Exception $e, int $attempt) => $attempt === 0)

// After  
->setRetryHandler(fn(Exception $e, int $attempt, $cmd) => $attempt === 0)
// or ignore it:
->setRetryHandler(fn(Exception $e, int $attempt, $_) => $attempt === 0)

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 29, 2026

Codecov Report

❌ Patch coverage is 63.63636% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.39%. Comparing base (dffa745) to head (ee253e3).

Files with missing lines Patch % Lines
src/Driver/Pdo/AbstractPdoCommand.php 63.63% 8 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1171      +/-   ##
============================================
- Coverage     98.62%   98.39%   -0.23%     
- Complexity     1645     1655      +10     
============================================
  Files           120      120              
  Lines          4292     4312      +20     
============================================
+ Hits           4233     4243      +10     
- Misses           59       69      +10     

☔ View full report in Codecov by Sentry.
📢 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.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the command retry mechanism to provide more context to retry handlers by passing the executing command instance, enabling smarter retry logic (e.g., recreating statements / inspecting params), as requested in #1155 and #1130.

Changes:

  • Pass CommandInterface ($this) as a third argument to the retry handler callback in PDO command execution.
  • Update CommandInterface::setRetryHandler() docblock to document the new handler signature and attempt indexing.
  • Add/expand AbstractCommand::setRetryHandler() docblock to reflect the new handler signature.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
src/Driver/Pdo/AbstractPdoCommand.php Passes the executing command instance into the retry handler during internalExecute().
src/Command/CommandInterface.php Updates retry-handler docblock/signature description to include CommandInterface parameter and attempt semantics.
src/Command/AbstractCommand.php Adds a matching retry-handler docblock on the concrete setRetryHandler() implementation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 211 to 213
if ($this->retryHandler === null || !($this->retryHandler)($e, $attempt, $this)) {
throw $e;
}
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change calls the retry handler with 3 arguments. There are existing in-repo handlers that still accept only 2 parameters (e.g. tests/Common/CommonCommandTest.php), which will now throw an ArgumentCountError at runtime. Please update all internal usages/tests/docs accordingly, or consider a backward-compatible invocation strategy (e.g. detect handler arity and pass only the supported number of args).

Suggested change
if ($this->retryHandler === null || !($this->retryHandler)($e, $attempt, $this)) {
throw $e;
}
$handler = $this->retryHandler;
if ($handler === null) {
throw $e;
}
$callable = \Closure::fromCallable($handler);
$reflection = new \ReflectionFunction($callable);
$paramCount = $reflection->getNumberOfParameters();
if ($paramCount >= 3) {
$shouldRetry = $callable($e, $attempt, $this);
} elseif ($paramCount === 2) {
$shouldRetry = $callable($e, $attempt);
} elseif ($paramCount === 1) {
$shouldRetry = $callable($e);
} else {
$shouldRetry = $callable();
}
if (!$shouldRetry) {
throw $e;
}

Copilot uses AI. Check for mistakes.
* to execute the command, starting from `1`.
* The closure will receive an {@see Exception} converted from the thrown database exception,
* the current attempt to execute the command (starting from `0`), and the {@see CommandInterface}
* instance to allow access to connection and parameters for reconnection logic.
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc says the retry handler can use the passed CommandInterface to access the connection for reconnection logic, but CommandInterface doesn’t expose the underlying ConnectionInterface (no getDb()/getConnection() method). Either adjust the docs to reflect what is actually accessible, or extend the API (e.g. add a connection accessor or pass ConnectionInterface as an argument) so reconnection logic is truly possible.

Suggested change
* instance to allow access to connection and parameters for reconnection logic.
* instance to allow access to the command and its parameters for custom retry logic.

Copilot uses AI. Check for mistakes.
}

/**
* Sets a closure (anonymous function) which called when a database exception is thrown when executing the command.
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docblock grammar: “Sets a closure ... which called” is ungrammatical; please change to “which is called” / “that is called”.

Suggested change
* Sets a closure (anonymous function) which called when a database exception is thrown when executing the command.
* Sets a closure (anonymous function) which is called when a database exception is thrown when executing the command.

Copilot uses AI. Check for mistakes.
*
* The closure will receive an {@see Exception} converted from the thrown database exception,
* the current attempt to execute the command (starting from `0`), and the {@see CommandInterface}
* instance to allow access to connection and parameters.
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doc states the passed CommandInterface allows access to the connection, but CommandInterface doesn’t provide any public accessor to the underlying ConnectionInterface. Either remove/adjust this wording, or add an explicit API to access the connection so retry handlers can actually implement reconnection logic as described.

Suggested change
* instance to allow access to connection and parameters.
* instance to allow access to the command's parameters and other state exposed by the interface.

Copilot uses AI. Check for mistakes.
$e = (new ConvertException($e, $rawSql))->run();

if ($this->retryHandler === null || !($this->retryHandler)($e, $attempt)) {
// ✨ Pass $this (CommandInterface) to retry handler
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inline comment uses a decorative emoji ("✨"), which is inconsistent with typical source comments and can be noisy in diffs/blame. Consider removing it or rewriting as a plain, descriptive comment (or relying on the code being self-explanatory).

Suggested change
// Pass $this (CommandInterface) to retry handler
// Pass $this (CommandInterface) to the retry handler.

Copilot uses AI. Check for mistakes.
bautrukevich and others added 2 commits March 29, 2026 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants