Modernize codebase with PHP 8.1+ features and expand test coverage#9
Modernize codebase with PHP 8.1+ features and expand test coverage#9aarondfrancis merged 6 commits intomainfrom
Conversation
- Add property types, return types, and union types to all source files - Use readonly properties in Result class - Fix composer.json version constraint typos and update dev dependencies - Add PHP 8.4 to test matrix, remove unused MySQL service from CI - Add 12 new tests covering Result, global disable, combined thresholds 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request upgrades PHP minimum version requirement to 8.1, updates Laravel dependencies to support versions 10–12, enhances the entire codebase with strict typing for properties and methods, removes MySQL service from tests, and adds comprehensive test coverage for threshold and result handling functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Flaky
participant Arbiter
participant Cache
participant Handler
Caller->>Flaky: run(callable)
activate Flaky
Flaky->>Flaky: shouldThrowImmediately()?
alt Protected bypassed or globally disabled
Flaky->>Caller: throw immediately
else Protection active
Flaky->>Flaky: execute callable<br/>(with retry logic)
alt Execution succeeds
Flaky->>Arbiter: handle(null)
Arbiter->>Cache: updateCachedStats(null)
Arbiter->>Caller: return Result(value, null)
else Execution throws Throwable
Flaky->>Arbiter: handle(exception)
Arbiter->>Arbiter: callHandler(exception)?
Arbiter->>Handler: execute if set
Arbiter->>Cache: updateCachedStats(exception)
Arbiter->>Arbiter: checkThresholds()<br/>outOfBounds, tooMany*, beyondDeadline
alt Thresholds exceeded
Arbiter->>Caller: rethrow or Result(null, exception)
else Thresholds not exceeded
alt retry configured & when() matches
Flaky->>Flaky: retry execution
else No retry
Flaky->>Caller: return Result(null, exception)
end
end
end
end
deactivate Flaky
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Bump orchestra/testbench minimum versions to avoid buggy early releases that cause "undeclared static property" errors in CI. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/Arbiter.php (1)
91-94: Potential null comparison inbeyondDeadline().
$this->deadlineis typed as?int, but this method compares it directly with>. While PHP will treatnullas0in numeric context (makingnow()->timestamp > nullreturntrue), this may not be the intended behavior if called beforehandle()initializes the deadline.Consider adding a null guard for defensive programming:
public function beyondDeadline(): bool { + if ($this->deadline === null) { + return false; + } return now()->timestamp > $this->deadline; }Alternatively, if returning
truewhen deadline is unset is intentional, add a comment clarifying this behavior.tests/Support/Commands/FlakyNoVaryCommand.php (1)
16-22: LGTM!The test command cleanly surfaces the internal arbiter key for testing command ID stability. The
getProtectedmacro usage aligns with the test infrastructure defined inFlakyTestServiceProvider.Optional: Consider adding an explicit return type to
handle()(e.g.,voidorint) for consistency with PHP 8.1+ strict typing throughout this PR, though Laravel allows omitting it.tests/Unit/CombinedThresholdsTest.php (1)
94-116: Consider resetting Carbon after the test.The test correctly uses
Carbon::setTestNow()to freeze time but doesn't reset it afterward. While this may be handled by the base class or PHPUnit teardown, explicitly resetting in atearDownor at the end of the test ensures no state leakage to other tests.Consider adding cleanup:
public function deadline_and_consecutive_work_together(): void { Carbon::setTestNow(); $id = __FUNCTION__; // 2 failures allowed Flaky::make($id) ->allowFailuresForSeconds(60) ->allowConsecutiveFailures(2) ->run(fn() => throw new Exception); Flaky::make($id) ->allowFailuresForSeconds(60) ->allowConsecutiveFailures(2) ->run(fn() => throw new Exception); // 3rd consecutive throws even within deadline $this->expectException(Exception::class); Flaky::make($id) ->allowFailuresForSeconds(60) ->allowConsecutiveFailures(2) ->run(fn() => throw new Exception); + + // Note: Carbon reset would happen in tearDown if needed }Alternatively, ensure the base
Baseclass handles Carbon cleanup in its teardown.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/style.yml(1 hunks).github/workflows/tests.yml(1 hunks).gitignore(1 hunks)composer.json(1 hunks)src/Arbiter.php(1 hunks)src/Flaky.php(5 hunks)src/FlakyCommand.php(4 hunks)src/Providers/FlakyServiceProvider.php(2 hunks)src/Result.php(1 hunks)tests/Support/Commands/FlakyNoVaryCommand.php(1 hunks)tests/Support/FlakyTestServiceProvider.php(2 hunks)tests/Unit/CombinedThresholdsTest.php(1 hunks)tests/Unit/CommandTest.php(1 hunks)tests/Unit/GlobalDisableTest.php(1 hunks)tests/Unit/ResultTest.php(1 hunks)tests/Unit/RetryTest.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
tests/Unit/ResultTest.php (1)
src/Result.php (2)
Result(12-40)throw(32-39)
tests/Support/Commands/FlakyNoVaryCommand.php (2)
src/FlakyCommand.php (3)
FlakyCommand(17-93)make(26-29)instance(46-52)src/Arbiter.php (1)
handle(55-69)
src/Result.php (1)
src/Arbiter.php (1)
__construct(38-53)
src/FlakyCommand.php (1)
src/Flaky.php (3)
make(28-31)Flaky(12-242)disableFlakyProtection(87-92)
src/Arbiter.php (1)
src/Flaky.php (3)
__construct(43-47)handle(69-76)handleFailures(109-114)
src/Flaky.php (2)
src/Arbiter.php (4)
Arbiter(15-128)__construct(38-53)handle(55-69)handleFailures(71-74)src/Result.php (2)
__construct(24-30)Result(12-40)
🪛 PHPMD (2.15.0)
src/FlakyCommand.php
51-51: Avoid unused local variables such as '$disabled'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (29)
.github/workflows/tests.yml (1)
20-28: LGTM!The PHP matrix extension to include 8.4 is appropriate, and the exclusions for Laravel 11/12 with PHP 8.1 are correct since those Laravel versions require PHP 8.2+.
.gitignore (1)
15-15: LGTM!Ignoring local Claude settings file is appropriate for keeping developer-specific tool configurations out of version control.
.github/workflows/style.yml (1)
21-21: LGTM!Updating the style check workflow to PHP 8.4 aligns with the expanded test matrix and ensures Pint runs against the latest PHP version.
src/Providers/FlakyServiceProvider.php (1)
17-38: LGTM!The explicit
voidreturn types on methods and closures are appropriate PHP 8.1+ modernizations that improve code clarity and enable stricter static analysis.src/Arbiter.php (3)
19-36: Well-structured typed properties.Using
int|floatfor threshold values to supportINFis a pragmatic approach. The typed properties improve code clarity and enable better static analysis.
50-52: Good use ofneverreturn type.The
neverreturn type correctly documents that this default handler always throws and never returns normally.
106-127: Stats persistence logic is sound.The conditional logic correctly handles resetting thresholds when exceeded and preserving cumulative stats on success. The explicit
$failed = $exception !== nullcheck improves readability.tests/Support/FlakyTestServiceProvider.php (1)
11-11: LGTM!The new
FlakyNoVaryCommandis properly imported and registered following the existing pattern. The alphabetical ordering of commands is maintained.Also applies to: 22-22
tests/Unit/CommandTest.php (1)
16-27: LGTM!The test correctly validates that without
varyOnInput, the command ID remains consistent regardless of input arguments. The assertions verify both key equality and the expected key format.tests/Unit/RetryTest.php (2)
17-17: LGTM!Adding explicit
voidreturn type aligns with PHP 8.1+ strict typing.
38-66: LGTM!The test comprehensively validates callable-based retry logic:
- Correctly expects 5 attempts when the callable returns
trueforTimeoutException- Verifies only 1 attempt (no retry) when callable returns
falsefor baseExceptionThe shared counter across runs is a good pattern for validating cumulative behavior.
tests/Unit/ResultTest.php (1)
13-58: LGTM!Comprehensive test coverage for the
Resultclass:
- Tests both exception throwing and fluent return behavior of
throw()- Validates all readonly properties (
value,succeeded,failed,exception) for both success and failure states- Uses
assertSameappropriately for object identity checkstests/Unit/GlobalDisableTest.php (2)
15-20: LGTM!Proper test isolation by re-enabling Flaky in
tearDown(). Callingparent::tearDown()after cleanup is the correct order.
22-65: LGTM!Comprehensive test coverage for global enable/disable functionality:
- Validates that
globallyDisable()bypasses protection (exception propagates immediately)- Validates that
globallyEnable()restores protection (exception is caught)- Tests
throwFailures()handler correctly throws the original exceptionThe
allowConsecutiveFailures(0)configuration in the third test ensures the handler triggers on the first failure.src/Result.php (3)
16-22: LGTM!The
readonlyproperties provide strong immutability guarantees for this value object. Explicit types (mixed,bool,?Throwable) improve IDE support and static analysis.
24-30: LGTM!Constructor cleanly computes derived state from the exception parameter. The logic
$succeeded = $exception === nullis clear and correct.
32-39: LGTM!The
throw(): staticreturn type enables fluent chaining (e.g.,$result->throw()->value) when no exception is present, while correctly re-throwing when an exception exists.tests/Unit/CombinedThresholdsTest.php (3)
17-23: LGTM! Standard test setup pattern.The cache flush in
setUpensures test isolation, which is essential for these stateful threshold tests.
25-61: Well-structured test for consecutive reset behavior.The test correctly validates that:
- Consecutive failures accumulate (2 failures)
- Success resets the consecutive counter
- After reset, 3 more consecutive failures are allowed
- The 4th consecutive failure (exceeding the limit of 3) throws
The test logic is sound and the assertions are appropriate.
63-92: Good coverage for total failure accumulation.The test validates that total failures persist across successes (2 + 3 = 5), and the 6th failure exceeds the limit of 5 and throws. The high consecutive limit (100) ensures only the total limit is being tested.
src/FlakyCommand.php (3)
21-24: Good modernization with PHP 8.1+ typed properties.The union type
array|falsefor$varyOnInputis appropriate for distinguishing between "not set" (false) and "set with keys" (array).
69-87: Improved condition clarity.The change from
if (count($this->varyOnInput))toif (count($this->varyOnInput) > 0)is a good clarity improvement, making the intent explicit.
89-92: Return typemixedis appropriate for the magic method.The
__callmethod correctly proxies to the Flaky instance and returns whatever the underlying method returns.src/Flaky.php (6)
16-26: Clean property declarations with PHP 8.1+ features.The strict typing and PHPDoc annotations for complex types like
array<class-string<Throwable>>are well-applied.
28-41: Static methods properly typed withstaticreturn andvoidreturn types.The global disable/enable pattern using a static boolean is appropriate for this use case.
49-67: Well-structured run method with proper exception flow.The logic correctly:
- Attempts retry with stored parameters
- Checks if exception should throw immediately (bypassed protection or non-flaky exception)
- Delegates to arbiter for threshold handling
- Returns a Result object
The null check for
$exceptioninshouldThrowImmediately(line 231) ensures the throw on line 61 is safe.
69-76: Thehandlemethod signature accepts nullable Throwable.The method properly wraps the exception-throwing logic in
run(). The null check on line 72 ensures exceptions are only thrown when provided.
94-107: Flexible retry configuration with normalizedwhenpredicate.The union type
string|array|callable|nullcombined withnormalizeRetryWhenprovides a clean API for specifying retriable exceptions.
174-186: Union typeint|floatallows INF for unlimited failures.This aligns with
Arbiter's property types whereINFis used as the default for unlimited thresholds.
| "require": { | ||
| "php": "^8.0", | ||
| "illuminate/support": "^10|^11.0|^12.0", | ||
| "illuminate/cache": "^10|^11.0|^12.0", | ||
| "illuminate/console": "^10|^11.|^12.00" | ||
| "php": "^8.1", | ||
| "illuminate/support": "^10.0|^11.0|^12.0", | ||
| "illuminate/cache": "^10.0|^11.0|^12.0", | ||
| "illuminate/console": "^10.0|^11.0|^12.0" | ||
| }, | ||
| "require-dev": { | ||
| "mockery/mockery": "^1.3.3", | ||
| "phpunit/phpunit": ">=8.5.23|^9|^10", | ||
| "mockery/mockery": "^1.6", | ||
| "phpunit/phpunit": "^10.5|^11.0", | ||
| "orchestra/testbench": "^8.0|^9.0|^10.0" |
There was a problem hiding this comment.
🧩 Analysis chain
Laravel 10 PHP minimum version requirement
<web_search_result>
Laravel 10 requires PHP 8.1 or greater. (laravel.com)
Citations:
- 1: https://laravel.com/docs/10.x/releases?utm_source=openai
</web_search_result>
Laravel 11 PHP minimum version requirement
<web_search_result>
Laravel 11 requires PHP 8.2 or greater. [1][2]
Sources:
[1] Laravel 11 release notes (official docs).
[2] Laravel 11 upgrade guide (official docs).
</web_search_result>
Laravel 12 PHP minimum version requirement
<web_search_result>
Laravel 12 requires PHP 8.2 or higher (supports PHP 8.2–8.4). [1][2]
Sources:
[1] Laravel 12 release notes (laravel.com).
[2] Laravel News — "Laravel 12 is Now Released".
</web_search_result>
Orchestra Testbench version compatibility Laravel
<web_search_result>
Orchestra Testbench version mapping to Laravel (short):
- Laravel 5.x → Testbench 3.x. [1]
- Laravel 6.x → Testbench 4.x. [1]
- Laravel 7.x → Testbench 5.x. [1]
- Laravel 8.x → Testbench 6.x. [1]
- Laravel 9.x → Testbench 7.x. [1]
- Laravel 10.x → Testbench 8.x. [1]
- Laravel 11.x → Testbench 9.x. [1]
- Laravel 12.x → Testbench 10.x. [1][2]
Install the testbench major that matches your Laravel major (use the ^ version constraint for the recommended minor). [1][2]
Sources: [1] orchestral/testbench-core README (GitHub). [2] Testbench compatibility table (packages/tools / Testbench docs).
</web_search_result>
PHPUnit 10.5 minimum PHP version requirement
<web_search_result>
PHPUnit 10.5 requires PHP >= 8.1. [1] [2]
Sources:
[1] PHPUnit 10.5 Installation manual.
[2] PHPUnit Supported Versions (PHP compatibility table).
</web_search_result>
PHPUnit 11.0 minimum PHP version requirement
<web_search_result>
PHPUnit 11.0 requires PHP 8.2 or later. [1][2]
Sources:
[1] PHPUnit 11 announcement — phpunit.de.
[2] Supported Versions (PHP compatibility) — phpunit.de.
</web_search_result>
Mockery 1.6 minimum PHP version requirement
<web_search_result>
Mockery 1.6 requires PHP >= 7.3. [1]
Sources:
[1] Packagist — mockery/mockery (requires php: >=7.3).
</web_search_result>
The PHP version constraint must be bumped to ^8.2 to support Laravel 11 and 12.
The original review's compatibility analysis contains a critical oversight:
- PHP ^8.1 works for Laravel 10 (correct), but Laravel 11 and 12 both require PHP ^8.2 minimum
- PHPUnit 11.0 also requires PHP ^8.2, while PHPUnit 10.5 supports PHP ^8.1
- With
"php": "^8.1"and"illuminate/support": "^10.0|^11.0|^12.0", users on PHP 8.1 attempting to install Laravel 11 or 12 will encounter dependency conflicts
Recommended fix: Change "php": "^8.1" to "php": "^8.2" to ensure proper alignment with Laravel 11/12 requirements and PHPUnit 11.0 compatibility.
🤖 Prompt for AI Agents
In composer.json around lines 12 to 21, the PHP platform requirement is set to
"^8.1" which is incompatible with Laravel 11/12 and PHPUnit 11; update the "php"
constraint to "^8.2" so the package can be installed with illuminate/* versions
^11.0|^12.0 and phpunit 11.0, then run composer validate and composer update to
refresh the lockfile and confirm there are no dependency conflicts.
| protected function exceptionIsFlaky(?Throwable $exception = null): bool | ||
| { | ||
| return is_null($this->flakyExceptions) || in_array(get_class($exception), $this->flakyExceptions, true); | ||
| return $this->flakyExceptions === null || in_array($exception::class, $this->flakyExceptions, true); | ||
| } |
There was a problem hiding this comment.
Potential null dereference when accessing exception class.
The method accepts ?Throwable $exception but accesses $exception::class without a null check. While the caller shouldThrowImmediately checks for null first (line 231), this method is protected and could be called independently in the future or via subclass.
Consider adding a null guard for defensive coding:
protected function exceptionIsFlaky(?Throwable $exception = null): bool
{
- return $this->flakyExceptions === null || in_array($exception::class, $this->flakyExceptions, true);
+ if ($exception === null) {
+ return true;
+ }
+
+ return $this->flakyExceptions === null || in_array($exception::class, $this->flakyExceptions, true);
}Alternatively, if this method should always receive a non-null exception, remove the nullable type hint:
- protected function exceptionIsFlaky(?Throwable $exception = null): bool
+ protected function exceptionIsFlaky(Throwable $exception): bool📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected function exceptionIsFlaky(?Throwable $exception = null): bool | |
| { | |
| return is_null($this->flakyExceptions) || in_array(get_class($exception), $this->flakyExceptions, true); | |
| return $this->flakyExceptions === null || in_array($exception::class, $this->flakyExceptions, true); | |
| } | |
| protected function exceptionIsFlaky(?Throwable $exception = null): bool | |
| { | |
| if ($exception === null) { | |
| return true; | |
| } | |
| return $this->flakyExceptions === null || in_array($exception::class, $this->flakyExceptions, true); | |
| } |
🤖 Prompt for AI Agents
In src/Flaky.php around lines 238 to 241, the method exceptionIsFlaky accepts a
nullable Throwable but dereferences $exception::class without a null check;
either add a defensive null guard at the start (e.g. if $exception === null
return false) before using ::class so the method is safe when called
independently, or if callers should always pass a non-null exception remove the
nullable type hint and update any callers to guarantee a Throwable is provided.
| public function instance(): Flaky | ||
| { | ||
| return Flaky::make($this->generateCommandId()) | ||
| // Only enable protection if the command is running | ||
| // via schedule not directly from CLI. | ||
| ->disableFlakyProtection($disabled = !$this->isScheduledCommand()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused variable assignment.
The variable $disabled is assigned but never used, as flagged by static analysis (PHPMD). The expression can be simplified.
public function instance(): Flaky
{
return Flaky::make($this->generateCommandId())
// Only enable protection if the command is running
// via schedule not directly from CLI.
- ->disableFlakyProtection($disabled = !$this->isScheduledCommand());
+ ->disableFlakyProtection(!$this->isScheduledCommand());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function instance(): Flaky | |
| { | |
| return Flaky::make($this->generateCommandId()) | |
| // Only enable protection if the command is running | |
| // via schedule not directly from CLI. | |
| ->disableFlakyProtection($disabled = !$this->isScheduledCommand()); | |
| } | |
| public function instance(): Flaky | |
| { | |
| return Flaky::make($this->generateCommandId()) | |
| // Only enable protection if the command is running | |
| // via schedule not directly from CLI. | |
| ->disableFlakyProtection(!$this->isScheduledCommand()); | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
51-51: Avoid unused local variables such as '$disabled'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In src/FlakyCommand.php around lines 46 to 52, the code assigns $disabled when
calling disableFlakyProtection but never uses that variable; remove the unused
assignment and pass the boolean expression directly (i.e. call
->disableFlakyProtection(!$this->isScheduledCommand())), eliminating the
unnecessary $disabled variable to satisfy static analysis.
Bump orchestra/testbench minimum versions to avoid buggy early releases that cause "undeclared static property" errors in CI. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The method is only called after null is already checked by the caller, so the nullable type hint was misleading and could cause null dereference if called directly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Summary by CodeRabbit
Release Notes
New Features
Flaky::make()method for creating instances with improved fluency.Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.