Fix for CHttpCacheFilter's Cache-Control headers being overwritten by started sessions #1628

Merged
merged 6 commits into from Nov 10, 2012

Conversation

Projects
None yet
4 participants
@DaSourcerer
Contributor

DaSourcerer commented Oct 29, 2012

As reported here, there is a problem with the Cache-Control header being overwritten by PHP's session logic. This can result into situations where http caching loses its effects.

This PR sets a header via session_cache_limiter() which will be immediately overwritten. In addition, the Pragma header will be cleared as well. This seems to work well in PHP v5.4.x and according to this comment, it should work with previous versions of PHP too. I'd still appreciate some review by others.

@ghost ghost assigned samdark Oct 29, 2012

@DaSourcerer

This comment has been minimized.

Show comment
Hide comment
@DaSourcerer

DaSourcerer Nov 5, 2012

Contributor

Are there any objections? If not, I'd like to add a line to CHANGELOG now ...

Contributor

DaSourcerer commented Nov 5, 2012

Are there any objections? If not, I'd like to add a line to CHANGELOG now ...

@mdomba

This comment has been minimized.

Show comment
Hide comment
@mdomba

mdomba Nov 5, 2012

Member

as you wrote... this still needs to be tested... "it should work" does not mean it works.

Member

mdomba commented Nov 5, 2012

as you wrote... this still needs to be tested... "it should work" does not mean it works.

@DaSourcerer

This comment has been minimized.

Show comment
Hide comment
@DaSourcerer

DaSourcerer Nov 6, 2012

Contributor

True. I'm a bit worried, though: Except for @samdark assigning himself, this PR hasn't seen any attention in a week. I realize that the issue resolved by this is not the most pressing one, but it clearly impairs the operation of CHttpCacheFilter for logged-in users, so I'd like to see this tested and pulled asap (or at least timely enough to be included in v1.1.13).

I just realized that I've got a box left which is locked at PHP v5.3, so I could actually test this myself. I'll report back soon.

Contributor

DaSourcerer commented Nov 6, 2012

True. I'm a bit worried, though: Except for @samdark assigning himself, this PR hasn't seen any attention in a week. I realize that the issue resolved by this is not the most pressing one, but it clearly impairs the operation of CHttpCacheFilter for logged-in users, so I'd like to see this tested and pulled asap (or at least timely enough to be included in v1.1.13).

I just realized that I've got a box left which is locked at PHP v5.3, so I could actually test this myself. I'll report back soon.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Nov 6, 2012

Member

@DaSourcerer it will be very helpful.

Member

samdark commented Nov 6, 2012

@DaSourcerer it will be very helpful.

@resurtm

This comment has been minimized.

Show comment
Hide comment
@resurtm

resurtm Nov 8, 2012

Contributor

Solution works as expected on 5.2.17. Feel free to request additional tests on 5.2.17.

Contributor

resurtm commented Nov 8, 2012

Solution works as expected on 5.2.17. Feel free to request additional tests on 5.2.17.

@@ -181,6 +181,11 @@ protected function send304Header()
*/
protected function sendCacheControlHeader()
{
+ if(Yii::app()->session->isStarted)

This comment has been minimized.

@resurtm

resurtm Nov 8, 2012

Contributor

Don't forget about indentation.

@resurtm

resurtm Nov 8, 2012

Contributor

Don't forget about indentation.

This comment has been minimized.

@DaSourcerer

DaSourcerer Nov 8, 2012

Contributor

Looks like this diff has been whitespace-damaged while it looked ok in my editor... Thanks for pointing this out.

@DaSourcerer

DaSourcerer Nov 8, 2012

Contributor

Looks like this diff has been whitespace-damaged while it looked ok in my editor... Thanks for pointing this out.

@DaSourcerer

This comment has been minimized.

Show comment
Hide comment
@DaSourcerer

DaSourcerer Nov 8, 2012

Contributor

I can now confirm that this works on PHP v5.3.18 as well.

Contributor

DaSourcerer commented Nov 8, 2012

I can now confirm that this works on PHP v5.3.18 as well.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Nov 9, 2012

Member

Nice. Add a changelog line, update and we will merge it.

Member

samdark commented Nov 9, 2012

Nice. Add a changelog line, update and we will merge it.

@DaSourcerer

This comment has been minimized.

Show comment
Hide comment
@DaSourcerer

DaSourcerer Nov 9, 2012

Contributor

Done with 686480e

Contributor

DaSourcerer commented Nov 9, 2012

Done with 686480e

CHANGELOG
@@ -35,6 +35,7 @@ Version 1.1.13 work in progress
- Bug #1652: Fixed incorrect syntax of CDbSchema::renameTable() for SQLite, added CSqliteSchema::renameTable() method (Sarke)
- Bug: Table schema is refreshed on Gii model generation when schemaCachingDuration is used (SonkoDmitry)
- Bug: CDbCommand::setFetchMode wasn't accepting additional arguments needed for PDO::FETCH_CLASS (samdark)
+- Bug: Active HTTP sessions overwrote the Cache-Control header set by CHttpCacheFilter (DaSourcerer)

This comment has been minimized.

@samdark

samdark Nov 9, 2012

Member

Need issue number here. In this case it's 1628. That's one last thing. Can't do it myself since using iPhone currently :)

@samdark

samdark Nov 9, 2012

Member

Need issue number here. In this case it's 1628. That's one last thing. Can't do it myself since using iPhone currently :)

This comment has been minimized.

@DaSourcerer

DaSourcerer Nov 10, 2012

Contributor

Ah, I thought since #1628 is "just" the PR, I could leave it out. Adding right away ...

@DaSourcerer

DaSourcerer Nov 10, 2012

Contributor

Ah, I thought since #1628 is "just" the PR, I could leave it out. Adding right away ...

samdark added a commit that referenced this pull request Nov 10, 2012

Merge pull request #1628 from DaSourcerer/http-caching
Fix for CHttpCacheFilter's Cache-Control headers being overwritten by started sessions

@samdark samdark merged commit 446bb67 into yiisoft:master Nov 10, 2012

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Nov 10, 2012

Member

Thanks!

Member

samdark commented Nov 10, 2012

Thanks!

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