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

[9.x] Add a separate Rate Limiting Service Provider (Deferrable) #34843

Closed
wants to merge 3 commits into from
Closed

[9.x] Add a separate Rate Limiting Service Provider (Deferrable) #34843

wants to merge 3 commits into from

Conversation

paras-malhotra
Copy link
Contributor

@paras-malhotra paras-malhotra commented Oct 15, 2020

With expanding the scope of RateLimiters to jobs as well (#34829), it might be the right time to introduce a RateLimitingServiceProvider in 9.x. This can be where users can register all their rate limiters so that there's a dedicated provider to do so (instead of using AppServiceProvider for jobs and RouteServiceProvider for routes).

Also, this should improve performance slightly as it is a deferrable provider, so the named rate limiter registrations would only trigger when the app resolves the RateLimiter.

This is a breaking change because I'm extracting out the RateLimiter singleton registration to a separate provider. That's why this PR targets master.

If this PR is accepted, I'll send across a PR to the Laravel skeleton repo for a RateLimiterServiceProvider in the app/Providers directory that extends the base provider in this PR and registers the named rate limiters in the boot method.

@Shizzen83
Copy link
Contributor

I encountered this issue but I simply used the resolving method of the container to lazily setup the rate limiter

$this->app->resolving(RateLimiter::class, function (RateLimiter $rateLimiter, $app) {
            $rateLimiter->for('api', function (Request $request) {
                return Limit::perMinute(60);
            });
        });

By this way, you don't need an additional service provider (you may for clarity of course, but no need for a deferrable one).

@paras-malhotra
Copy link
Contributor Author

paras-malhotra commented Oct 15, 2020

Yep, that's a good workaround for performance. I just think it may make sense to have it as a service provider for both convenience (one class for both route and job rate limiters vs job limiters in AppServiceProvider and route limiters in RouteServiceProvider) and performance (without the workaround you suggested).

Additionally, at some point I guess there may be some use cases for sharing both route and job rate limiters, so a RateLimitingServiceProvider may be the way to go.

@GrahamCampbell
Copy link
Member

$this->app->resolving(RateLimiter::class, function (RateLimiter $rateLimiter, $app) {

This is absolutely the correct way to do this, and is not a "workaround". It is just the correct way to do it, if you want the best performance. Similar to registering blade stuff.

@paras-malhotra paras-malhotra deleted the rate_limiting_service_provider branch October 15, 2020 13:53
@paras-malhotra
Copy link
Contributor Author

You're probably right but I would have appreciated some more insights into when to create a ServiceProvider rather than a resolving callback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants