Skip to content

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

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

Closed
wants to merge 1 commit into from

Conversation

dbu
Copy link
Collaborator

@dbu dbu commented Aug 6, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets fixes #110
Documentation TODO (want feedback first)
License MIT

What's in this PR?

Add new configuration options for default_host and force_host for each client.

Why?

See #110

Example Usage

    clients:
        test:
            factory: httplug.factory.guzzle6
            http_methods_client: true
            options:
                default_host: http://localhost

You can specify the httplug.client.{name}.default|force_host_plugin in the plugins list if you dont want the host plugin to come last in the chain.

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
  • Documentation pull request created (if not simply a bugfix)

@dbu
Copy link
Collaborator Author

dbu commented Aug 6, 2016

just tested it in the web summercamp tutorial app and the configuration works as expected.

->addArgument(new Reference($uriService))
->addArgument(['replace' => true])
;
if (!in_array($addHostPlugin, $plugins)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by default, we append the AddHostPlugin to the very end of the chain, but you can explicitly set it in the plugins list. i will explain this in the documentation

@Nyholm
Copy link
Member

Nyholm commented Aug 6, 2016

I do not like the new options key. I get questions like: Why isn't http_methods_client under options? How come we have both options and config?

    clients:
        test:
            factory: httplug.factory.guzzle6
            http_methods_client: true
            options:
                default_host: http://localhost
            config: 
               verify: false

Also, we should support other plugins like HeaderAppend, HeaderDefaults, HeaderRemove and HeaderSet. All these plugins (and AddHost) are very client specific. Ie, you are not likely to have the same host configured in the AddHost plugin for all clients. Compared to the CachePlugin which is very likely that you use for all clients.

Possible solution:
Maybe we should introduce two scopes of plugins. Public and Private? See this config:

httplug:
    plugins: # Public plugins
        cache:
            enabled: true
            cache_pool: 'my_cache_pool'
            stream_factory: 'httplug.stream_factory'
            config:
                default_ttl: 3600
                respect_cache_headers: true
        logger:
            enabled: true
            logger: 'logger'
            formatter: null
        redirect:
            preserve_header: true
            use_default_for_multiple: true
    clients:
        test:
            factory: httplug.factory.guzzle6
            plugins: ['httplug.plugin.logger', 'httplug.plugin.cache']
            http_methods_client: true
            config: 
                verify: false
            extra_plugins:  # Private plugins only for this client
               add_host:
                    host: http://localhost
                    replace: true
               header_append:
                   User-Agent: 'Foobar'

@dbu
Copy link
Collaborator Author

dbu commented Aug 7, 2016

i like that idea a lot! i guess the basic question is how much we want the user to know things like adding the host are plugins, but we already went with making this transparent for the general plugins. for people not familiar with the lib, there will be a tiny confusion why something is a config and something else is an extra plugin, but i think its manageable with doc.

i guess we should also repeat the public plugins in the extra_plugins of a client. so that you can have a different redirect/log/cache/whatever behaviour. and i will keep the behaviour that you can change the order by adding the private plugins in the plugins list of the client.

@dbu
Copy link
Collaborator Author

dbu commented Aug 7, 2016

see #112

@dbu dbu closed this Aug 7, 2016
@dbu dbu deleted the default-host branch August 7, 2016 11:22
@sagikazarmark sagikazarmark modified the milestone: v1.3.0 Aug 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

configure base path for a client
3 participants