HTTP message proposal #72

Merged
merged 6 commits into from Jan 28, 2014

Conversation

Projects
None yet
10 participants
Contributor

thewilkybarkid commented Dec 31, 2012

This follows on from the discussion at https://groups.google.com/forum/?fromgroups=#!topic/php-fig/73cM2qq_uho

It provides basic descriptions for a MessageInterface, RequestInterface and ResponseInterface. These will need expanding, including how to use them.

Contributor

thewilkybarkid commented Dec 31, 2012

I have almost completed a HTTP message package too (including abstract request/response classes). This isn't on GitHub yet.

proposed/http.md
+ *
+ * @return ResponseInterface|null Response, or null if not set.
+ */
+ public function getResponse();
@Ocramius

Ocramius Dec 31, 2012

Contributor

A request may be re-used multiple times, and the response may be different each time. I think this assumption is wrong.

@simensen

simensen Dec 31, 2012

Contributor

Ditto. I mentioned this on the ML.

proposed/http.md
+ *
+ * @return RequestInterface Reference to the request.
+ */
+ public function setResponse(ResponseInterface $response);
@Ocramius

Ocramius Dec 31, 2012

Contributor

Same as above. Not sure why a request would be aware of a response

proposed/http.md
+ /**
+ * {@inheritdoc}
+ *
+ * @return RequestInterface Reference to the request.
@Ocramius

Ocramius Dec 31, 2012

Contributor

Instead of using all these repetitions, you can simply skip those and instead set the return type to self in the parent interface.

proposed/http.md
+ *
+ * @return RequestInterface|null Request, or null if not set.
+ */
+ public function getRequest();
@Ocramius

Ocramius Dec 31, 2012

Contributor

Same as above. Response has no reason to know a request that caused it. It's a response, it bubbles up across layers of your application regardless of where the request got stuck.

@simensen

simensen Dec 31, 2012

Contributor

Ditto. I mentioned this on the ML.

proposed/http.md
+ /**
+ * {@inheritdoc}
+ *
+ * @return ResponseInterface Reference to the response.
@Ocramius

Ocramius Dec 31, 2012

Contributor

Same as above with repetitions. Fix the return type in the parent interfaces by using @return self

Contributor

thewilkybarkid commented Jan 2, 2013

I've just committed the package to https://github.com/thewilkybarkid/psr-http. It uses the proposal as it currently is, but as it obviously will change I'll try to keep it as up to date as possible (large changes might take a while to do!).

marijn commented Jan 3, 2013

I really hate the fact that these interfaces promote mutability. I would argue they should be modeled as value objects...

Contributor

simensen commented Jan 3, 2013

@marijn I have similar feelings and thought it would be nice to treat request and response as value objects. However, the idea was put forth that for observers/plugins and such it would make sense for requests and responses to be able to be changed.

Simensen: I think the idea of creating mutable vs immutable requests is really complicated and would make it hard to add functionality to an HTTP client implemented with this proposed interface. I also think clients, observers, or whatever else people choose to implement on top of these PSRs should be able to modify a request or response as needed (take a look at Guzzle's plugin system for examples). — @mtdowling

marijn commented Jan 3, 2013

I also think clients, observers, or whatever else people choose to implement on top of these PSRs should be able to modify a request or response as needed

Which is exactly my point, it is no longer the same request and should not be treated as such.

Contributor

Ocramius commented Jan 3, 2013

@simensen even if it's hard, the interface can give a direction on that by just defining getters for example... That would be enough to allow implementation of immutables

Contributor

simensen commented Jan 3, 2013

@Ocramius Don't get me wrong, I'm a big fan of the idea and proposed how I'd like to see the interfaces laid out on the mailing list. :) Since it was brought up here independently I wanted to share the concerns @mtdowling's expressed on the mailing list about the idea of splitting the interface into read/write interfaces.

Sending a read-only Request instance to something like Guzzle would require Guzzle to transform the entire Request into a native internal writable Request (whether it be based on PSR HTTP Message interface or not) in order to be able to function correctly since it expects to be able to write to the Request before it has been processed.

Contributor

mtdowling commented Jan 3, 2013

A large number of HTTP clients allow you to change a request pre-flight:

... and then you can start listing a ton of other clients for other languages:

Having mutable and immutable objects would add a large amount of complexity to a HTTP message PSR, cause the final product to be very inflexible, and would not reflect what is currently being used in the real world by developers.

Why is this important? If you really believe it is important, build evidence around it. Provide a list of other popular HTTP libraries that have a distinction between mutable and immutable request/response objects.

+ /**
+ * Sets the response status code.
+ *
+ * @param integer $statusCode Status code.
@staabm

staabm Jan 6, 2013

getStatusCode returns a string,...

@thewilkybarkid

thewilkybarkid Jan 7, 2013

Contributor

Good spot!

+ * Gets the response reason phrase.
+ *
+ * If it has not been explicitly set using `setReasonPhrase()` it SHOULD
+ * return the RFC 2616 recommended reason phrase.
@staabm

staabm Jan 6, 2013

Should this PSR contain a class which hold all http status codes and reason phrases?

@Ocramius

Ocramius Jan 6, 2013

Contributor

Would be VERY useful, but I think it's not really needed, and it would also be hard to have an agreement on it. Interfaces for basic exceptions (4xx, 5xx) may be interesting.

@thewilkybarkid

thewilkybarkid Jan 7, 2013

Contributor

It would be useful to have, but should be in an abstract implementation if anywhere. As for exception interfaces, I think they should be left to implementers if they want them (not sure they add that much, and it would also be very hard to agree on which ones!).

@mtdowling

mtdowling Aug 6, 2013

Contributor

I agree that exceptions should be left to implementations. IMO, there's no need to define them in this PSR.

@bakura10 bakura10 referenced this pull request in aws/aws-sdk-php-zf2 Apr 27, 2013

Closed

S3 signature #5

+ *
+ * @throws InvalidArgumentException When the body is not valid.
+ */
+ public function setBody($body);
@BenMorel

BenMorel May 13, 2013

It think that to become a standard, the interface must support streamable bodies : you can't just expect an object to be castable to string. Imagine you're transferring a 100MB file in the body, it won't fit in memory, so this must accept a stream as well.
An alternative would be to create a MessageBody interface, that would expose methods such as read() etc.

@thewilkybarkid

thewilkybarkid May 14, 2013

Contributor

I don't think stream handling should be enforced here, that would stop adoption by lightweight implementers such as Buzz.

Instead, It would be useful to have a StreamInterface defined in a separate PSR (Guzzle and Drupal, for example, have one of these already). Implementers of this interface can then choose to handle StreamInterface objects differently.

+ public function setHeaders(array $headers);
+
+ /**
+ * Adds headers, replacing those that are already set.
@mtdowling

mtdowling Aug 6, 2013

Contributor

I think that this method shouldn't replace headers that are already set, but instead merge into the existing headers. For example, if there is already a Cache-Control header set to "public", and I call addHeaders(['Cache-Control' => 'max-age=2592000']), then the Cache-Control header should become public, max-age=2592000.

+ * MUST be converted to comma-separated strings; the ordering MUST be
+ * maintained.
+ *
+ * Null values will remove existing headers.
@mtdowling

mtdowling Aug 6, 2013

Contributor

Why is this in the specification?

+ *
+ * @param string $header Header name.
+ *
+ * @return string|null Header value, or null if not set.
@mtdowling

mtdowling Aug 6, 2013

Contributor

Should this mention that the return value can be a string or an object that implements __toString()?

@BenMorel

BenMorel Aug 6, 2013

IMO string is fine, it implicitly means that whatever the implementation chooses to return, it must be castable as a string.

+
+namespace Psr\Http;
+
+use Psr\Http\Exception\InvalidArgumentException;
@mtdowling

mtdowling Aug 6, 2013

Contributor

I don't see the value in adding a marker type interface for an InvalidArgumentException. I've errantly done this in the past myself, and I now see that this sort of exception marking has no value when an existing standard library exception provides the exact same semantic meaning. (See: http://my.safaribooksonline.com/book/programming/java/9780137150021/exceptions/ch09lev1sec4)

@willdurand

willdurand Aug 23, 2013

One benefit is to be able to filter/sort exceptions for a given component.

+ *
+ * The array keys are the header name, the values the header value.
+ *
+ * @return array Headers.
@mtdowling

mtdowling Aug 6, 2013

Contributor

How would someone use a simple key/value pair array that is returned from this function when HTTP headers are meant to be case-insensitive? When using a simple array, you either have multiple array keys of the same name with different casing (e.g. "Foo", "foo") or you have to put something like "use the lowercase name of the header to retrieve a header from the array". I think more thought need to be put into the return value of this method in order for this method to be used consistently across different implementations.

Guzzle returns an object called HeaderCollection that implements \ArrayAccess, \IteratorAggregate, and Countable. This header collection is basically a case insensitive dictionary that returns objects implementing HeaderInterface. HeaderInterface is a class implements __toString, has a "name" property, and allows multiple headers values to be specified. When cast to a string, the header values are imploded on the header's "glue" variable.

@willdurand

willdurand Aug 23, 2013

In Symfony, your HeaderCollection is called a HeaderBag. I think we should rely on such object instead of an array.

+ *
+ * @return bool If the header is present.
+ */
+ public function hasHeader($header);
@willdurand

willdurand Aug 23, 2013

We could expose the headers as an object (see above) only, so that this method and the one right below would not be part of this interface (Demeter's Law etc.)

@Ocramius

Ocramius Aug 25, 2013

Contributor

+1

Even without the headers object, checking if the header exists is trivial through getHeaders()

+ * A request message from a client to a server.
+ */
+interface RequestInterface extends MessageInterface
+{
@willdurand

willdurand Aug 23, 2013

Maybe this interface could declare some constants, such as the HTTP verbs.

+ *
+ * @throws InvalidArgumentException When part of the header set is not valid.
+ */
+ public function addHeaders(array $headers);
@Ocramius

Ocramius Aug 25, 2013

Contributor

As @mtdowling has described above, this method seems to enforce too much behaviour that could live in either a headers container. setHeader and addHeader would be better at this.

@mtdowling

mtdowling Aug 29, 2013

Contributor

Agreed. This method could be removed IMO.

+ *
+ * @see getBodyAsString()
+ */
+ public function getBody();
@Ocramius

Ocramius Aug 25, 2013

Contributor

Not sure why the body of an HTTP message would be anything different from a string. In HTTP, it's always a string, the fact that it's json encoded or url encoded is a different matter that can be handled in getDecodedBody or something similar.

@mtdowling

mtdowling Aug 25, 2013

Contributor

Streaming HTTP responses is very important when downloading really large files (e.g., Amazon S3 objects that are gigabytes). By forcing it into a string, you are forcing it into memory. I don't think there should be a getBodyAsString() method though.

Looking back at the various setters in the interface, I'm wondering if they should be there at all. This goes back to my idea of allowing immutable implementations of the various message objects. I don't see why setters would need to be implemented by libraries adopting such a PSR.

Setters are actually pretty important in an HTTP message PSR. Consumers should be able to modify requests pre-flight.

Was there some kind of consensus on this?

No. I made this comment when this was first brought up, but nobody responded:
#72 (comment)

@BenMorel

BenMorel Aug 26, 2013

I agree there should not be a getBodyAsString() method. IMO, we should force getBody() to return a BodyInterface, which among others would offer the __toString() method for those who want to transparently use getBody() as if it returned a string. This interface could also offer methods to stream the output.

@ghost

ghost Aug 26, 2013

A few Actions That has a Laminator
Buy Fake Fendi Handbag http://www.mcexplorer.com/Fendi-Handbags-For-Sale-Replica/

@willdurand

willdurand Aug 26, 2013

Please, Keep It Simple.. The body of a HTTP request is a string, getBody() should return a string value, period.

There is no need for a BodyInterface or whatever.

@thewilkybarkid

thewilkybarkid Aug 26, 2013

Contributor

I think it should just be what was given to setBody() (or during construction), so by default it can be either a string/stringable object, but it could be something else if the implementation allows it (native stream or whatever). That should be flexible but simple.

@Ocramius

Ocramius Aug 26, 2013

Contributor

@mtdowling has a point though. What happens if you got large streamable contents?

@BenMorel

BenMorel Aug 26, 2013

@sebpot It's up to the implementation, small HTTP messages could return new StringBody($string); while an Amazon S3 response could return new StreamBody($stream); for example. The whole purpose of the interface is to be able to access it in a unified manner, without having to wonder about the actual type (and do dirty things such as if (is_string($body)) {} else if(is_resource($body)) {}).
Note that when I suggest an interface, it could actually be a plain class as well, such as MessageBody. We could provide several factory methods, such as MessageBody::fromString() and MessageBody::fromStream().

@willdurand Did you read @mtdowling's comment above? It would be just impossible to use this interface for large objects such as the ones returned by Amazon S3. But if as I suggest, getBody() returns a class/interface that declares __toString(), then you'll be free, as a consumer of the library, to use it just as if it returned a plain string.

@thewilkybarkid Unfortunately, that would lead to dirty conditionals to deal with the body, as I mentioned above.

@ghost

ghost Aug 26, 2013

Hello, Neat post. There is a problem along with your website in internet explorer, may test this? IE still is the marketplace leader and a huge portion of folks will miss your fantastic writing because of this problem.
[url=http://www.eurocontrol.es/newsletters/consultoria/catalogo/calzoncillos-de-calvin-klein-baratos.htm]Calzoncillos De Calvin Klein Baratos[/url]

@willdurand

willdurand Aug 26, 2013

Ah no, I missed last @mtdowling's comment.

Contributor

Ocramius commented Aug 25, 2013

Looking back at the various setters in the interface, I'm wondering if they should be there at all.

This goes back to my idea of allowing immutable implementations of the various message objects.

Was there some kind of consensus on this? I don't see why setters would need to be implemented by libraries adopting such a PSR. Assuming that I wanted to provide a new message and pass it to another library, I could do something like (pseudo):

$originalMessage = $someOtherEndpoint->gimmeMessage();

$myMessage = new MyMessage();
$myMessage->setWhatever($originalMessage);

$someOtherEndpoint->sendMessage($myMessage);

As long as getter implementation is respected, the $someOtherEndpoint should be able to understand $myMessage.

Contributor

thewilkybarkid commented Aug 26, 2013

@Ocramius At certain points I think immutable messages make sense (eg the user's request/response for a sub-request in a controller), but they usually need to go through layers (eg cache) where they can be modified. As @mtdowling said, I don't know of any library/framework that actually has immutable request/response objects.

Contributor

Ocramius commented Aug 26, 2013

@thewilkybarkid the fact that most libs take the "mutable" way doesn't mean that interfaces should be extended to setters.

@ghost

ghost commented Aug 26, 2013

I spared a thought that would be better to ask you here before pulling the request.

Just wrote some interfaces and need your opinion before doing with this anything else. Look at sebpot/Http and tell me if there's any sense in proposing this package here as PSR, and if so, what corrections should I do.

Contributor

mtdowling commented Jan 8, 2014

I've submitted a new PR that is a (mostly rewritten) fork of this PR with changes that I feel are necessary to provide a meaningful PSR for HTTP messages.

#244

@philsturgeon philsturgeon merged commit 7d93894 into php-fig:master Jan 28, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment