Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RateLimiter] Make time dependency explicit and mockable #47999

Open
Slamdunk opened this issue Oct 26, 2022 · 4 comments · May be fixed by #49222
Open

[RateLimiter] Make time dependency explicit and mockable #47999

Slamdunk opened this issue Oct 26, 2022 · 4 comments · May be fixed by #49222

Comments

@Slamdunk
Copy link
Contributor

Description

Currently the RateLimiter component has time dependency hardcoded, with lot of microtime(true) calls everywhere making it unmockable and thus untestable.

Internally Symfony leverages the \Symfony\Bridge\PhpUnit\ClockMock::register to mock and test the component, but this is not a suitable solution for the component consumers.

Example

The Optimal solution would be to set for every class the ClockInterface dependency in the constructor, but of course this would be a BC Break; we can do it in the next major, but not now in v6.

A backward compatible workaround for v6 could be to have a dedicated clock registry that every RateLimiter class consumes, something like:

namespace Symfony\Component\RateLimiter;

use Symfony\Component\Clock\ClockInterface;
use Symfony\Component\Clock\NativeClock;

/**
 * @deprecated Symfony v7 is going to have explicit ClockInterface dependency
 */
final class RateLimiterClockRegistry
{
    private static ClockInterface $clock;

    public static function setClock(ClockInterface $clock): void
    {
        self::$clock = $clock;
    }

    public static function getClock(): ClockInterface
    {
        if (! isset(self::$clock)) {
            self::$clock = new NativeClock();
        }

        return self::$clock;
    }
}

Then all call can be edited to:

-$now = microtime(true);
+$now = (float) RateLimiterClockRegistry::getClock()->format("U.u");

And the component become mockable by consumers as well.

I am willing to create the PR if this sounds good to the maintainers.

@nicolas-grekas
Copy link
Member

Internally Symfony leverages the \Symfony\Bridge\PhpUnit\ClockMock::register to mock and test the component, but this is not a suitable solution for the component consumers.

Can you elaborate why about this?

@Slamdunk
Copy link
Contributor Author

\Symfony\Bridge\PhpUnit\ClockMock::register it's not inside the rate limiter package.
A user shouldn't require another external package just to mock the rate limiter, especially if it leverages hacky eval calls and cleaner alternatives can be easily implemented.

@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@Slamdunk
Copy link
Contributor Author

Slamdunk commented May 3, 2023

Would you still like to see this feature?

Yes

@carsonbot carsonbot removed the Stalled label May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants