[2.0][HttpFoundation] Improved Cache-Control header when no-cache is sent #6037

dlsniper · 2012-11-17T11:00:43Z

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: not sure, should be
Fixes the following tickets: #4681
Todo: check if this actually works
License of the code: MIT
Documentation PR: ~

@pulzarraider or @sstok does this work for you guys? I'm not sure how to test it nor do I have IE as I'm on Ubuntu.

Thanks!

dlsniper · 2012-11-17T11:10:41Z

I'm not sure what Travis wants or how can I test it, any help in running the tests is greatly appreciated. Thanks!

sstok · 2012-11-17T11:18:17Z

Travis failures are external, someone messed-up an upgrade or something.

Seems good to me, not sure if IE8 will work as expected.
http://stackoverflow.com/questions/1038707/cant-display-pdf-from-https-in-ie-8-on-64-bit-vista

But I'm not able to test this currently.

dlsniper · 2012-11-17T11:22:49Z

I've read that as well as : http://support.microsoft.com/kb/234067
MIcrosoft did dropped the ball on this one :)
If anyone with IE8/SSL setup could confirm this, it would be great.

Thanks.

dlsniper · 2012-11-17T11:51:17Z

Seems that using AppCache breaks things after applying this.
I'll come up with a solution for it later on.

[Sat Nov 17 13:41:40 2012] [error] [client 127.0.0.1] PHP Fatal error:  Uncaught exception 'RuntimeException' with message 'The Expires HTTP header is not parseable (-1).' in /var/www/personal/symfony/vendor/symfony/symfony/src/Symfony/Component/HttpFoundation/HeaderBag.php:239
Stack trace:
#0 /var/www/personal/symfony/vendor/symfony/symfony/src/Symfony/Component/HttpFoundation/Response.php(641): Symfony\\Component\\HttpFoundation\\HeaderBag->getDate('Expires')
#1 /var/www/personal/symfony/vendor/symfony/symfony/src/Symfony/Component/HttpFoundation/Response.php(689): Symfony\\Component\\HttpFoundation\\Response->getExpires()
#2 /var/www/personal/symfony/vendor/symfony/symfony/src/Symfony/Component/HttpFoundation/Response.php(747): Symfony\\Component\\HttpFoundation\\Response->getMaxAge()
#3 /var/www/personal/symfony/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/HttpCache/EsiResponseCacheStrategy.php(45): Symfony\\Component\\HttpFoundation\\Response->getTtl()
#4 /var/www/personal/symfony/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php(207): Symfony\\Co in /var/www/personal/symfony/vendor/symfony/symfony/src/Symfony/Component/HttpFoundation/HeaderBag.php on line 23

dlsniper · 2012-11-18T12:04:36Z

I've changed the Expiry time to 1st of January 2000 as -1 doesn't really seem a valid option and it's back far enough in the past.

pborreli · 2012-11-18T12:28:45Z

from http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html

HTTP/1.1 clients and caches MUST treat other invalid date formats, especially including the value "0", as in the past (i.e., "already expired").

To mark a response as "already expired," an origin server sends an Expires date that is equal to the Date header value. (See the rules for expiration calculations in section 13.2.4.)

fabpot · 2012-11-19T13:33:31Z

src/Symfony/Component/HttpFoundation/ResponseHeaderBag.php

+
+                $this->headers['pragma'] = array('no-cache');
+                $this->headerNames['pragma'] = 'Pragma';
+            }


I would have used parent::set('expires', 0) and parent::set('pragma', 'no-cache') instead.

fabpot · 2012-11-19T13:35:19Z

The patch is not enough. If the computed value is not no-cache, then the pragma must be removed, but removing the Expires one can be tricky if it has been set previously. As this is a protocol issue, and as we should do that only for HTTP 1.0, what about moving this logic to the Response::prepare() method?

dlsniper · 2012-11-20T22:55:29Z

I've moved the logic to the Response::prepare() method as indicated, but I'm not sure about the rest of the comment as 'Pragma' is still used under HTTP/1.1.

dlsniper · 2012-11-20T22:58:25Z

Also, while I understand that Request and Response are different 'scopes', I'm not sure why Symfony Standard does this: $this->setProtocolVersion('1.0'); in the constructor of Response and then not change it to the supported/requested by client protocol. I've noticed it while testing with Firefox, I'd always get a HTTP/1.0 response when I'm in fact sending a HTTP/1.1 request. Could this be considered as bug as well?

fabpot · 2012-11-21T08:24:49Z

@dlsniper This has been fixed in 2.1.

fabpot · 2012-11-21T08:25:48Z

src/Symfony/Component/HttpFoundation/HeaderBag.php

@@ -236,6 +236,10 @@ public function getDate($key, \DateTime $default = null)
            return $default;
        }

+        if (-1 == $value || 0 === $value) {


This change should be reverted as it breaks BC. This can be discussed for the master branch though.

With the move of the code that relied on this I'll remove this but indeed we should talk about this in either master or 2.1 as it's not consistent between the values that are sent by the framework and what can sent by the user.

dlsniper · 2012-11-21T20:32:14Z

I'll squash this when you think it's ready to be merged then I'll make a PR for 2.1/master branch if needed.

lsmith77 · 2012-11-25T18:17:20Z

not really sure if this is related but as it might I want to mention liip/LiipImagineBundle#124

fabpot · 2012-12-06T13:27:01Z

src/Symfony/Component/HttpFoundation/HeaderBag.php

@@ -226,7 +226,7 @@ public function remove($key)
     * @param string    $key     The parameter key
     * @param \DateTime $default The default value
     *
-     * @return \DateTime The filtered value
+     * @return null|\DateTime The filtered value


should be reverted

fabpot · 2012-12-06T13:29:20Z

This looks good to me. Can you add some unit tests, rebase, and squash? Thanks a lot.

dlsniper · 2012-12-08T18:25:15Z

Sorry but I'm not sure what to test for exactly, any pointers would be great :)

fabpot · 2012-12-10T12:53:42Z

src/Symfony/Component/HttpFoundation/Response.php

@@ -143,6 +143,14 @@ public function prepare()
        if ($this->headers->has('Transfer-Encoding')) {
            $this->headers->remove('Content-Length');
        }
+
+


You should remove one blank line here.

fabpot · 2012-12-10T12:54:59Z

Writing tests should be easy enough: check that the pragma header is set when the right conditions are met.

fabpot · 2012-12-11T09:30:48Z

@dlsniper I've merged your changes in the 2.1 branch as we don't have enough information in 2.0 to make it work correctly (we don't have access to the Request and so the protocol is always 1.0). Also, I've reverted the changes to the getMaxAge() method as the getExpires() method always returns a DateTime instance.

dlsniper · 2012-12-11T09:40:58Z

@fabpot thanks. Unfortunately my time is a bit limited at the beginning of this week, hence the lack of response from he with regards to the test.

Best regards.

* 2.1: fixed CS fixed CS [Security] fixed path info encoding (closes #6040, closes #5695) [HttpFoundation] added some tests for the previous merge and removed dead code (closes #6037) Improved Cache-Control header when no-cache is sent removed unneeded comment Fix to allow null values in labels array fix date in changelog removed the Travis icon (as this is not stable enough -- many false positive, closes #6186) Revert "merged branch gajdaw/finder_splfileinfo_fpassthu (PR #4751)" (closes #6224) Fixed a typo Fixed: HeaderBag::parseCacheControl() not parsing quoted zero correctly [Form] Fix const inside an anonymous function [Config] Loader::import must return imported data [DoctrineBridge] Fixed caching in DoctrineType when "choices" or "preferred_choices" is passed [Form] Fixed the default value of "format" in DateType to DateType::DEFAULT_FORMAT if "widget" is not "single_text" [HttpFoundation] fixed a small regression Conflicts: src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php

arendjantetteroo · 2012-12-22T18:43:13Z

I'm getting this error : PHP Fatal error: Uncaught exception 'RuntimeException' with message 'The Expires HTTP header is not parseable (-1). since upgrading from 2.1.4 to 2.1.6. Since it's mentioned a few comments up, i'm curious if this patch is responsible. Should i change something in my http cache code to make it work?
My application is used by IE6 and up.

fabpot reviewed Nov 19, 2012
View reviewed changes

fabpot reviewed Nov 21, 2012
View reviewed changes

fabpot reviewed Dec 6, 2012
View reviewed changes

Improved Cache-Control header when no-cache is sent

22c6b2c

fabpot reviewed Dec 10, 2012
View reviewed changes

fabpot closed this in 47dfb9c Dec 11, 2012

arendjantetteroo mentioned this pull request Dec 22, 2012

[HTTPCache] Expires HTTP Header is not parseable(-1) #6469

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2.0][HttpFoundation] Improved Cache-Control header when no-cache is sent #6037

[2.0][HttpFoundation] Improved Cache-Control header when no-cache is sent #6037

dlsniper commented Nov 17, 2012

dlsniper commented Nov 17, 2012

sstok commented Nov 17, 2012

dlsniper commented Nov 17, 2012

dlsniper commented Nov 17, 2012

dlsniper commented Nov 18, 2012

pborreli commented Nov 18, 2012

fabpot Nov 19, 2012

fabpot commented Nov 19, 2012

dlsniper commented Nov 20, 2012

dlsniper commented Nov 20, 2012

fabpot commented Nov 21, 2012

fabpot Nov 21, 2012

dlsniper Nov 21, 2012

dlsniper commented Nov 21, 2012

lsmith77 commented Nov 25, 2012

fabpot Dec 6, 2012

fabpot commented Dec 6, 2012

dlsniper commented Dec 8, 2012

fabpot Dec 10, 2012

fabpot commented Dec 10, 2012

fabpot commented Dec 11, 2012

dlsniper commented Dec 11, 2012

arendjantetteroo commented Dec 22, 2012

[2.0][HttpFoundation] Improved Cache-Control header when no-cache is sent #6037

[2.0][HttpFoundation] Improved Cache-Control header when no-cache is sent #6037

Conversation

dlsniper commented Nov 17, 2012

dlsniper commented Nov 17, 2012

sstok commented Nov 17, 2012

dlsniper commented Nov 17, 2012

dlsniper commented Nov 17, 2012

dlsniper commented Nov 18, 2012

pborreli commented Nov 18, 2012

fabpot Nov 19, 2012

Choose a reason for hiding this comment

fabpot commented Nov 19, 2012

dlsniper commented Nov 20, 2012

dlsniper commented Nov 20, 2012

fabpot commented Nov 21, 2012

fabpot Nov 21, 2012

Choose a reason for hiding this comment

dlsniper Nov 21, 2012

Choose a reason for hiding this comment

dlsniper commented Nov 21, 2012

lsmith77 commented Nov 25, 2012

fabpot Dec 6, 2012

Choose a reason for hiding this comment

fabpot commented Dec 6, 2012

dlsniper commented Dec 8, 2012

fabpot Dec 10, 2012

Choose a reason for hiding this comment

fabpot commented Dec 10, 2012

fabpot commented Dec 11, 2012

dlsniper commented Dec 11, 2012

arendjantetteroo commented Dec 22, 2012