RateLimiter attempt method returns true if callback returns values considered false

[a-bashtannik]

• Laravel Version: 9.36.3 and other
• PHP Version: 8.1.11 and other
• Database Driver & Version: doesn't matter

Description:

The attempt method contains ?: operator and casts the return value of $callback function to true for all list of values considered as false.

    public function attempt($key, $maxAttempts, Closure $callback, $decaySeconds = 60)
    {
        if ($this->tooManyAttempts($key, $maxAttempts)) {
            return false;
        }

        return tap($callback() ?: true, function () use ($key, $decaySeconds) {
            $this->hit($key, $decaySeconds);
        });
    }

Steps To Reproduce:

$return = RateLimiter::attempt('key', 10, fn() => false, 1); // $return = true, not false

$return = RateLimiter::attempt('key', 10, fn() => [], 1); // $return = true, not []
$return = RateLimiter::attempt('key', 10, fn() => 0, 1); // $return = true, not 0
$return = RateLimiter::attempt('key', 10, fn() => 0.0, 1); // $return = true, not 0.0
$return = RateLimiter::attempt('key', 10, fn() => "", 1); // $return = true, not an empty string

// etc.

Obviously, it's a bug, but the fix also affects the attempt() method return value and may bring backward incompatibility to those who depend on true return.

I believe we have to change attempt() body, remove the cast:

$callback() ?: true

and return explicitly defined user value

$callback()

Case: 3rd-party APIs may return empty arrays as a successful response.

I'm ready to provide PR but kindly ask the community to advise how to deal with this bug-fix effect.
Does it make sense to push the fix in the next version branch?

[driesvints]

Heya, this is the expected behavior. The callback isn't a mechanism to control wether a message should be sent or not. It's the callback that sends the message. What you return in the callback is up to you and it returns true by default.

[a-bashtannik]

expected behavior
is up to you
the function substitutes return casting a list of values to true

[driesvints]

I believe the API was specifically designed to return a truthy value because false is only expected when the "attempt" fails because of rate limiting. Otherwise you have no way of knowing if false means that the rate limiting kicked in or you simple got back false for your callback. This indeed has the effect on the values you listed but I believe this is expected. If you want you can attempt a PR but like you said it should target the next major version.