Allow to configure plugins specifically for a client #112

dbu · 2016-08-07T10:50:07Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	fixes #110
Documentation	php-http/documentation#142
License	MIT

What's in this PR?

Add new configuration for plugins specifically to a client. The global plugin configuration is duplicated to allow configuring all plugins with client specific information.

Why?

See #110

Example Usage

httplug:
    plugins:
        logger:
            enabled: true
            logger: 'logger'
            formatter: null
        redirect:
            enabled: true
            preserve_header: true
            use_default_for_multiple: true
        retry:
            enabled: true
            retry: 1
    clients:
        todo:
            factory: "httplug.factory.guzzle6"
            plugins:
                - 'httplug.plugin.logger'
                - 'httplug.plugin.redirect'
                -  add_host:
                     host: http://localhost
                - 'httplug.plugin.retry'
        acme:
            factory: 'httplug.factory.guzzle6'
            plugins: ['httplug.plugin.redirect', 'httplug.plugin.retry']

You can specify the httplug.client.{name}.plugin.add_host in the plugins list if you want the host plugin to come somewhere else than last in the chain.

Checklist

dbu · 2016-08-07T10:54:03Z

DependencyInjection/Configuration.php

+            $decoder->canBeEnabled();
+        } else {
+            $decoder->canBeDisabled();
+        }


this construct is repeated quite a bit, but i can't think of a better solution. the global plugins need to be enabled by default, but on the client, we want to only see plugins that where explicitly configured.

dbu · 2016-08-07T11:14:45Z

DependencyInjection/HttplugExtension.php

@@ -145,7 +158,9 @@ private function configurePluginByName($name, Definition $definition, array $con
                $definition->replaceArgument(0, new Reference($config['cookie_jar']));
                break;
            case 'decoder':
-                $definition->addArgument($config['use_content_encoding']);
+                $definition->addArgument([
+                    'use_content_encoding' => $config['use_content_encoding'],


separated the bugfix into #113 and then realized we have other changes in master already, so we won't do a patch release for the fix. but i guess after we merge this we want to tag 1.3 very soon so the bugfix will be released soon.

sagikazarmark · 2016-08-07T12:23:33Z

How can you control the order of plugins?

Maybe we can have this within the plugins section: numeric index: look for service, string index: configure extra plugin.

dbu · 2016-08-07T13:02:30Z

without further information, extra_plugins are appended to the explicitly configured plugins in the order they have been defined. but you can specify each of the extra_plugin in the plugins field of the client. i will document how to know the service name of the extra plugins.

dbu · 2016-08-07T13:08:27Z

Maybe we can have this within the plugins section: numeric index: look for service, string index: configure extra plugin.

ah, now i see what you mean. if we want to mix, i would rather say that inside client.{name}.plugins you can have string (= service id) or extra plugin configuration. this will make it easier to handle the ordering. the drawback would be that the configuration format is different from shared plugins, we would need to add a type field. or we have something like this: ``` plugins: logger: ... client: acme: plugins: - add_host: host: ... - httplug.plugin.logger ``` i think this last option is probably the most user friendly. (but i am scared how to express that in the configuration definition :-( )

Nyholm · 2016-08-08T06:41:52Z

I like this approach:

plugins:
    logger:
        ...
client:
    acme:
        plugins:
            - add_host:
                  host: ...
            - httplug.plugin.logger

It makes more sense. But yes, the configuration.php is going to be difficult. But we should try.

dbu · 2016-08-11T06:57:37Z

CHANGELOG.md

@@ -9,6 +9,7 @@

 ### Added

+- You can now configure plugins on each client in `extra_plugins`. Plugins that you previously had to define your own services can be configured on the client.


needs update

dbu · 2016-08-11T09:57:44Z

DependencyInjection/Configuration.php

-                            ->scalarNode('retry')->defaultValue(1)->end()
-                        ->end()
-                    ->end() // End retry plugin
+                // TODO add aditional plugins that are only usable on a specific client


what are the plugins we should allow to configure?

HeaderSet, HeaderRemove, HeaderDefaults, HeaderAppend, ContentLength

ContentLength is globally available and has no configuration options. did you mean something else?

No, my bad.

Nyholm · 2016-08-12T08:01:27Z

The plugin_journal expects a single dimensional array at https://github.com/php-http/HttplugBundle/pull/112/files#diff-18f3f26a96b967c0a5a3e13086d8367dR256

(Try to open the web profiler page and you will see an error)

Nyholm · 2016-08-12T08:07:22Z

DependencyInjection/HttplugExtension.php

        $this->configureClients($container, $config);
+        $this->configureSharedPlugins($container, $config['plugins']); // must be after clients, as the extra_plugins in clients might use plugins as template that will be removed


Update this comment. We cant use (extra_plugins)

Nyholm · 2016-08-12T08:19:05Z

I've reviewed this. It is a really good improvement. I like the way you add the shared configuration on each client in the Configuration.php.

I made a few comments, It was a bug but otherwise it looks fine.

For the record. Here is a working configuration (we do not use extra_plugins as the first comment says):

httplug:
    plugins:
        logger:
            enabled: true
            logger: 'logger'
            formatter: null
        redirect:
            enabled: true
            preserve_header: true
            use_default_for_multiple: true
        retry:
            enabled: true
            retry: 1
    clients:
        todo:
            factory: "httplug.factory.guzzle6"
            plugins:
                - 'httplug.plugin.logger'
                - 'httplug.plugin.redirect'
                -  add_host:
                     host: http://localhost
                - 'httplug.plugin.retry'
        acme:
            factory: 'httplug.factory.guzzle6'
            plugins: ['httplug.plugin.redirect', 'httplug.plugin.retry']

dbu · 2016-08-12T09:31:55Z

adding a new test in #118 to prevent the regression you spotted.

dbu · 2016-08-12T11:56:01Z

DependencyInjection/Configuration.php

            ->arrayNode('clients')
+                ->fixXmlConfig('plugin')


this must go after the ->prototype('array') array line to apply to the things inside the array and not to the array itself (which has numerical keys)

dbu · 2016-08-12T13:04:20Z

yay, this seems to actually work out! trying to get the doc started, then lets merge this.

dbu · 2016-08-12T13:53:45Z

this is now ready. i quickly tried in a symfony app and it works now fine, including debug toolbar.

Nyholm · 2016-08-12T14:15:08Z

I've started tested it a little. Found a unrelated bug... (#121)

I'll will try to do a final review tonight if you are happy with the current state of this PR.

dbu · 2016-08-12T14:17:12Z

great! yes i am happy with it. squashed the commits and it does look right to me. but am sure there are things to improve and i might have missed something.

Nyholm · 2016-08-12T18:27:37Z

DependencyInjection/Configuration.php

+                    ->arrayNode('plugins')
+                        ->info('A list of plugin service ids and client specific plugin definitions. The order is important.')
+                        ->prototype('array')
+        ;


You do not call ->end() here and I know that it is on purpose. You should at least add a comment here. Or event better: Create the plugin node and then add it to the tree. (I think that is possible)

Nyholm · 2016-08-12T18:48:05Z

Im happy to merge when all my comments have been addressed or answered.

Good job 👍

dbu · 2016-08-15T07:13:34Z

did some more cleanup based on your feedback, @Nyholm

any more suggestions or is this good?

Nyholm · 2016-08-15T08:07:55Z

Im all good. I'm very happy with this PR. It is an amazing improvement. Thank you

sagikazarmark added the in progress label Aug 7, 2016

dbu force-pushed the client-extra-plugins branch 2 times, most recently from 6c98e84 to 70e599e Compare August 7, 2016 10:52

dbu reviewed Aug 7, 2016
View reviewed changes

dbu mentioned this pull request Aug 7, 2016

fix plugin instantiation #113

Merged

1 task

dbu reviewed Aug 7, 2016
View reviewed changes

dbu mentioned this pull request Aug 7, 2016

Add support for default_host and force_host options on each client #111

Closed

2 tasks

dbu force-pushed the client-extra-plugins branch from 110f452 to 64a3ebb Compare August 7, 2016 13:00

dbu changed the title ~~Allow to configure plugins specifically for a client~~ [WIP] Allow to configure plugins specifically for a client Aug 7, 2016

dbu force-pushed the client-extra-plugins branch from 64a3ebb to e101865 Compare August 8, 2016 08:11

dbu reviewed Aug 11, 2016
View reviewed changes

dbu force-pushed the client-extra-plugins branch from deb2ba2 to 9a1e3a1 Compare August 11, 2016 09:55

dbu reviewed Aug 11, 2016
View reviewed changes

dbu force-pushed the client-extra-plugins branch from 9a1e3a1 to 50a90f9 Compare August 11, 2016 09:59

Nyholm reviewed Aug 12, 2016
View reviewed changes

dbu mentioned this pull request Aug 12, 2016

test that journal is set up correctly #118

Merged

dbu force-pushed the client-extra-plugins branch from 134fc36 to deb4eb3 Compare August 12, 2016 09:49

dbu reviewed Aug 12, 2016
View reviewed changes

dbu changed the title ~~[WIP] Allow to configure plugins specifically for a client~~ Allow to configure plugins specifically for a client Aug 12, 2016

dbu mentioned this pull request Aug 12, 2016

document configuring plugins on client php-http/documentation#142

Merged

dbu force-pushed the client-extra-plugins branch from ac21608 to c6058e8 Compare August 12, 2016 14:01

Nyholm reviewed Aug 12, 2016
View reviewed changes

dbu force-pushed the client-extra-plugins branch from c6058e8 to 80122ec Compare August 15, 2016 06:52

Allow to configure plugins specifically for a client

11eeafc

dbu force-pushed the client-extra-plugins branch from 80122ec to 11eeafc Compare August 15, 2016 06:55

Nyholm merged commit 925fa8e into master Aug 15, 2016

sagikazarmark removed the in progress label Aug 15, 2016

Nyholm deleted the client-extra-plugins branch August 15, 2016 08:08

sagikazarmark modified the milestone: v1.3.0 Aug 16, 2016

		@@ -9,6 +9,7 @@

		### Added

		- You can now configure plugins on each client in `extra_plugins`. Plugins that you previously had to define your own services can be configured on the client.

		$this->configureClients($container, $config);
		$this->configureSharedPlugins($container, $config['plugins']); // must be after clients, as the extra_plugins in clients might use plugins as template that will be removed

Allow to configure plugins specifically for a client #112

Allow to configure plugins specifically for a client #112

Uh oh!

Conversation

dbu commented Aug 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this PR?

Why?

Example Usage

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Aug 7, 2016

Uh oh!

dbu commented Aug 7, 2016

Uh oh!

dbu commented Aug 7, 2016 via email

Uh oh!

Nyholm commented Aug 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Aug 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Aug 12, 2016

Uh oh!

dbu commented Aug 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Aug 12, 2016

Uh oh!

dbu commented Aug 12, 2016

Uh oh!

Nyholm commented Aug 12, 2016

Uh oh!

dbu commented Aug 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Aug 12, 2016

Uh oh!

dbu commented Aug 15, 2016

Uh oh!

Nyholm commented Aug 15, 2016

Uh oh!

Uh oh!

dbu commented Aug 7, 2016 •

edited

Loading