add ThrottlePlugin #460

Big-Shark · 2024-06-14T14:38:22Z

Q	A
Bug fix?	yes
New feature?	yes
BC breaks?	no
Deprecations?	no
License	MIT

What's in this PR?

Add config settings for ThrottlePlugin

Why?

Adding the plugin without using configurations causes inconvenience.

Example

    default:
        factory: 'httplug.factory.curl'
        config:
            CURLOPT_TIMEOUT: 10
        plugins:
            - throttle:
                  name: default
                  key: default
                  tokens: 1
                  max_time: 1

Big-Shark · 2024-06-14T19:42:55Z

@dbu Please, can you check my PR. If you have some question, just ask me.

dbu

thanks, nice work! what happens if somebody would install the old version of throttle plugin with these changes? with the named parameters, we would get an error from symfony about the new parameters, i think? if that is correct, we should add a conflict in composer.json to avoid installing this with a too old version.

can you please add to the HttplugExtensionTest::testClientPlugins to enable the throttle plugin and verify the service is created (the test is generic, just add the config and the expected service name)

to keep the documentation up to date, can you please also add to https://github.com/php-http/documentation/blob/main/integrations/symfony-bundle.rst#list-of-services and https://github.com/php-http/documentation/blob/main/integrations/symfony-full-configuration.rst ?

dbu · 2024-06-15T09:16:12Z

src/DependencyInjection/HttplugExtension.php

@@ -292,6 +294,28 @@ private function configurePluginByName($name, Definition $definition, array $con

                break;

+            case 'throttle':
+                if (!\interface_exists(LimiterInterface::class)) {


this check is redundant. the throttle-plugin repository depends on symfony/rate-limiter. checking for the ThrottlePlugin is enough.

Big-Shark · 2024-06-16T20:55:43Z

@dbu

thanks, nice work! what happens if somebody would install the old version of throttle plugin with these changes? with the named parameters, we would get an error from symfony about the new parameters, i think? if that is correct, we should add a conflict in composer.json to avoid installing this with a too old version.

yes, you are right, thanks

can you please add to the HttplugExtensionTest::testClientPlugins to enable the throttle plugin and verify the service is created (the test is generic, just add the config and the expected service name)

I tried, but minimal supported version for bundle, it's 7.3, but throttle plugin works only with 7.4 and upper, and I can not run this test in 7.3. I think we can add this only in v2, if you want.
https://github.com/php-http/HttplugBundle/actions/runs/9521365832/job/26248616912

ostrolucky · 2024-06-17T07:05:06Z

I think you can make this work if you use positional arguments instead of named

Big-Shark · 2024-06-17T07:29:42Z

@ostrolucky Possible, but is it necessary, because there are no backwards compatible changes?

ostrolucky · 2024-06-17T07:40:03Z

Well I believe it will be backwards compatible with positional arguments instead of named ones.

Big-Shark · 2024-06-17T08:03:41Z

@ostrolucky It will work, but the options won't work as expected, which can also be confusing.

dbu

regarding the test with the plugin: i see. then lets add that in the 2.x branch. i will merge this now and then merge 1.x to 2.x

Big-Shark · 2024-06-17T08:08:10Z

@dbu thanks, please, create new release

dbu · 2024-06-17T08:17:26Z

on it, on it ;-)

https://github.com/php-http/HttplugBundle/releases/tag/1.34.0

and merging to 2.x in #461 (i added the test right away)

dbu · 2024-06-17T08:23:09Z

src/DependencyInjection/Configuration.php

+            ->canBeEnabled()
+            ->addDefaultsIfNotSet()
+            ->children()
+                ->scalarNode('name')->end()


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.

@dbu You need to set name, name this is required option

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?

Done, check #462

dbu · 2024-06-17T08:24:06Z

src/DependencyInjection/HttplugExtension.php

+                $key = $config['name'] ? '.'.$config['name'] : '';
+                $container
+                    ->register($serviceId.$key, LimiterInterface::class)
+                    ->setFactory([new Reference('limiter.'.$config['name']), 'create'])


this does not check if the name is set or not. and if the name is empty, it will use limiter., is that the intention?

@dbu No, it was my mistake, I assume that if the default value is not specified, then it is required by default

dbu · 2024-06-17T09:26:44Z

src/DependencyInjection/HttplugExtension.php

+                    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'] : '';


if name is required, this check is for the special case where name is set to ''? below it would still do strange things when name is ''

and i think symfony configuration will not allow '' on something that is set to be required

Done, check #462

Maxim Solovev added 4 commits June 14, 2024 16:36

add ThrottlePlugin

7cb6fb1

fix tests

fddf99c

fix cs

abc4007

fix tests

b391113

Big-Shark changed the title ~~add ThrottlePlugin~~ draft: add ThrottlePlugin Jun 14, 2024

Maxim Solovev added 5 commits June 14, 2024 21:17

fixes

904108e

fix test

d3fefda

add tests

27b776f

revert test

774c740

fix test

f10afcb

Big-Shark changed the title ~~draft: add ThrottlePlugin~~ add ThrottlePlugin Jun 14, 2024

add additional checks

e9e64bf

dbu reviewed Jun 15, 2024

View reviewed changes

Maxim Solovev added 2 commits June 16, 2024 22:53

add conflic version

8f2f9bf

remove dead code

12e1816

Big-Shark mentioned this pull request Jun 16, 2024

add throttle plugin php-http/documentation#323

Merged

dbu approved these changes Jun 17, 2024

View reviewed changes

dbu merged commit 58223eb into php-http:1.x Jun 17, 2024
14 checks passed

dbu reviewed Jun 17, 2024

View reviewed changes

Big-Shark mentioned this pull request Jun 17, 2024

Throttle plugin small fixes #462

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ThrottlePlugin #460

add ThrottlePlugin #460

Big-Shark commented Jun 14, 2024 •

edited

Loading

Big-Shark commented Jun 14, 2024

dbu left a comment

dbu Jun 15, 2024

Big-Shark Jun 16, 2024

Big-Shark commented Jun 16, 2024 •

edited

Loading

ostrolucky commented Jun 17, 2024

Big-Shark commented Jun 17, 2024

ostrolucky commented Jun 17, 2024

Big-Shark commented Jun 17, 2024

dbu left a comment

Big-Shark commented Jun 17, 2024

dbu commented Jun 17, 2024 •

edited

Loading

dbu Jun 17, 2024 •

edited

Loading

Big-Shark Jun 17, 2024

dbu Jun 17, 2024 •

edited

Loading

Big-Shark Jun 17, 2024

dbu Jun 17, 2024

Big-Shark Jun 17, 2024

dbu Jun 17, 2024

dbu Jun 17, 2024

Big-Shark Jun 17, 2024

add ThrottlePlugin #460

add ThrottlePlugin #460

Conversation

Big-Shark commented Jun 14, 2024 • edited Loading

What's in this PR?

Why?

Example

Big-Shark commented Jun 14, 2024

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Big-Shark commented Jun 16, 2024 • edited Loading

ostrolucky commented Jun 17, 2024

Big-Shark commented Jun 17, 2024

ostrolucky commented Jun 17, 2024

Big-Shark commented Jun 17, 2024

dbu left a comment

Choose a reason for hiding this comment

Big-Shark commented Jun 17, 2024

dbu commented Jun 17, 2024 • edited Loading

dbu Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Big-Shark commented Jun 14, 2024 •

edited

Loading

Big-Shark commented Jun 16, 2024 •

edited

Loading

dbu commented Jun 17, 2024 •

edited

Loading

dbu Jun 17, 2024 •

edited

Loading

dbu Jun 17, 2024 •

edited

Loading