Warning header #184

Closed
wants to merge 5 commits into
from

Projects

None yet

2 participants

@thewilkybarkid
Contributor

A HTTP message can have more than one warning; this stores them as a Guzzle\Http\Message\Warning object and allows easy manipulation (similar to the Cache-Control directives).

It also adds a '110 Response is stale' warning to the cache plugin when it serves a stale response.

(This is the first of a few stale response PRs.)

@mtdowling
Member

This is looking good. Can you make sure that it's PSR-2 compliant (maybe run the PHP-CS fixer)?

What do you think about creating a Guzzle\Http\Message\Header namespace? We could also refactor the Cache-Control support to do something similar to what you've done here.

@thewilkybarkid
Contributor

I like the namespace idea; some other headers could potentially become objects (eg cookies) too.

@mtdowling mtdowling commented on the diff Dec 10, 2012
src/Guzzle/Http/Message/AbstractMessage.php
@@ -193,7 +201,11 @@ public function getTokenizedHeader($header, $token = ';')
$data = new Collection();
foreach ($this->getHeader($header) as $singleValue) {
- foreach (explode($token, $singleValue) as $kvp) {
+ preg_match_all('#,?(?!$)(?<match>([^' . preg_quote($token) . '"]?("[^"]*")?)+)#', $singleValue, $matches);
@mtdowling
mtdowling Dec 10, 2012 Member

Why is this better than just exploding on the token? When would this actually be needed in practice?

@thewilkybarkid
thewilkybarkid Dec 10, 2012 Contributor

In the event that a warning contains a date, explode() wouldn't parse the date correctly (eg for 110 test "Response is stale" "Mon, 03 Dec 2012 09:15:53 GMT", 111 test "Revalidation failed" the first comma isn't a delimiter).

That said, after rereading the spec I'm not sure that date is ever actually of any use, so it might be better to actually ignore it if given (and if the date's different to the response's date, strip the warning completely).

@mtdowling
Member

Where all will the warning header be used? This PR is adding a lot of methods to the MessageInterface. I actually wish I had thought of a better way to handle the CacheControl stuff than how it is currently implemented.

Did you know that Guzzle\Http\Message\Header can hold multiple values? Does that remove some of the need for this PR?

$request->addHeader('Warning', 'foo')->addHeader('Warning', 'Baz');
foreach ($request->getHeader('Warning') as $value) {
    echo $value . ' ';
}
@thewilkybarkid
Contributor

The cache plugin must add the 110 warning when it serves a stale response (RFC2616§14.9.3); I would also like to add support for RFC5861's stale-if-error directive which could result in the 111 warning be added. Full-blown caches like Squid and Varnish obviously already add these. In my Symfony2 GuzzleBundle I would then add support for it to display the warnings in the profiler. This PR provides a way to easily add and recognise specific warnings (without having to do it manually).

Both this and the Cache-Control header could be refactored into being an object property (cache-control would need different implementations for requests and responses, while the warnings would be a collection). So you could do something like:

$response->cacheControl->setMaxAge(6000);
$response->warnings->add(110);
@mtdowling
Member

I'm working on some good changes that will go out in the an upcoming 3.2.0 release. I'm evaluating how we can have specialized header classes for things like Cache-Control, Set-Cookie, Cookie, etc. I imagine that they would extend from Guzzle\Http\Message\Header and be instantiated by some kind of header factory. What are you thoughts on this? Is there a consensus on the PSR discussion on how this should be handled?

@thewilkybarkid
Contributor

Not yet unfortunately. I suggested a few options, but there hasn't been much talk despite a few prompts. Please do take a look and comment, I'm keen to keep it going as it could be pretty powerful.

(I see there's been some Guzzle URL tweaks recently, take a look at the URI proposal too!)

@mtdowling
Member

Guzzle 3.6 will have the ability to create custom Header classes under the Guzzle\Http\Message\Header\ namespace: https://github.com/guzzle/guzzle/tree/master/src/Guzzle/Http/Message/Header. Do you think this warning header code can be added there?

@mtdowling mtdowling commented on the diff Jun 1, 2013
src/Guzzle/Http/Message/AbstractMessage.php
@@ -2,8 +2,11 @@
@mtdowling
mtdowling Jun 1, 2013 Member

It looks like all changes made in this file should be reverted. With the release of Guzzle 3.6, we are no longer adding custom header methods to message objects, but rather to specific header classes (e.g. Guzzle\Http\Message\Header\Warning).

@mtdowling mtdowling commented on the diff Jun 1, 2013
src/Guzzle/Http/Message/Header/Header.php
@@ -1,6 +1,6 @@
<?php
@mtdowling
mtdowling Jun 1, 2013 Member

This should be reverted as well. I decided not to move header to from Guzzle\Http\Message so as not to break anyone. This could be moved in Guzzle 4.0.

@mtdowling mtdowling commented on the diff Jun 1, 2013
src/Guzzle/Http/Message/Header/HeaderComparison.php
@@ -1,6 +1,6 @@
<?php
@mtdowling
mtdowling Jun 1, 2013 Member

Also should revert this. I moved HeaderComparison to a test namespace as this is only ever used for testing, and probably nobody uses it but me.

@mtdowling mtdowling commented on the diff Jun 1, 2013
src/Guzzle/Http/Message/MessageInterface.php
@@ -2,6 +2,7 @@
namespace Guzzle\Http\Message;
@mtdowling
mtdowling Jun 1, 2013 Member

I believe all changes here should be reverted

@mtdowling mtdowling commented on the diff Jun 1, 2013
src/Guzzle/Http/Message/Response.php
@@ -697,17 +697,6 @@ public function getVia()
}
/**
- * Get the Warning HTTP header
@mtdowling
mtdowling Jun 1, 2013 Member

All header helpers should return string values. If you need a header object, you must use $request->getHeader('Warning');

@mtdowling mtdowling commented on the diff Jun 1, 2013
src/Guzzle/Http/Message/Header/Warning.php
@@ -0,0 +1,152 @@
+<?php
+
+namespace Guzzle\Http\Message\Header;
+
+use DateTime;
+
+/**
+ * HTTP message warning.
+ */
+class Warning
@mtdowling
mtdowling Jun 1, 2013 Member

This should now extend from Guzzle\Http\Message\Header

@mtdowling
Member

I'm going to go ahead and close this because this is so out of date. I'm guess that if you were to update this, you'd probably almost need to start over.

@mtdowling mtdowling closed this Jun 2, 2013
@thewilkybarkid
Contributor

I was thinking the same!

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