Make sure you can configure clients and register them as services

[Nyholm]

This will implement the feature mentioned in #10.

This will let the user configure HttpClients for a specific implementation. Everything specific to a client, say guzzle5, is in one class.

At the moment there is no validation of the special config. We just say: "The config you specify here is sent unparsed to Guzzle".

[dbu]

what should happen if this does not work out? i would rather throw an exception than allow returning null

[Nyholm]

Done. We throw a ClientWasNotCreated exception.

[sagikazarmark]

If something, then rather ClientNotFound

[Nyholm]

My intention was to include the scenario where a clients configuration was wrong and we could not create a client because of it.

[sagikazarmark]

Then InvalidConfiguration or something like that, but ClientWasNotCreated does not tell too much about the "WHY" in my opinion.

[Nyholm]

Good point, I'll change to InvalidConfiguration

[dbu]

thanks for the pull request! from the bundle user perspective, this looks very good. for the implementation side, i am a bit unsure as it seems we duplicate discovery in some way. then again, its not very heavy so i could live with that.

and @sagikazarmark had some ideas about using puli as another way to find implementations. mark, what is your opinion on this PR? how would puli fit in here?

[dbu]

we could treat this as a shortcut for a service name and put httplug.client. in front of it. then people could also specify full service names instead and have their own factories.

[dbu]

if this would be the full factory service name instead of a name for the meta-factory, we could get rid of the the service tags and the meta factory that registers factories by short name (aka is a mini-container for httplug factories)

[Nyholm]

To address comments about the generic factory (#11 (comment), #11 (comment), #11 (comment)):

I want to have a generic factory to which you could add instances of a ClientFactoryInterface by simply add a httplug.client_factory tag to the service definition. This means that we do not need a list of adapter name=>factory class. And it makes the bundle easily extendable if someone would like to add their own class.

httplug:
  clients: 
    my_guzzle5: 
      adapter: my_special_client

class SpecialFactory implements ClientFactoryInterface
{
    public function createClient(array $config = array())
    {
        return new SpecialClient($config);
    }

    public function getName()
    {
        return 'my_special_client';
    }
}

<service id="acme.factory.mySpecialFactory" class="Acme\SpecialFactory" public="false">
       <tag name="httplug.client_factory" />
</service>

We could get around using a generic factory and achieve the same goal by doing this in a compiler pass. The drawback of this is we need to store a lot of configuration in the container's parameters. That approach seams also more "magic" and dirty IMO.

[sagikazarmark]

Puli provides a repository and a discovery layer. With puli we can find the installed classes, but so far, I am not sure we can instantiate/configure them automatically. That said, would be good if puli could replace the current discovery layer.

On the other hand, there is an upcoming DefinitionInterface which could work with Puli nicely:
http://www.thecodingmachine.com/interoperable-php-packages-with-definition-interop-and-puli/

Although it is far in the future.

Client Factory is also something I was thinking about (I think we actually had that interface in HTTPlug earlier).

I am also a bit unsure about this. Mainly because I can't really see the difference between creating configuration, passing it to a client factory and creating a client service directly with configuration. You need config and a registered service both ways. What is the advantage then?

[Nyholm]

Mainly because I can't really see the difference between creating configuration, passing it to a client factory and creating a client service directly with configuration. You need config and a registered service both ways. What is the advantage then?

For the Guzzle clients, there is none. You are correct. You can do something like this in the Extension class:

$guzzleClient = $container->register('httplug.guzzle.client', \GuzzleHttp\Client::class);
$guzzleClient->addArgument($config);
$httpClient = $container->register('httplug.guzzle', \Http\Adapter\Guzzle5HttpAdapter::class);
$httpClient->addArgument(new Reference('httplug.guzzle.client'));

But how would you do it for Buzz? Their Browser need a Client, say that we choose their Curl client then we have to configure options with a setOption($option, $value) method. That would be a pain to do in the Extension class. It would also violate the single responsibility principle and the open/close principle. This is where a factory is beneficial.

This implementation is SOLID. We could event put the implementation factories (Guzzle5Factory, Guzzle6Factory and BuzzFactory) in separate packages if we would like to. That would make the Httplug bundle apply with the common reuse principle. But that's a decision we make later.

[dbu]

i think i can agree to have factories that are tailored to the symfony DI use case in this bundle. its small utitlity code and we can configure several clients which seems a good thing, and difficult to do otherwise. also, having the client configuration in symfony config rather than in some puli resource file (if that is even possible) feels more natural for symfony.

i am still unsure about the generic factory, however. tried to explain my idea in #11 (comment)

[Nyholm]

i am still unsure about the generic factory, however. tried to explain my idea in #11 (comment)

You are correct with your comment. The generic factory is not needed if we specify the factory service id instead of the factory name. That solution is easier, I should update. Thank you!

[Nyholm]

There you go.

[dbu]

cool! i think this is now ready. can you squash the commits please?

there seems to be an issue with the configuration test using an outdated namespace for the guzzle adapter (see https://travis-ci.org/php-http/HttplugBundle/jobs/99706320) - can you maybe adjust that as well?

[Nyholm]

Sure, no problem. I squashed the commits.

I've aslo updated the old namespace for Guzzle6 and updated the discovery package to 0.6.0. I still get an error though.

RuntimeException: Puli Factory is not available

[sagikazarmark]

Puli cli must be added at least as development dependency to make tests passing.

[Nyholm]

Puli cli must be added at least as development dependency to make tests passing.

Done. Thank you.

But shouldn't that be added as a dependency for the discovery? Could you use discovery without Puli/cli?

[sagikazarmark]

No, you can't. But you can install it either as composer dependency in your project or the executable in your prefix. It is under discussion if we should make it a dependency in discovery.

[Nyholm]

@sagikazarmark as always: Thank you for a clarifying answer.

[dbu]

i agree that we should change this, but maybe to 3.0 right away. we need to adjust the symfony version for the matrix lines below as well so we still test 2.7, 2.8 and 3.0

[Nyholm]

Should I also use PHP7 as standard?

[dbu]

yeah lets do that!

[dbu]

we seem to run into a conflict with symfony 3 and discovery / puli: https://travis-ci.org/php-http/HttplugBundle/jobs/99844044

and we need to increase the lowest version of discovery that we allow to make the lowest version tests pass again: https://travis-ci.org/php-http/HttplugBundle/jobs/99844042

[sagikazarmark]

Hm, I'll check why puli fails with symfony 3.0.

[Nyholm]

Before merging this: can we make sure that the Guzzle5 Adapter follows the newly accepted naming conventions (Http\Adapter\Guzzle5\Client namespace)

Of course

[Nyholm]

There are more puli related issues.

I think that webmozart/key-value-store as a missing dependency on webmozart/json. Aslo the GeneratedPuliFactory is not compatible with webmozart/key-value-store.

Ping @webmozart

class GeneratedPuliFactory
{
    /**
     * Creates the resource repository.
     *
     * @return ResourceRepository The created resource repository.
     */
    public function createRepository()
    {
        if (!interface_exists('Puli\Repository\Api\ResourceRepository')) {
            throw new RuntimeException('Please install puli/repository to create ResourceRepository instances.');
        }

        $store = new JsonFileStore(__DIR__.'/path-mappings.json', true); // <-- This line
        $repo = new PathMappingRepository($store, __DIR__.'/..');

        return $repo;
    }
// ...
}

class JsonFileStore implements SortableStore, CountableStore
{
    // ---
    public function __construct($path, $flags = 0)
    {
        Assert::string($path, 'The path must be a string. Got: %s');
        Assert::notEmpty($path, 'The path must not be empty.');
        Assert::integer($flags, 'The flags must be an integer. Got: %s'); // <-- $flag will be a boolean
    // ...
    }
}

[sagikazarmark]

Yes, there is an incompatibility between puli versions. Not sure if it is fixed or not, but can't really do anything about it, until a new beta version is tagged.

[webmozart]

If you use the latest beta releases of all the packages (not dev), everything should work. The dependency on webmozart/json is optional, but it probably makes sense to include it in puli/composer-plugin since json is the default backend.

[sagikazarmark]

@webmozart Everything, except Symfony 3, right? (In case of Puli CLI)

[webmozart]

@sagikazarmark Yes exactly. Symfony 3 support was only added in master. Btw. if you need chat support, join https://gitter.im/puli/issues, there's always people hanging around.

[dbu]

this would now need to be 0.6.3.

but we have the same discussion as in FriendsOfSymfony/FOSHttpCache#268 - maybe we should not depend on discovery and explain to add a client and either be explicit or additionally add discovery. and adjust our code to nicely report when discovery is missing and nothing was configured.

okay, but that is a bit out of scope for this PR, if you can upgrade to 0.6.3 for now, that should hopefully fix the build.

[sagikazarmark]

I think our discover layer can just be replaced by puli discovery. The symfony bundle provides a service for it. See how it is used in our discovery.

[Nyholm]

I can't get it to work with Symfony3 and our discovery since it depends on Puli/cli. I think it is okey to allow Symfony3 to fail at this point until the issues discussed are resolved. (The bundle is not even on packagist yet.)

There is also an issue with the lowest stability test. It seams that puli cant find a Http\Client\HttpClient. I am using all the lastest versions of php-http/* and I cant figure out why it fails. Any suggestions?

[dbu]

i am okay to skip symfony 3 until the chain around puli / phar etc is cleaned up. about lowest version: did you open the composer install line on travis-ci to see what exact versions it installed? maybe the difference is visible there?

[sagikazarmark]

There is a warning at the end of composer install command:

Warning: Could not load Puli packages: RuntimeException: Function has already been defined

[Nyholm]

That is a dependency error that is out of scope for this PR and bundle. I guess that will be solved when we can rely on stable versions of Puli. I'm adding the "prefer-lowest" test to allow_failures as well.

[sagikazarmark]

That is a dependency error that is out of scope for this PR and bundle.

Wow, this is the best "I don't know what causes the error" explanation I have ever heard. 😝

[Nyholm]

haha =)

[dbu]

thanks tobias. created #13 and #14 about the build failures.

[Nyholm]

Thank you for merging.