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
ELPP-2452 Added cache headers to recommendations #57
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a test.
Hi @thewilkybarkid we don't currently have a web test foundation set up for recommendations, would you like me to set that up for this? If so is there more test cases that you think should be covered? |
@stephenwf we will also have to add |
src/App/Kernel.php
Outdated
} | ||
|
||
public function cache(Request $request, Response $response) | ||
{ | ||
$response->setMaxAge($this->app['config']['ttl']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way to override this for particular responses? (will be needed in the future)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ping has been added that does not use caching, make sure to merge develop
in this branch to solve possible conflicts
3aeef43
to
ccbd73c
Compare
src/App/Kernel.php
Outdated
@@ -430,7 +430,7 @@ public function handleException(Throwable $e): Response | |||
return new JsonResponse(array_filter([ | |||
'error' => $e->getMessage(), | |||
'trace' => $this->app['config']['debug'] ? $e->getTraceAsString() : null, | |||
]), $e->getCode()); | |||
]), $e->getCode() ? $e->getCode() : 500); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$e->getCode() ?? Response::HTTP_INTERNAL_SERVER_ERROR
?
We'll need a test for |
Now that web test cases seem well supported, I agree; there is a smoke test but it doesn't check caching headers. |
tests/src/Web/CacheHeadersTest.php
Outdated
$this->jsonRequest('GET', '/ping'); | ||
$response = $this->getResponse(); | ||
$this->assertEquals(200, $response->getStatusCode()); | ||
$this->assertEquals('max-age=300, must-revalidate, no-cache, no-store, public, stale-if-error=86400, stale-while-revalidate=300', $response->headers->get('Cache-Control')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be the same as https://github.com/elifesciences/search/blob/develop/tests/src/Search/Web/PingTest.php#L25-L27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I've added in a PrivateResponse
class that bypasses the cache method completely.
No description provided.