Http caching fixes #242
Http caching fixes #242
Conversation
- Move etag validation to http transport - If etag validates automatically set the response status to not modified - Calculate the etag based on the request and not the response content
|
@ercanozkaya Can you review my last changes too. Small issue in the header parsing. Instead of solving it in the http message in a generic way i'm solving it more locally. |
| $result = $cache_control['max-age']; | ||
| $values = explode(',', $this->_headers->get('Cache-Control', null)); | ||
|
|
||
| foreach($values as $key => $value) |
There was a problem hiding this comment.
@johanjanssens should we refactor this code to a getCacheControl method?
| $cache_control = (array) $this->_headers->get('Cache-Control', null, false); | ||
| if (isset($cache_control['max-age'])) { | ||
| $result = $cache_control['max-age']; | ||
| $values = explode(',', $this->_headers->get('Cache-Control', null)); |
There was a problem hiding this comment.
@johanjanssens I'm confused about something. Cacheable behaviors sets the header value as an array. When we come here we parse it as a string. Should we support them both?
This reverts commit a9b2976.
…l header information as array
| */ | ||
| public function getCacheControl() | ||
| { | ||
| $values = $this->_headers->get('Cache-Control', null); |
There was a problem hiding this comment.
@johanjanssens Testing using the following code, there is still an issue:
$response->headers->set('Cache-Control', ['private', 'no-cache', 'max-age' => 100]);
var_dump('actual', $response->headers->get('Cache-Control', null, false));
var_dump('parsed', $response->getCacheControl()); // PROBLEM: only returns 'private'
var_dump('max age', $response->getMaxAge()); // PROBLEM: returns 0 as above call does not return max-age
// The below code works as intended
$response->headers->set('Cache-Control', 'private, no-cache, max-age=100');
var_dump('actual', $response->headers->get('Cache-Control', null, false));
var_dump('parsed', $response->getCacheControl());
var_dump('max age', $response->getMaxAge());
This is happening because we are not passing the third parameter named $first in the above line. However when you pass it this time it breaks the string parsing :) Then the string is returned as an unparsed array
There was a problem hiding this comment.
@ercanozkaya You are right, i have improved the code to handle this case too. Please review.
- Do not cast header values to array when being set - Properly handle rendering of multiple header values
…array by default
|
@ercanozkaya Can you re-review? I believe to have resolved the issues now, your test is validating on my end. |
|
@ercanozkaya Would you mind merging these changed to Kodekit? |
|
@johanjanssens all done
…On Wed, Jan 9, 2019 at 1:19 AM Johan Janssens ***@***.***> wrote:
@ercanozkaya <https://github.com/ercanozkaya> Would you mind merging
these changed to Kodekit?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#242 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFhDuSV49eathb4qaFZAA4KkrKsJuh4ks5vBRlogaJpZM4ZlIBk>
.
--
Ercan Özkaya
|
This reverts commit c5afea0.
…aching"" This reverts commit a032fc4.
This reverts commit 269e22b.
The PR also moves etag based validation to the http response transport making it an integral part of the request/response cycle.
It fixes an issue with setting of headers and serialisation if the header values are set as an array. The code wasn't able to handle this properly.