artisansdk/ratelimiter

Hit TTL potentially set incorrectly...

telkins opened this issue · 2 comments

This is related to #4 and #5 .

This is a little more difficult for me to figure out this late at night, but it looks like the TTL that's set in Limiter::hit() is far too low. In my simple tests, it's expiring super fast.

Here's ArtisanSdk\RateLimiter\Limiter::hit():

    public function hit(): int
    {
        foreach ($this->buckets as $bucket) {
            $bucket->fill();
            $this->cache->put(
                $bucket->key(),
                $bucket->toArray(),
                ceil($bucket->duration() / 60)
            );
        }

The TTL is set to the number of seconds divided by 60....giving minutes. Again, unless I'm wrong, the TTL should be in seconds.

I'll make a quick PR for this, too, but there may be other minutes/seconds issues floating about. 😳 Or....I could be wrong.... 😬

The cache should be in seconds for Laravel 6+ while my signatures already assumed that in configuration. So therein lies the problem, this code was written for Laravel 5 originally and Laravel changed cache signatures at some point. So this should just be $bucket->duration() instead of ceil($bucket->duration / 60). I've got the 1.0.0-rc1 tag for Laravel 5 folks so let's just clean up the code to remove the minute assumption for cache duration.

Tag released 1.0.0-rc3 is compatible with specifying timeout() and duration() in seconds instead of minutes.