[RateLimiter] Use RateLimiterFactory with custom limit #51403
Conversation
Add an optional custom limit to the RateLimiterFactory to overwrite configured values.
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution 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.
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?
| public function create(string $key = null, int $limit = null): LimiterInterface | |
| public function create(string $key = null, array $config = null): LimiterInterface |
There was a problem hiding this comment.
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.
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 a RateLimiterFactoryInterface so there is no limit to the customization.
There was a problem hiding this comment.
Alright :) I will create the interface instead later this week. Thanks guys!
|
I put some ideas on this feature here: #51401 (comment) |
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.