Throttle plugin small fixes #462
Conversation
| ->scalarNode('key')->defaultNull()->end() | ||
| ->integerNode('tokens')->defaultValue(1)->end() | ||
| ->floatNode('max_time')->defaultNull()->end() | ||
| ->scalarNode('name')->isRequired()->info('Rate limiter configuration name from rate_limiter.yaml')->end() |
There was a problem hiding this comment.
this will tricky when a user configures their own service for a rate limiter factory and does not call it limiter.{name}.
in other places, we ask for a full service name for things. i think i would prefer to do the same here:
| ->scalarNode('name')->isRequired()->info('Rate limiter configuration name from rate_limiter.yaml')->end() | |
| ->scalarNode('name')->isRequired()->info('Rate limiter factory service name, e.g. limiter.http_client for a rate limiter called http_client')->end() |
wdyt? if we change it here, we also need to adjust the extension class.
and now that i understand what it is, it should be called more specific limiter_name. or while we do change it to be the service, limiter_service.
There was a problem hiding this comment.
@dbu Symfony automatic create factory in container, I just have this config
I think using just "name" more understandable than whole name in container, like "limiter.http_client"
There was a problem hiding this comment.
yes, when one configures in framework bundle, that creates the services with this naming pattern.
the thing is that a user might configure their own service rather than use the framework configuration, e.g. when they have a custom implementation of the rate limiter. and for that i think its more consistent with the rest of the bundle to have the full service name.
Co-authored-by: David Buchmann <[email protected]>
Co-authored-by: David Buchmann <[email protected]>
|
wrapped up in #465, thanks! |
What's in this PR?
Did code more clearly