php-http/httplugbundle

Symfony Framework Integration for HTTPlug

httplugbundle's Issues

The scope of this bundle?

I'm curious what your intention of this bundle is. I know it has been a while since this was updated and your focus are on the libraries. At the moment this bundle creates services for the client and factories. It means that it is only good for dependency injection. Is it any other wins atm?

I had an idea of actually be implementation specific here. The bundle config could allow you to configure clients and make sure to create services for a configured client. Example:

httplug:
  clients:
    acme: 
      adapter: guzzle6
      timeout: 2
      verify_ssl: false

services:
  linkedin:
    class: Happyr\LinkedIn\LinkedIn
    arguments: ["@httplug.acme"]

What do you think? Would you accept a pr implementing this?

fix symfony 3 build and lowest version build

in #11 we started to ignore some build errors. it seems the main problem is dependencies of puli. one option is to drop php-http/discovery and just document it (see php-http/discovery#33). the other is once puli is changed to not need the cli in the dependencies.

MVP

Initial goal:

• php_http.client => alias to something providing Http\Adapter\HttpAdapter (note that the adapter and client interfaces are supposed to be merged
• php_http.message_factory => alias to something providing Http\Message\MessageFactory
• php_http.uri_factory => alias to something providing Http\Message\UriFactory

If no configuration is given, this bundle provides services using the ::find as factory methods. This is the zero-config scenario. those could be named php_http.client.default and so on. the default service could be used with a service decorator when several clients are needed.

Alternatively, the user can specify the client (adapter) class / factory class in which case a service instantiating that class directly is created. This would replace the factory service definition and be php_http.client.default as well.

A third option is to specify the service explicitly, allowing to configure clients with custom initialization. In that case, the php_http.client.default can still exist (e.g. as a base for service decorator) but the explicit service is aliased.

Add support for clients in client-common

For example methods client

Configure default plugins

There are some plugins in php-http/plugins that got dependencies. We should allow the user to configure those plugins.

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

Reference: #16 (comment)

Icon under 2.7

Fix yaml indentation in README

Captured body length cannot be 0

Q	A
Bug?	yes
New Feature?	no
Version	1.2.x

Actual Behavior

toolbar.captured_body_length cannot be set to 0 manually

Possible Solutions

Remove ->cannotBeEmpty() from configuration. Maybe also use integerNode instead of scalarNode. Is that a BC break?

Rename toolbar config to profiling

Q	A
Bug?	no
New Feature?	yes
Version	2.0.0

Actual Behavior

Currently we have the config called toolbar.

Expected Behavior

profiling seems to be a common standard to me. See symfony and DoctrineBundle.

This could be done supporting BC in the next minor, then remove toolbar in the next major.

Remove the Wiki

We do not use it..

Add configuration for autentication

I would like to configure different clients with authentication.

httplug:
  clients:
    guzzle5:
      factory: 'httplug.factory.guzzle5'
      plugins: ['httplug.plugin.logger']
    trans_logger:
      factory: 'httplug.factory.guzzle5'
      authentication: 
        type: 'basic'
        username: 'foo'
        password: 'bar'
    microservice2:
      factory: 'httplug.factory.guzzle5'
      plugins: ['httplug.plugin.logger', 'my_cache_plugin']
      authentication: 
        type: 'wsse'
        username: 'baz'
        password: 'biz'

What do you think? Is this something we should support? Do you like the config example?

Create container parameters for plugin classes

Right now if I want to configure a custom plugin service, I need to use it's classname. Wouldn't it be better to have all the plugin classnames as parameters in the container?

Remove Puli dependency

The new discovery should be used, although I expect some conflicts.

/cc @Nyholm

Documentation: Use for Reusable Bundles

The current documentation says:

Use for Reusable Bundles
Rather than code against specific HTTP clients, you want to use the Httplug Client interface. To avoid building your own infrastructure to define services for the client, simply require: php-http/httplug-bundle in your bundles composer.json. You SHOULD provide configuration for each of your services that needs an HTTP client to specify the service to use, defaulting to httplug.client. This way, the default case needs no additional configuration for your users.
The only steps they need is require one of the adapter implementations in their projects composer.json and instantiating the HttplugBundle in their kernel.

Is this really the best practice? If a third party bundle define httplug.client in their services.yml it will be very difficult to change the client used. I've done like this time to time but it does not feel 100% perfect. It is a lot to write for the third party bundle but it works perfectly.

Can I get some feedback? Does it exists any other options?

Add support for string curl client params

Maybe we could accept curl client constants as well. Before passing to the client, we could resolve the constant names to actual values. What do you think?

There's not always a Stack on the Collector

Q	A
Bug?	yes
New Feature?	no
Version	1.4.0

In some cases the call to getCurrentStack() on the collector can return false. https://github.com/php-http/HttplugBundle/blob/master/Collector/Collector.php#L67-L70
This happens when there has not been any Stack added to the Collector and end() only returns false

This results in a break when it's not taken in consideration here: https://github.com/php-http/HttplugBundle/blob/master/Collector/ProfilePlugin.php#L61

Update discovery dependency

An incompatibility issue has been fixed in 0.6.4

Support configuration for UrlMatcher

.. or we should at least document how you use the UrlMatcher with the bundle. I want to write something here: http://php-translation.readthedocs.io/en/latest/guides/configure-httplug.html#configure-caching

All request to Google API should be cached but no others.

missing for stable release

lets collect what remains to be done and discuss here whether things must be in 1.0.0 or not. lets create issues for concrete tasks and reference them, and put them into https://github.com/php-http/HttplugBundle/milestones/1.0.0 if we think we need them.

Enable StyleCI

Do we need a dependency injection tag?

When I use this bundle I starting to convert services that were previously dependent on Guzzle or that used the auto discovery. With this bundle I can inject the httplug.client and httplug.message_factory. I tend to write services that looks like this:

<?php

namespace Acme\CoreBundle\Service;

use Http\Client\HttpClient;
use Http\Message\MessageFactory;

/** 
 * A service that send data to the statistics server
 */
class StatisticsClient
{
    /**
     * @var string
     */
    private $apiUrl;

    /**
     * @var HttpClient
     */
    private $client;

    /**
     * @var MessageFactory
     */
    private $messageFactory;

    /**
     *
     * @param string $apiUrl
     * @param HttpClient $client
     * @param MessageFactory $messageFactory
     */
    public function __construct(HttpClient $client, MessageFactory $messageFactory, $apiUrl)
    {
        $this->apiUrl = $apiUrl;
        $this->client = $client;
        $this->messageFactory = $messageFactory;
    }

    /**
     * @param array  $data
     */
    public function post(array $data)
    {
        // do some logic on $data

        $request = $this->messageFactory->createRequest('POST', $this->apiUrl, [], json_encode($data));
        $this->client->sendRequest($request);
    }
}

All of them depends on httplug.client and httplug.message_factory. Im will add a dependency injection tag and a trait in my application to easily do a setter injection on services like this.
Question: Is that something that should live in the bundle?

Suggestion on tag name: httplug
Traits: ClientAccessorTrait, MessageFactoryAccessorTrait, StreamFactoryAccessorTrait, UrlFactoryAccessorTrait. All of them have a getter and setter for a private property.

EDIT:

Or maybe just one trait and one tag that will inject the httplug.client and httplug.message_factory only. Please correct me if I'm wrong but those will be the most used services.
Btw, this suggestion is all about developer experience.

Guzzle 6 Factory does not respect our defaults for a Guzzle client

https://github.com/php-http/guzzle6-adapter/blob/master/src/Client.php#L30

investigate what other things we could port from IvoryHttpAdapterBundle

https://github.com/egeloen/IvoryHttpAdapterBundle

Improve HttplugExtension

Q	A
Bug?	yes
New Feature?	no
Version	1.2.x

Actual Behavior

The extension became quite unstable with the 1.2.0 release. Profiling toolbar is not so well separated (enabled in production mode, etc). This has to be improved.

detailed reporting in toolbar

Right now, we add the reporting at the begin of the plugin chain. This means we don't see the exact request as it was sent to the server, and we dont see whether a request is answered from cache or not, and we will miss redirects.

We could use a second plugin at the end of the chain, and correlate the requests to determine: request going to actual HTTP client (and thus to real server) / request leading to redirect / request served from cache.

See also https://github.com/php-http/HttplugBundle/pull/19/files#r49300763

Another thing is that we should identify which client was used, in the case where there is more than one client that reports requests. (follow up of #36)

Update link in error message

We introduced an error message in #55 if PuliBundle was not installed. We should update the link so it points directly to the heading in our documentation that explains how to set up the bundle with and without discovery.

Support multiple logger channels

Example config with maintaining BC:

httplug:
    plugins:
        logger:
            logger: logger_service_id
            my_logger: ~
            my_other_logger: service_id
            my_another_logger:
                logger: ~

WDYT?

DataCollector for the symfony toolbar

see also https://github.com/egeloen/IvoryHttpAdapterBundle/blob/master/DataCollector/IvoryHttpAdapterDataCollector.php and https://github.com/misd-service-development/guzzle-bundle/blob/master/DataCollector/GuzzleDataCollector.php

i think this will also need to use some tags to discover additional clients that are defined as service.

Register on packagist

It seems this package is not registered on packagist.

WebProfiler broken when there was a redirect.

Q	A
Bug?	yes
New Feature?	no
Version	ec48ab0

Actual Behavior

When you are using the redirect plugin and there was a redirect we get a longer request stack and that will break the the profiler page.

Steps to Reproduce

Configure a client with some plugins and use the Redirect plugin in the middle of the chain. Then make a call where the server responds with a redirect. Now inspect the request in the web profiler.

Event listener to register adapter/factory with discovery very early

It would make sense to have a request listener that comes very early and registers the adapter or factory as specified. This would be a workaround for libraries that solely depend on discovery and have no injection to override the discovery.

stop having separate functional config for hhvm?

We are using a separate config for testing with hhvm because puli did not work with hhvm. But we don't use puli in the bundle tests anymore, so i think we could get rid of that, right?

Introduce behaviours

Q	A
Bug?	no
New Feature?	yes

At the moment we support flexible client and methods client via options of the client definition with simple flags set to true. However this makes it harder to nest these client behaviors (even if it might not make too much sense at the moment). Instead of these flags, I propose the following:

httplug:
    client:
        acme:
            factory: 'acme_client_factory'
            behaviors:
                - flexible
                - something_else
                - batch

This would allow to nest multiple behavioral clients into each other. Of course we have to take care of type safety (behavioral clients might be sync or async clients (or both), but we need to check that before each nesting to see what the next behavior supports)

This would also ensure futureproof flexibility.

deprecate old guzzle bundles

talk to the maintainers of the tons of guzzle bundles and see if they can contribute to this bundle instead and deprecate their bundles.

Remove discovery dependency, rely on puli directly

Discovery is a static, convenience wrapper around Puli discovery. Instead of using discovery as factories for services, we should use Puli discovery service to find resources at compile time (if that's possible at all).

This would have two side-effects:

• If puli discovery is used at compile time, then container compilation will fail if no resource is found (message factory, etc). IMO this is good, because the earlier we know something is bad the better it is
• We won't get the same NotFound exception

WDYT?

Configure plugins clients with plugins

Can I get some feedback on this approach to configure the plugins?

my_redirect_plugin:
  class: Http\Client\Plugin\RedirectPlugin 
my_logger_plugin:
  class: Http\Client\Plugin\LoggerPlugin 
  arguments: ["@monolog"]
my_cache_plugin:
  class: Http\Client\Plugin\CachePlugin 
  arguments: ["@cache", "@stream_factory"]

httplug:
  clients: 
    my_guzzle5: 
      factory: 'httplug.factory.guzzle5'
      plugins: ['@my_logger_plugin', '@my_cache_plugin']
      config:
        defaults:
          verify_ssl: false
          timeout: 4
    acme: 
      factory: 'httplug.factory.guzzle6'
      plugins: ['@my_logger_plugin', '@my_redirect_plugin']
      config:
        base_uri: 'http://google.se/'

This would allow you to use your own plugins, set the order etc. It would also be easy to inject a JournalPlugin to use internally for the debug toolbar.

We could go with an approach where we name the plugins etc etc but it is a simplier implementation and easier to use (no magic) if we reference them as a service.

discovery not working leads to test error

should be fixed by puli/issues#168 and puli/issues#172

Httplug

Shouldn't this be renamed to httplug? Also, I think you could move it to the organization.

Todos before stable

• Make PluginClientFactory final
• Fix PluginClientFactory constructr docblock (array of Plugins)
• Update changelog
• Update README title: "Symfony integration for HTTPlug"

investigate hhvm build failure

investigate why hhvm builds fail and fix them. we started ignoring the fail in #11

Use cUrl formatter

Q	A
Bug?	no
New Feature?	yes
Version

We should implement the curl formatter on the profiler page.

Show plugins created without the Symfony config

Q	A
Bug?	no
New Feature?	question
Version

I just tested the bundle together with the knp github client. It is creating a PluginClient and adds a lot of extra plugins. I was (foolishly) expecting to see all plugin steps in the WebProfiler.

Is it some way we could make this happen?... Or is it anyway one could configure the knp GitHub client differently?

New toolbar icon

The icon that is used in the web-toolbar is the exact same as the Ajax toolbar. We should create our own icon.svg.

Maybe with straight arrows going back and froth instead of bent? Or maybe a white version of our logo?

Integrate with Symfony Debug Timeline

Q	A
Bug?	no
New Feature?	yes
Version	n/a

Looking at @csarrazi's Guzzle Bundle they have a really nice integration with the Symfony Debug timeline. That is something we should support as well.

Update design on WebProfiler

Q	A
Bug?	yes
New Feature?	yes
Version	n/a

Our WebProfiler has just been updated with much more information and new design. But there is room for improvements. If we look at @florianpreusner's Guzzle Bundle we see some nice screenshots of how it could look like.

configure base path for a client

Q	A
Bug?	no
New Feature?	yes

Actual Behavior

To configure a client with a default host, we can use the AddHostPlugin. However, this plugin can not be configured within HttplugBundle as far as i can see. I ended up doing this:

parameters:
    server: "http://localhost:8001"

services:
    app.http.todo_hostname_uri:
        class: Psr\Http\Message\UriInterface
        factory: ["@httplug.uri_factory", createUri]
        arguments: ["%server%"]
    app.http.plugin.todo_hostname:
        class: Http\Client\Common\Plugin\AddHostPlugin
        arguments: ["@app.http.todo_hostname_uri"]

httplug:
    clients:
        todo:
            factory: "httplug.factory.guzzle6"
            plugins: ["app.http.plugin.todo_hostname", "httplug.plugin.error"]

Possible Solutions

Not sure, but it should be easier than this imo. AddHostPlugin typehints UriInterface, and afaik we can't use a factory for an argument, hence we need to define a service for the uri object.

We could add a configuration in the clients.{name} space for default_host and force_host but i am not sure how well this scales with other options. maybe an options subsection?

Put sources in src/

To be consistent with other php-http repositories I would suggest to put the source files in src/ and tests in tests/. It is already 11 files in the repository root and only one file belongs to the bundle.

This is bundle which you might want to treat differently, that's why I ask a questions before sending a PR.

Do you want this change?

Move documentation

Under an integration category in the docs?

I see stability errors for symfony2

Could not find package php-http/httplug-bundle at any version for your minimum-stability (stable). Check the package spelling or your minimum-stability

my composer.json

{
    "name": "root/client",
    "license": "proprietary",
    "type": "project",
    "autoload": {
        "psr-4": {
            "": "src/"
        }
    },
    "require-dev":{
        "php-http/client-implementation": "^1.0"
    },
    "require": {
        "php": ">=5.3.9",
        "symfony/symfony": "2.7.*",
        "doctrine/orm": "^2.4.8",
        "doctrine/doctrine-bundle": "~1.4",
        "symfony/assetic-bundle": "~2.3",
        "symfony/swiftmailer-bundle": "~2.3",
        "symfony/monolog-bundle": "~2.4",
        "sensio/distribution-bundle": "~4.0",
        "sensio/framework-extra-bundle": "^3.0.2",
        "incenteev/composer-parameter-handler": "~2.0",
        "psr/http-message": "^1.0",
        "phpspec/phpspec": "^2.3",
        "henrikbjorn/phpspec-code-coverage": "^1.0",
        "php-http/httplug": "^0.1.0",
        "php-http/guzzle6-adapter": "^1.0"
    },
    "require-dev": {
        "sensio/generator-bundle": "~2.3"
    },
    "scripts": {
        "post-install-cmd": [
            "Incenteev\\ParameterHandler\\ScriptHandler::buildParameters",
            "Sensio\\Bundle\\DistributionBundle\\Composer\\ScriptHandler::buildBootstrap",
            "Sensio\\Bundle\\DistributionBundle\\Composer\\ScriptHandler::clearCache",
            "Sensio\\Bundle\\DistributionBundle\\Composer\\ScriptHandler::installAssets",
            "Sensio\\Bundle\\DistributionBundle\\Composer\\ScriptHandler::installRequirementsFile",
            "Sensio\\Bundle\\DistributionBundle\\Composer\\ScriptHandler::prepareDeploymentTarget"
        ],
        "post-update-cmd": [
            "Incenteev\\ParameterHandler\\ScriptHandler::buildParameters",
            "Sensio\\Bundle\\DistributionBundle\\Composer\\ScriptHandler::buildBootstrap",
            "Sensio\\Bundle\\DistributionBundle\\Composer\\ScriptHandler::clearCache",
            "Sensio\\Bundle\\DistributionBundle\\Composer\\ScriptHandler::installAssets",
            "Sensio\\Bundle\\DistributionBundle\\Composer\\ScriptHandler::installRequirementsFile",
            "Sensio\\Bundle\\DistributionBundle\\Composer\\ScriptHandler::prepareDeploymentTarget"
        ]
    },
    "config": {
        "bin-dir": "bin"
    },
    "extra": {
        "symfony-app-dir": "app",
        "symfony-web-dir": "web",
        "symfony-assets-install": "relative",
        "incenteev-parameters": {
            "file": "app/config/parameters.yml"
        }
    }
}

Interface suffix

I know it's kind of Symfony standard, but does it really make sense?