php-http / HttplugBundle

Symfony Framework Integration for HTTPlug
http://httplug.io
MIT License
381 stars 50 forks source link

add ThrottlePlugin #460

Closed Big-Shark closed 3 months ago

Big-Shark commented 3 months ago
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 commented 3 months ago

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

Big-Shark commented 3 months ago

@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 image

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 commented 3 months ago

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

Big-Shark commented 3 months ago

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

ostrolucky commented 3 months ago

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

Big-Shark commented 3 months ago

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

Big-Shark commented 3 months ago

@dbu thanks, please, create new release

dbu commented 3 months ago

on it, on it ;-)

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

and merging to 2.x in https://github.com/php-http/HttplugBundle/pull/461 (i added the test right away)