Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[RateLimiter] Use RateLimiterFactory with custom limit#51403
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.4
Are you sure you want to change the base?
Conversation
Add an optional custom limit to the RateLimiterFactory to overwrite configured values.
carsonbot commentedAug 16, 2023
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
| * Parsing a custom limit to this function will overwrite the limit in your configuration for this request. | ||
| * When using a NoLimiter the limit will be ignored. | ||
| */ | ||
| public function create(string $key = null, int $limit = null): LimiterInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What if you also want at some point to configure the rate dynamically? A solution could be to provide the whole config instead (and resolve it), WDYT?
| publicfunction create(string$key =null,int$limit =null):LimiterInterface | |
| publicfunction create(string$key =null,array$config =null):LimiterInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this would then force to validate the provided config each time this method is called.
To me, if you want to do things dynamically, it would be better to introduce a RateLimiterFactoryInterface allowing to do a custom implementation of the factory than adding a configuration override argument.
Thus, if you are creating a custom caller for the factory, you could already instantiate the limiter yourselves. And if you don't call it yourselves (relying on a usage in Symfony), you would not be able to pass a custom config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Indeed, I that if we add an argument to the factory method to customize it, there is no point to not adding others (even if the limit is the only common parameter).
That's why, in my opinion too, it is way better to indeed introduce aRateLimiterFactoryInterface so there is no limit to the customization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Alright :) I will create the interface instead later this week. Thanks guys!
kbond commentedMar 26, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I put some ideas on this feature here:#51401 (comment) |
Uh oh!
There was an error while loading.Please reload this page.
Add an optional custom limit to the RateLimiterFactory to overwrite configured values.
The RateLimiterFactory by default uses the limit set in Config. However in certain situations you might want to change this limit per user. This allows for for that option. When no limit is passed to the Factory, the default configured value will be used.