Configure plugins #37

Nyholm · 2016-01-17T12:35:48Z

This PR fixes #17

httplug: 
  plugins: 
    cache:
      cache_pool: 'acme.cache'
      config: 
        respect_headers: true
    logger:
      formatter: 'acme.formatter'

This make sure that you can configure all our 9 configurable plugins. Some plugins (like RetryPlugin) has only optional configuration. They are enabled by default. They others are disabled by default.

Also, the LoggerPlugin is treated special. We inject the logger service automatically and it is enabled by default. This is done because of ease of use.

Nyholm · 2016-01-17T15:59:08Z

Also, the LoggerPlugin is treated special. We inject the logger service automatically and it is enabled by default. This is done because of ease of use.

I'm having second thoughts about this. It would mean that a Symfony installation that does not have Monolog would get an error like "httplug.plugin.logger has a dependency on a service 'logger'. No such service found".

I guess that it would be less "magic" if we removed it and let the user configure the logger like any other plugin with dependencies.

joelwurtz · 2016-01-17T19:32:55Z

Maybe we can also enabled the stopwatch plugin based on the kernel.debug value by default ?

Nyholm · 2016-01-18T07:26:02Z

Maybe we can also enabled the stopwatch plugin based on the kernel.debug value by default ?

Same problem there as with the logger. What if the stopwatch service does not exist?

Nyholm · 2016-01-18T07:32:09Z

I've updated the PR. The logger is not treated any special

joelwurtz · 2016-01-18T09:04:54Z

Same problem there as with the logger. What if the stopwatch service does not exist?

I don't think this is too much of a problem, we can just throw an exception if service is not available. IMO if someone have desactived the stopwatch and log service in his debug mode / application he will know how to change the configuration :)

sagikazarmark · 2016-01-19T00:02:41Z

What if we only enable them automatically if:

the user haven't enabled it manually
the relevant service is available

Nyholm · 2016-01-19T04:55:10Z

hm, sure. I can give the logging plugin and the stopwatch plugin its dependencies in a compiler pass (if the dependency services exists). That would make sense.

dbu · 2016-01-20T16:52:15Z

DependencyInjection/Configuration.php

+                    ->arrayNode('authentication')
+                    ->canBeEnabled()
+                        ->children()
+                            ->scalarNode('authentication')->isRequired()->cannotBeEmpty()->end()


can we add an info to this? this is the name of the authentication service that implements some php-http interface?

Added info() on all nodes that require a service.

dbu · 2016-01-20T16:58:47Z

cool! i like the idea with the compiler pass. we could say: if explicitly disabled, we remove the service definition in the di class. if explicitly enabled, we somehow tell the compiler pass to explode if stopwatch/logger are not available.

the default behaviour should be that if the requirements are met, we enable those plugin, otherwise we automatically disable them. would need some container parameter i fear. and injecting the optoinal plugins to the client services in the compiler pass. hm. which will lead to priorisation challenges.

Nyholm · 2016-01-21T08:45:47Z

the default behaviour should be that if the requirements are met, we enable those plugin, otherwise we automatically disable them. would need some container parameter i fear. and injecting the optoinal plugins to the client services in the compiler pass. hm. which will lead to priorisation challenges.

I might be able to do it without ana container parameters. I'll give it some thoughts after work.

dbu · 2016-01-21T08:52:26Z

DependencyInjection/HttplugExtension.php

+     * @param ContainerBuilder $container
+     * @param array            $config
+     */
+    protected function configurePlugins(ContainerBuilder $container, array $config)


why not private?

Nyholm · 2016-01-21T19:37:50Z

@dbu mentioned an issue in #37 (comment)

does this make sense? why not let the user define both the journal and the history plugin service and enable the plugin themselves?

the debug toolbar history is auto-injected into every client we know of, when we determine that the toolbar is wanted. what is the use for this plugin? i would have to add it manually, right? i don't see the use of having it in the configuration like this, and would prefer dropping it and explaining in the documentation why its not in there.

i think this would just clutter the configuration with things that won't be used much, if at all.

First of, we do not use httplug.plugin.history for the toolbar. We register our own httplug.collector.history_plugin. The httplug.plugin.* are for the users.

You do highlight the fact that the following two way of using the cache plugins are exactly the same:

Option A)

// config.yml
httplug:
  plugins:
    cache:
      cache_pool: 'cache'
  clients:
    acme:
      factory: 'httplug.factory.guzzle6'
      plugins: ['httplug.plugin.cache']

Option B)

// services.yml
services: 
  my_cache_plugin:
    class: Http\Client\Plugin\CachePlugin
    arguments: ["@cache"]

// config.yml
httplug:
  clients:
    acme:
      factory: 'httplug.factory.guzzle6'
      plugins: ['my_cache_plugin']

B is the generic approach that will always work and is suggested that you do with your custom made plugins.

A makes much more sense for plugins like decoder, retry and redirect because they only have scalar dependencies. In fact, option B would be unnecessarily complicated for such plugins.

I believe that we should allow these two options for configuring all plugins because it would be weird to support option A for only a subset of our plugins.

Nyholm · 2016-01-21T20:08:22Z

DependencyInjection/CompilerPass/PluginPass.php

+        if (!$container->hasDefinition('debug.stopwatch')) {
+            $container->removeDefinition('httplug.plugin.stopwatch');
+        }
+    }


With this compiler pass we make sure that the exception messages makes sense.

Before:
If logger plugin was or wasn't used and logger is not defined you would get an exception like "httplug.plugin.logger" requested an non existing service 'logger'".

Now:
If logger plugin is used and logger is missing you will get an exception like "httplug.client.acme has requested an non existing service 'httplug.plugin.logger'"

hm, i wonder if we could instead detect the problem in the compiler pass and throw the exception "to use the logger plugin, you need to have a logger service configured"

That would require us to know what plugins that are being used. We have currently no clue about that in the compiler pass. It could be solved with container parameters or a DebugPluginClient::getPlugins().

You could also make an argument that the very few users that have logger/stopwatch disabled probably knows about it and are skilled enough to figure out that the LoggerPlugin requires a logger. But now I am making assumptions based on no facts.

dbu · 2016-01-22T13:53:23Z

okay, agree on consistency and allowing to configure all standard plugins.

Nyholm · 2016-01-26T21:49:41Z

With this compiler pass we make sure that the exception messages makes sense.

Before:
If logger plugin was or wasn't used and logger is not defined you would get an exception like "httplug.plugin.logger" requested an non existing service 'logger'".

Now:
If logger plugin is used and logger is missing you will get an exception like "httplug.client.acme has requested an non existing service 'httplug.plugin.logger'"

Im happy to say that I was wrong.
If you do not have a service with id logger and if you are not using or logging plugin no error will show. The logging plugin is private so it will be removed if not used when we build the container.

If, however, I use the logging plugin (still with no logger I will get an exception like this:

[Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException]              
  The service "httplug.client.acme" has a dependency on a non-existent service "logger".

// config.yml
httplug:
  clients:
    acme:
      factory: 'httplug.factory.guzzle6'
      plugins: ['httplug.plugin.logger']

No compiler pass needed =)

dbu · 2016-01-27T07:42:00Z

looks good!
@Nyholm can you please adjust the configuration phpunit test? it does not expect the plugins section of the configuration: https://travis-ci.org/php-http/HttplugBundle/jobs/105014582

Nyholm · 2016-01-29T07:21:38Z

@dbu, I have addressed all the comments and feedback. I think this is ready to merge.

sagikazarmark · 2016-01-29T07:22:47Z

Do you think we can tag a new release after this PR? Either 1.0 or a new MJZ. I don't want to rush with the stable if you think there is more work to be done.

Nyholm · 2016-01-29T07:30:08Z

I do not think this is ready for a stable release yet. There are some changes that we want to do on the issue tracker. Also, I have not been running this in production yet. I want to make sure it is no major bugs and that concept/API/config makes sense when adding more third party bundles.

sagikazarmark · 2016-01-29T07:30:51Z

Okay. Although I already use it in production 😄

Configure plugins

dbu · 2016-01-29T08:22:47Z

thanks a lot!

i created #38 for the release discussion

Nyholm force-pushed the plugin_conf branch from f91061b to ad61c35 Compare January 17, 2016 12:42

Nyholm force-pushed the plugin_conf branch from 81c0f49 to d7b86b4 Compare January 18, 2016 07:31

dbu reviewed Jan 20, 2016
View reviewed changes

dbu reviewed Jan 21, 2016
View reviewed changes

Nyholm force-pushed the plugin_conf branch 2 times, most recently from 4389dfe to 79c85d5 Compare January 21, 2016 19:21

Nyholm force-pushed the plugin_conf branch 2 times, most recently from 515cd74 to 84f5ae4 Compare January 21, 2016 19:52

Nyholm reviewed Jan 21, 2016
View reviewed changes

Nyholm force-pushed the plugin_conf branch 3 times, most recently from 307f2b0 to 7e5816f Compare January 26, 2016 21:54

Nyholm force-pushed the plugin_conf branch from 7e5816f to 60d2a5d Compare January 27, 2016 08:05

Added config for the default plugins

99b1cff

Nyholm force-pushed the plugin_conf branch from 60d2a5d to 99b1cff Compare January 27, 2016 08:06

dbu added a commit that referenced this pull request Jan 29, 2016

Merge pull request #37 from Nyholm/plugin_conf

e25f3d1

Configure plugins

dbu merged commit e25f3d1 into php-http:master Jan 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure plugins #37

Configure plugins #37

Nyholm commented Jan 17, 2016

Nyholm commented Jan 17, 2016

joelwurtz commented Jan 17, 2016

Nyholm commented Jan 18, 2016

Nyholm commented Jan 18, 2016

joelwurtz commented Jan 18, 2016

sagikazarmark commented Jan 19, 2016

Nyholm commented Jan 19, 2016

dbu Jan 20, 2016

Nyholm Jan 21, 2016

dbu commented Jan 20, 2016

Nyholm commented Jan 21, 2016

dbu Jan 21, 2016

Nyholm Jan 21, 2016

Nyholm commented Jan 21, 2016

Nyholm Jan 21, 2016

dbu Jan 22, 2016

Nyholm Jan 22, 2016

dbu commented Jan 22, 2016

Nyholm commented Jan 26, 2016

dbu commented Jan 27, 2016

Nyholm commented Jan 29, 2016

sagikazarmark commented Jan 29, 2016

Nyholm commented Jan 29, 2016

sagikazarmark commented Jan 29, 2016

dbu commented Jan 29, 2016

Configure plugins #37

Configure plugins #37

Conversation

Nyholm commented Jan 17, 2016

Nyholm commented Jan 17, 2016

joelwurtz commented Jan 17, 2016

Nyholm commented Jan 18, 2016

Nyholm commented Jan 18, 2016

joelwurtz commented Jan 18, 2016

sagikazarmark commented Jan 19, 2016

Nyholm commented Jan 19, 2016

dbu Jan 20, 2016

Choose a reason for hiding this comment

Nyholm Jan 21, 2016

Choose a reason for hiding this comment

dbu commented Jan 20, 2016

Nyholm commented Jan 21, 2016

dbu Jan 21, 2016

Choose a reason for hiding this comment

Nyholm Jan 21, 2016

Choose a reason for hiding this comment

Nyholm commented Jan 21, 2016

Nyholm Jan 21, 2016

Choose a reason for hiding this comment

dbu Jan 22, 2016

Choose a reason for hiding this comment

Nyholm Jan 22, 2016

Choose a reason for hiding this comment

dbu commented Jan 22, 2016

Nyholm commented Jan 26, 2016

dbu commented Jan 27, 2016

Nyholm commented Jan 29, 2016

sagikazarmark commented Jan 29, 2016

Nyholm commented Jan 29, 2016

sagikazarmark commented Jan 29, 2016

dbu commented Jan 29, 2016