[WIP] Allow override of request cache headers #150
Conversation
|
Hmm, it seems the test failures are what we expect. It seems you have removed your messages from #99. What exact error message do you get when running the tests locally? |
|
@ddeboer I had to install php with pear, after that it worked. I moved to phpspec long time ago and my phpunit skills have rusted |
|
Okay, good to hear! Does that mean you can continue working on tests for this PR? |
|
I've modified existing tests to reflect new config changes.
Any other tests that I'm missing? |
| 'private' => true, | ||
| 'public' => true, | ||
| 'private' => true, | ||
| 'public' => true, |
There was a problem hiding this comment.
Please don't align =>. It does not add much value, and it is the perfect way to have coding standards enforcing merge conflicts more frequently by requiring whitespace changes on unrelated lines
|
this looks good, i agree with the idea and the implementation (except for the config details) can you please document this feature in the relevant documentation sections? doc is in this repo under Resources/doc, and online at http://foshttpcachebundle.readthedocs.org/ |
| ) { | ||
| $response->headers->addCacheControlDirective($flag); | ||
| if (!empty($controls[$key])) { | ||
| if (!$response->headers->hasCacheControlDirective($flag) || $overwrite) { |
There was a problem hiding this comment.
this is a code style question really, but i would prefer to write it as
if (!empty($controls[$key])
&& ($overwrite || !$response->headers->hasCacheControlDirective($flag))
) {
no need for two nested if statements. having $overwrite first as its way cheaper to check than the method call.
|
sorry for not responding. i was not aware that you pushed more commits. best write a comment after pushing, telling me to have a look if its good now ;-) |
|
PR updated, I'm going to check documentation now |
|
cool. i think it just needs 1 or 2 places to mention in the doc, where a user would be glad to learn about this option. |
|
doc updated! |
|
i did some cleanups and created a new PR as #151 - please provide feedback there |
This PR corresponds to feature #99.
Any comments welcome (I know, tests are missing..)