-
Notifications
You must be signed in to change notification settings - Fork 50
add ThrottlePlugin #460
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
add ThrottlePlugin #460
Changes from 10 commits
7cb6fb1
fddf99c
abc4007
b391113
904108e
d3fefda
27b776f
774c740
f10afcb
e9e64bf
8f2f9bf
12e1816
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
use Http\Client\Common\HttpMethodsClient; | ||
use Http\Client\Common\HttpMethodsClientInterface; | ||
use Http\Client\Common\Plugin\AuthenticationPlugin; | ||
use Http\Client\Common\Plugin\ThrottlePlugin; | ||
use Http\Client\Common\PluginClient; | ||
use Http\Client\Common\PluginClientFactory; | ||
use Http\Client\HttpAsyncClient; | ||
|
@@ -35,6 +36,7 @@ | |
use Symfony\Component\DependencyInjection\Loader\XmlFileLoader; | ||
use Symfony\Component\DependencyInjection\Reference; | ||
use Symfony\Component\HttpKernel\Kernel; | ||
use Symfony\Component\RateLimiter\LimiterInterface; | ||
use Twig\Environment as TwigEnvironment; | ||
|
||
/** | ||
|
@@ -292,6 +294,28 @@ private function configurePluginByName($name, Definition $definition, array $con | |
|
||
break; | ||
|
||
case 'throttle': | ||
if (!\interface_exists(LimiterInterface::class)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this check is redundant. the throttle-plugin repository depends on symfony/rate-limiter. checking for the ThrottlePlugin is enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
throw new InvalidConfigurationException('You need to require the Rate Limiter to be able to use it: "composer require symfony/rate-limiter".'); | ||
} | ||
|
||
if (!\class_exists(ThrottlePlugin::class)) { | ||
throw new InvalidConfigurationException('You need to require the Throttle Plugin to be able to use it: "composer require php-http/throttle-plugin".'); | ||
} | ||
|
||
$key = $config['name'] ? '.'.$config['name'] : ''; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if name is required, this check is for the special case where name is set to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and i think symfony configuration will not allow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, check #462 |
||
$container | ||
->register($serviceId.$key, LimiterInterface::class) | ||
->setFactory([new Reference('limiter.'.$config['name']), 'create']) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this does not check if the name is set or not. and if the name is empty, it will use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dbu No, it was my mistake, I assume that if the default value is not specified, then it is required by default |
||
->addArgument($config['key']) | ||
->setPublic(false); | ||
|
||
$definition->replaceArgument(0, new Reference($serviceId.$key)); | ||
$definition->setArgument('$tokens', $config['tokens']); | ||
$definition->setArgument('$maxTime', $config['max_time']); | ||
|
||
break; | ||
|
||
/* client specific plugins */ | ||
|
||
case 'add_host': | ||
|
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.
looking at the test failure in #461: what is the name here? and what is key?
the name is not made required, but the configuration class does not handle a missing name field.
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.
@dbu You need to set name, name this is required option
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.
then we are missing
isRequired()
can you please also add an
info()
as well with an explanation what this name is? the name of what thing do we specify here?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.
Done, check #462