building the initial bundle #8

dbu · 2015-10-17T17:09:35Z

continued from #6
fix #7

once merged, we can move this repo to the php-http organisation.

dbu · 2015-10-17T17:11:07Z

@sagikazarmark can you review again? i think i adressed all your concerns raised in #6. also doing the rename. what should the main namespace be? currently its Http/HttplugBundle. is that ok?

dbu · 2015-10-17T17:13:05Z

composer.json

+    "require": {
+        "php-http/discovery": "^0.1.1",
+        "php-http/httplug": "^0.1.0",
+        "php-http/httplug-implementation": "^0.1.0",


are these namespaces correct? or what will the virtual package be?

We discussed naming in php-http/httplug#68. I would prefer those two, but I am open to suggestions.

but depending on php-http/psr-client-implementation should be enough, right? if people need a client that has the convenience methods, they can just add that to their dependencies and configure it, but psr-client is kind of the minimum...

i have to wait for our decision on php-http/httplug#68 (comment) until i can adapt this

sagikazarmark · 2015-10-17T19:07:22Z

I see Vendor\Bundle\NamedBundle regularly. Is it a common pattern? If so , what do you think about it?

Regarding naming: I trust in your choice as you have more experience in it.

sagikazarmark · 2015-10-17T19:10:31Z

DependencyInjection/Configuration.php

@@ -0,0 +1,73 @@
+<?php
+
+namespace Http\ClientBundle\DependencyInjection;


Isn't this HttplugBundle?

dbu · 2015-10-18T07:55:19Z

I see Vendor\Bundle\NamedBundle regularly. Is it a common pattern? If so, what do you think about it?

the Bundle bit is optional. what is our vendor name? Http? or PhpHttp? i
looked at the httplug code and there its still Http/Client so i went with
that. do we keep the httplug namespace like that?

sagikazarmark · 2015-10-18T11:21:48Z

Okay, we can skip the Bundle in the namepace.

Honestly, I am not sure about naming. We kept Http\Client for the httplug package. Not sure about that either, but I think the bundle could be HttplugBundle regardless of what the package namespace is.

dbu · 2015-10-19T06:20:30Z

README.md

+
+This bundle provides 3 services: 
+
+* `httplug.client` a service that provides the `Http\Client\HttpClient`


depending on the decision on php-http/httplug#68 (comment) this might need to be changed

and fine as well.

dbu · 2015-10-29T17:44:47Z

updated. will have to check whether i can install it and all that, then would be ready to transfer to the php-http organisation.

also need to see about tests that make sure we are in sync with all parts of php-http. what is the state of the guzzle adapter?

sagikazarmark · 2015-10-30T06:45:04Z

I think guzzle implements the latest-greatest httplug, thanks to @joelwurtz.

sagikazarmark · 2015-10-30T06:49:08Z

Will it be possible to configure multiple clients? Also, can we configure Guzzle Client for example prior to passing it to the adapter? (I guess it has to be done in services somehow)

dbu · 2015-10-30T07:33:48Z

Will it be possible to configure multiple clients?

this is certainly something we will want to make possible, but right now the bundle does not help with that.

Also, can we configure Guzzle Client for example prior to passing it to the adapter? (I guess it has to be done in services somehow)

the bundle does not know about specific adapters. the idea for that use case would be that you simply configure guzzle and the adapter as services in your application and tell the bundle that this service name is the default. with httplug being just the contract, the bundle can not really help with setting up specific clients. where it will really shine however is when we have the plugin system in place. then we can allow to configure pipelines. i also plan to add services for the utils clients (Batch, Methods). but not in this PR, first get the MVP done.

sagikazarmark · 2015-10-30T07:39:12Z

that this service name is the default.

Like that, I was thinking about the same.

the bundle can not really help with setting up specific clients

Actually, I think that specific clients should be configured through their own bundle and then the configured service should be passed to the adapter.

i also plan to add services for the utils clients (Batch, Methods)

👍

joelwurtz · 2015-10-30T08:48:55Z

README.md

+        message_factory: httplug.message_factory.default
+        uri_factory: httplug.uri_factory.default
+    classes:
+        client: ~ # uses discovery if not specified


Hum, maybe we can have a class for each adapter which can inject/send a NodeConfiguration ? So we would now how to configure each adapter.

I think it should be done in service configuration by the user.

the user can specify a class for the client / adapter and we just
instantiate that class without arguments. in a first step, i would
expect the user to define a service for their adapter if they need
specific configuration on their client/adapter.

the next step will be to allow to configure plugins and wrapper clients.
i want to avoid configuring the actual guzzle client, but if the
guzzle adapter has specific configuration, we can add that at some
point. not sure how much specific configuration the adapters will have
however.

not sure how much specific configuration the adapters will have however.

Shouldn't be any at all.

Is it possible to specify adapter classes as services, so we can configure the underlying client?

For example create a service for Guzzle6Adapter where a Guzzle Client instance is being injected which can be cpnfigured as a service as well.

Is it possible to specify adapter classes as services

that seems too complicated to me. either use the factory or simply define a service for the underlying client and another one for the adapter, then configure this bundle to use your service.

i updated the readme to better explain the idea.

simply define a service for the underlying client and another one for the adapter, then configure this bundle to use your service.

That's exactly what I want.

dbu · 2015-11-05T22:01:16Z

i think this is ready for an initial version. note that the bundle does not do much more than providing the autowiring as a symfony service - but with the option to specify a manually created service instead. once plugins are defined, we should work on that. and also provide convenient ways to get the HttpMethodsClient, BatchClient and AsyncClient as services.

any objections if i merge this and move it to the php-http organisation?

sagikazarmark · 2015-11-05T22:27:39Z

Nope, go ahead.

building the initial bundle

dbu reviewed Oct 17, 2015
View reviewed changes

sagikazarmark reviewed Oct 17, 2015
View reviewed changes

dbu mentioned this pull request Oct 19, 2015

naming of the virtual package php-http/httplug#68

Closed

dbu force-pushed the initial-bundle branch from b927d5c to 40d8914 Compare October 19, 2015 06:18

dbu reviewed Oct 19, 2015
View reviewed changes

dbu force-pushed the initial-bundle branch from 40d8914 to b966e71 Compare October 29, 2015 17:43

joelwurtz reviewed Oct 30, 2015
View reviewed changes

dbu force-pushed the initial-bundle branch from b966e71 to 6309884 Compare November 5, 2015 21:53

building the initial bundle

702eda2

dbu force-pushed the initial-bundle branch from 6309884 to 702eda2 Compare November 5, 2015 21:59

dbu added a commit that referenced this pull request Nov 6, 2015

Merge pull request #8 from dbu/initial-bundle

2a388f8

building the initial bundle

dbu merged commit 2a388f8 into master Nov 6, 2015

dbu deleted the initial-bundle branch November 6, 2015 07:49

		@@ -0,0 +1,73 @@
		<?php

		namespace Http\ClientBundle\DependencyInjection;


		This bundle provides 3 services:

		* `httplug.client` a service that provides the `Http\Client\HttpClient`

building the initial bundle #8

building the initial bundle #8

Uh oh!

Conversation

dbu commented Oct 17, 2015

Uh oh!

dbu commented Oct 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu Oct 19, 2015 via email

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Oct 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Oct 18, 2015

Uh oh!

sagikazarmark commented Oct 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Oct 29, 2015

Uh oh!

sagikazarmark commented Oct 30, 2015

Uh oh!

sagikazarmark commented Oct 30, 2015

Uh oh!

dbu commented Oct 30, 2015 via email

Uh oh!

sagikazarmark commented Oct 30, 2015

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!

Choose a reason for hiding this comment

Uh oh!

dbu commented Nov 5, 2015

Uh oh!

sagikazarmark commented Nov 5, 2015

Uh oh!

Uh oh!