Do any of the added tests fail without this PR?

It is very unclear to me what this PR is changing.

If $this->cache->get() returns null for an expired key, forcing $this->cache->forget() (through $this->resetAttempts()) has no practical effects.

The next $this->attempts() call just calls $this->cache->get() under the hood with a default value of zero. So if the first $this->cache->get() already missed an expired key, the second call (through $this->attempts()) will already return the default value.

If none of the tests fail without this PR, please add a test case that does fail to highlight any added benefits from the proposed changes.

Thanks for the feedback!

I've added a new test testRateLimiterFailsWithoutFix() that fails without this PR to clearly demonstrate the problem:

Without the PR, after the decay period expires, tooManyAttempts() still returns true even though the key has expired, meaning the attempts do not reset properly.

With the PR, tooManyAttempts() correctly resets attempts when the cache key has expired, and the test now passes.

This ensures the PR’s benefit is explicit: users are no longer throttled longer than expected, and the RateLimiter behaves consistently after the decay period.

The previously added test testRateLimiterResetsAfterDecay() also confirms the correct behavior post-fix, covering multiple hits and reset scenarios.

I need someone to explain why this is the case though:

Without the PR, after the decay period expires, tooManyAttempts() still returns true even though the key has expired, meaning the attempts do not reset properly.

The reason this happens is because tooManyAttempts() internally checks attempts() >= maxAttempts without verifying if the key has expired and been removed from the cache.

Here’s the sequence without the fix:

Before decay period ends → The cache key exists and has a hit count (e.g., attempts = 5). tooManyAttempts() correctly returns true.

After decay period expires →

$this->cache->get($key) returns null because the key is gone.

However, tooManyAttempts() does not explicitly reset attempts here; it simply calls attempts(), which defaults to returning the previous in-memory counter value that hasn't been cleared.

So the comparison attempts() >= maxAttempts still evaluates as true.

This means the "lock" persists longer than the intended decay period because there is no explicit reset after an expired key is detected.

The fix ensures that if the cache key is missing (expired), we force a reset via resetAttempts(), so the next tooManyAttempts() call starts from zero, restoring expected rate limiting behavior.

The added failing test proves this: without the fix, the key expiration alone is not enough to reset the in-memory count, and throttling incorrectly continues.

Tests are failing on this PR.

Initially, all tests were passing.

However, after @rodrigopedra suggested demonstrating that the fix is truly effective, I updated the tests to explicitly reproduce the failing scenario without the PR applied.

As a result, the current failing tests are intentional — they confirm that the issue exists in the original implementation and is resolved by this PR. Once the fix is applied, these tests pass, clearly showing the benefit of the change.

Once the fix is applied, these tests pass, clearly showing the benefit of the change.

Tests in the PR pipeline are run against the PR code, not against unmerged code.

If they are still failing, the proposed changes are ineffective.

The added test case does not fail with the current code.

In a new Laravel project I added this artisan command:

<?php // routes/console.php

use Illuminate\Cache\ArrayStore;
use Illuminate\Cache\RateLimiter;
use Illuminate\Cache\Repository;
use Illuminate\Support\Facades\Artisan;

Artisan::command('app:test', function () {
    // Use in-memory cache directly
    $cache = new Repository(new ArrayStore());
    $limiter = new RateLimiter($cache);

    $key = 'test-fix-key';
    $maxAttempts = 1;
    $decaySeconds = 1;

    // Hit the key once
    $limiter->hit($key, $decaySeconds);
    \dump($limiter->tooManyAttempts($key, $maxAttempts)); // assert true

    // Wait for decay period
    sleep($decaySeconds + 1);

    // Without the PR fix, tooManyAttempts() would still return true
    // With our PR, this passes
    \dump($limiter->tooManyAttempts($key, $maxAttempts)); // assert false
});

And got these results:

$ php artisan --version
Laravel Framework 12.24.0

$ php artisan app:test
true // routes/console.php:19
false // routes/console.php:26

it simply calls attempts(), which defaults to returning the previous in-memory counter value that hasn't been cleared.

RateLimiter@attempts() does not read any in-memory value.

It retrieves the value from the cache store every time, which will return null (defaulted to zero) when the key is missing or expired:

Thank you for taking the time to explain this and for your guidance. I really appreciate it and hope that in the future I can contribute more practical and valuable PRs.

The test that fails on the test pipeline is the first one added: testRateLimiterResetsAfterDecay.

It fails because the foreach hits the test 3 times ($i = 0, 1, 2), and thus on the third time the $this->assertFalse($limiter->tooManyAttempts($key, $maxAttempts)); statement fails.

The foreach should iterate twice only (e.g., for ($i = 0; $i < $maxAttempts - 1; $i++), given that $maxAttempts = 3).

@alipowerful7, if you have Docker installed, you can run this command on your local laravel/framework fork:

$ ./bin/test.sh

This will set up everything Laravel needs to run all tests locally with the provided Docker containers.

Then you can run all tests locally by running:

$ ./vendor/bin/phpunit 

Then you can verify if any tests fail before submitting new changes or a new PR.

After working on your changes, you can then run:

$ ./bin/test.sh --down

To stop and remove all added containers needed for tests.

I really appreciate you taking the time to guide me through this. Your clear explanations and patience have been invaluable, and I’ve learned a lot from your insights. I hope to apply what I’ve learned to submit more effective and meaningful PRs in the future.

@alipowerful7 no worries.

Every journey starts with a first step, and I also had valuable help when I started mine.

Thanks for the kind words and have a nice day =)

Marking this as draft until tests are passing.