Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP cache methods #129

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

HTTP cache methods #129

wants to merge 6 commits into from

Conversation

JindrichPilar
Copy link

HTTP cache API added publicCache(), privateCache(), noCache()

publicCache(), privateCache(), sendCacheHeaders() added
noCache() refactored
tests added
@Rican7
Copy link
Member

Rican7 commented Aug 6, 2013

@jinora this is a great pull request! I appreciate you including tests also.

A few things though. I'll comment on the code lines where appropriate. Otherwise, HTTP caching requires many different pieces and directives, as @chriso points out here.

*/

/** Public HTTP cache flag */
const PUBLIC_CACHE = 'public_cache';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new constants should probably just be integers. They're values don't really have much significance, since they're meant to be compared as constants, so following the PHP (Java inspired) convention of simply assigning an integer value would be better suited here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String constant significance lies in debugging. Where string can suggest flags meaning without having to look at constant list. My mistake, I overlooked that DISPATCH constants use integers. Will cahnge to integers.

@chriso
Copy link
Contributor

chriso commented Aug 7, 2013

It looks fine to merge, but whether or not this belongs in this library (a microframework) is debatable.

Up to you @Rican7.

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

Successfully merging this pull request may close these issues.

None yet

3 participants