Closed joelwurtz closed 8 years ago
Looks good to me.
One comment: Plugin seems to be a bit inconsistent with the other namespaces, which are mostly plural (in case of a collection instead of one, separated component).
So Http\Plugins then ?
Sounds logical to me :smile:
pinging client / adapters maintainers since it concerns them a lot @mekras @shulard @Nyholm @ddeboer WDYT ?
:+1:
Hello!
For me your proposal is a lot cleaner than the actual one! :+1:
Le 19 déc. 2015 à 21:14, Joel Wurtz notifications@github.com a écrit :
pinging client / adapters maintainers since it concerns them a lot @mekras @shulard @Nyholm @ddeboer WDYT ?
— Reply to this email directly or view it on GitHub.
I spent hours writing this comment, so it might be that some of my thoughts got mingled. If you take something out of this, let it be two things: we have too many repos, and these are really nicely formatted tables :D.
Repo | Description | Namespace | Scope |
---|---|---|---|
authentication |
Authenticate PSR-7 requests | Http\Authentication |
Message |
client-tools |
Tools for HTTPlug client implementors | Http\Client\Tools |
Client |
cookie |
Simple Cookie container logic for HTTP Requests | Http\Cookie |
Message |
curl-client |
cURL client | Http\Curl |
Client |
discovery |
Finds installed adapters and message factories | Http\Discovery |
Adapter/Message |
encoding |
PSR 7 Stream for transfer / content encoding headers (chunked, deflate, compress, ...) | Http\Encoding |
Message |
guzzle5-adapter |
Guzzle 5 HTTP adapter | Http\Adapter |
Adapter |
guzzle6-adapter |
Guzzle 6 HTTP adapter | Http\Adapter |
Adapter |
helper |
Helper classes for HTTP related data | Http\Helper |
Message |
httplug |
HTTPlug, the HTTP client abstraction for PHP | Http\Client |
Client |
message-decorator |
Decorators for PSR-7 HTTP Messages | Http\Message |
Message |
message-factory |
Factory interfaces for PSR-7 HTTP Message | Http\Message |
Message |
mock-adapter |
Mock HTTP Adapter for testing | Http\Adapter |
Adapter |
plugins |
Plugin client, decorate http client with middlewares | Http\Client\Plugin |
Client/Message |
promise |
Promise used for asynchronous HTTP requests | Http\Promise |
message |
psr7-vcr |
Not even started yet (mea culpa :)) | (none) | Client |
react-adapter |
(no description) | Http\Adapter |
Adapter |
socket-client |
Socket client | Http\Socket |
Client |
utils |
HTTP Client Utilities | Http\Client\Utils |
Client/Message |
With this categorization in mind, I would like to suggest the following changes.
Overall: From php-http/utils, I would use just one message factory, either zend-diactoros or guzzle/psr7 - any PSR-7 compliant HTTP Client should be able to work with either of these. Both have no special dependencies, so this should just be a matter of decision.
Suggestion: Merge all repos into one, e.g. php-http/message
.
Reasoning: It could of course be that I want to create messages with the factory, but I don't need authentication or a special stream, but the single components don't disturb each other. At the moment, if I wanted to create an authenticated HTTP Message with Cookies, I would have to require
Repo(s) | Current Namespace(s) | New Namespace |
---|---|---|
authentication |
Http\Authentication |
Http\Message\Request\Authentication |
cookie |
Http\Cookie |
Http\Message\Request\Cookie |
encoding |
Http\Encoding |
Http\Message\Stream |
helper |
Http\Helper |
Http\Message\Helper Http\Message\Helper\Extractor Http\Message\Helper\Normalizer Http\Message\Helper\Parser |
message-decorator message-factory utils (parts) |
Http\Message |
Http\Message\Factory Http\Message\Factory\Decorator Http\Message\Factory\MessageFactory Http\Message\Factory\StreamFactory Http\Message\Factory\UriFactory |
promise client-tools (parts) |
Http\Promise |
Http\Message\Promise Http\Message\Promise\FulfilledPromise Http\Message\Promise\RejectedPromise |
Repo | Current Namespace | New Namespace |
---|---|---|
httplug |
Http\Client |
Http\Client |
batch-client (extracted from utils ) |
N/A | Http\Client Http\Client\BatchClient (class) |
client-tools |
Http\Client |
Http\Client\Tools |
curl-client |
Http\Curl |
Http\Client Http\Client\CurlClient (class)(shouldn't this be an adapter?) |
plugins |
Http\Client\Plugin |
Http\Client\Plugin |
socket-client |
Http\Socket |
Http\Client Http\Client\SocketClient (class)(shouldn't this be an adapter?) |
vcr-client /psr7-vcr |
N/A | Http\Client Http\Client\VcrClient (class) |
Repo | Current Namespace | New Namespace |
---|---|---|
curl-adapter ? |
N/A | Http\Adapter\Curl |
guzzle5-adapter |
Http\Adapter |
Http\Adapter\Guzzle5 |
guzzle6-adapter |
Http\Adapter |
Http\Adapter\Guzzle6 |
mock-adapter |
Http\Adapter |
Http\Adapter\Mock |
react-adapter |
Http\Adapter |
Http\Adapter\React |
socket-adapter ? |
N/A | Http\Adapter\Socket |
Wow @jeromegamez I cannot find words to describe this. :smile: :+1:
The fact that we have too many packages came up a few times already. There are upsides and downsides of both the single repo and the multi repo solution.
but the single components don't disturb each other.
This is true, but I would at least partially modify this statement: I would exclude contracts and limit to implementations.
Contracts are probably better to keep somewhat separated so that you can require them without implementations, which is useful when you want to implement your custom stuff.
I would use just one message factory, either zend-diactoros or guzzle/psr7
I am not sure I understand this. Should we support only one? That's not really interoperable.
shouldn't this be an adapter?
Well, as you originally proposed in April when we started the whole thing, we are actually working on an HTTP Client abstraction, not an apdater one. So when we implement something new, then it is actually a client. When we implement a wrapper something already existing, then it's an adapter.
To tell the truth, the most useful proposal from your comment is the message thing for me. Message related stuff is really spread across many repositories. With the following modifications, I think that we should actually start working on php-http/message:
Honestly, we really have many components which can be categorized from this point of view (used with messages) and I haven't thought about them before. Good idea @jeromegamez
I would to always use singular instead of plural (so Plugin and Adapter instead of Plugins and Adapters).Utils is the only namespace I would keep in plural.
Not sure if we should merge encoding into a message package as it relies on guzzle psr7.
Sorry, I'm writing from my mobile, so please excuse my brevity :)
I don't think relying on guzzle/psr7 is a bad thing - it's a lib without dependencies and could be easily exchanged if someday something "better" comes up.
For me it's the same argument as with the message factory: we have the psr/http-message specification, and as long as the generated messages/streams are PSR-7 compliant, it shouldn't matter which lib we use to create them - each client/adapter can handle PSR-7 compliant Message-/Stream-Objects. If they are created by guzzle/psr7 or zend-diactoros or any other factory, shouldn't matter in the end. Please tell me if I'm wrong!
Here is an argument:
https://github.com/satooshi/php-coveralls/issues/80#issuecomment-165829065
I think it is the basic idea behind adapters, and should be for messages as well that you can use your own. So if you already use something in your project, why would you need to install another?
Good point
@xabbuh can you explain why you would use singular? it feels ok to me and would require less changes, which is a plus. but are there specific reasons for it?
If the namespace were plural, I would expect each and every class from that namespace to a plugin, an Adapter and so on. While this might be true for Adapters right now a plugin usually contains lots more classes than just a single entry point class.
I would expect each and every class from that namespace to a plugin, an Adapter and so on. While this might be true for Adapters right now
Not quite: the guzzle6-adapter already contains an extra class: the wrapper (adapter?) around Guzzle promises. The fact that we (sometimes) have multiple classes per adapter makes me agree with @joelwurtz’s proposal to have a namespace per adapter, but I prefer singular too, so: Http\Adapter\Guzzle6Adapter
.
Re the amount of packages: I’m not too happy with the tools and utils names; what, exactly, is the difference between the two?
Not quite: the guzzle6-adapter already contains an extra class: the wrapper (adapter?) around Guzzle promises.
Well, this then supports my reasoning for using a singular Adapter
namespace even more. :)
Re the amount of packages: I’m not too happy with the tools and utils names; what, exactly, is the difference between the two?
Actually it is under discussion to merge the two under a better name: php-http/httplug#99
The difference between them: Utils holds utilities for reusable library implementors, class-tools holds utilities for client implementors.
Okay, based on community feedback, it seems that singular namespace names are more welcome. Based on this and #99 I would like to summarize the current package reorganization plan:
authentication + message-factory-implementations + message-decorator + encoding = message utils + client-tools = client-common
Namespaces:
Http\Client
for HTTPlug and Client implementations: curl, socket, mock
Http\Adapter
for adapters: Guzzle 5,6, React
Http\Message
for message-factory contract and php-http/message
Http\Client\Common
for common client stuff: batch client, methods client, client decorators and emulators (merged utils and client-tools)
Did I mis(understood) something?
(Not as nice as @jeromegamez's :wink: )
I like it! :+1:
Some questions left :
Http\Adapter\Guzzle5
/ Http\Adapter\Guzzle6
/ ... ?+1 for the subnamespaces.
What should the client classes be named in this case?
Http\Adapter\Guzzle6\Client
Http\Adapter\Guzzle6\GuzzleClient
Http\Adapter\Guzzle6\Promise
Http\Adapter\Guzzle6\Client +1 Http\Adapter\Guzzle6\GuzzleClient -1 Http\Adapter\Guzzle6\Promise +1
Well, same class name would actually make things easier when you just replace the implementation with another: rewrite the namespace import, but don't change the code.
how do you mean? if i got this correctly i am -1: if things depend on order of autoload or explode if for some reason i have more than one client/adapter in my project (e g partial migration) this is not nice. and its vety implicit
I think what @sagikazarmark means is that in the following code I would maybe just have to replace use Http\Adapter\Guzzle6\Client
with use Http\Adapter\Buzz\Client
when I want to switch the implementation being used in my project:
use Http\Adapter\Guzzle6\Client;
$client = new Client(...);
// ...
@xabbuh exactly.
By the way, I don't think that this is really relevant (constructor arguments can differ or people just use a DI container). However, I like Client
as the name as it is short, self-explanatory and it will be consistent throughout the different implementations.
The only annoying thing with Http\Adapter\Guzzle6\Client
is that Client
is more common in projects than Guzzle6Client
so IDE auto-completion will not work as well. Still, I prefer the simple Client
for the reasons that @xabbuh mentions.
@xabbuh We usually try to achieve to allow using adapters with zero-config. In that case it might be useful.
@ddeboer it depends on the IDE. For example PHPStorm is smart enough to mitigate this problem.
@sagikazarmark Cool! That's even better then.
ah, got it. yeah Client as class name is fine. people not liking that can always alias to something else in the use statemenz.
Implemented the new namespace setup in our Guzzle6 adapter. Last thing that needs to be decided for the adapters is whether the class name should be Client
or Adapter
, as discussed on the PR.
Ideas:
the issue says namespace, but we talk about the FQN here, right?
my vote goes for Http\Guzzle6Adapter\Client
but i can live with all of
them. having adapter in the namespace and client as class name seems a
good idea to me.
the issue says namespace, but we talk about the FQN here, right?
Yeah, this discussion is kinda for everything now.
So namespaceing then (and FQCNs):
Adapters: Http\SomeAdapter\Client
Clients: Http\SomeClient\Client
+1
I would prefer a schema that follows Http\Adapter\<name>\Client
.
Http\Adapter\Guzzle6\Client
or Http\Adapter\Guzzle6Adapter\Client
Too long and the second is redundant. Why do you prefer this version?
Http\Adapter\Guzzle6\Client looks good to me too. agree that Http\Adapter\Guzzle6Adapter\Client would be redundant.
@sagikazarmark I meant the former one (the latter is indeed redundant). This way all adapters will be under a common namespace.
@sagikazarmark I’m fine with Http\Adapter\Guzzle6\Client
, too. Do you have any objections to it? The only problem I see is the one mentioned here: usually external clients (such as Guzzle) have a class called Client
, which may cause some confusion with our adapter’s (HTTPlug) Client
class. However, I don’t really mind doing:
use GuzzleHttp\Client as GuzzleClient;
use Http\Adapter\Guzzle6\Client;
$guzzleClient = new GuzzleClient(…);
$httplugClient = new Client($guzzle);
Fine by me as well.
i would do this to be explicit:
use GuzzleHttp\Client as GuzzleClient;
use Http\Adapter\Guzzle6\Client as GuzzleAdapter;
I think this is resolved, isn't it?
Yes.
I think react and curl adpater / client need to change their namespace, but maybe better to do an issue directly on the repository.
You are right. Issues are opened.
I would like to propose the following namespace for our different package (goal is to have, in the future, possibility to assemble all package into a single one with subsplit):
Http\Client
(not changed)Http\Promise
(not changed)Http\Message
(not changed)Http\Tools
(Http\Client\Tools
actually)Http\Utils
(Http\Client\Utils actually)Http\Plugin
(Http\Client\Plugin actually)Http\Adapters\[Adapter Name]
, so Guzzle6 will have this namespace for example :Http\Adapters\Guzzle6
(instead ofHttp\Adapter
actually)Http\Clients\[Client Name]
, so Socket will haveHttp\Clients\Socket
Http\Discovery
(not changed)WDYT ?