Problem/Motivation
Reported by @effulgentsia at #2827797-55: ResourceResponse(Subscriber) + Dynamic Page Cache: making authenticated ResourceResponses significantly faster:
+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php @@ -395,13 +409,44 @@ public function testGet() { + // Assert that Dynamic Page Cache did not store a ResourceResponse object, + // which needs serialization after every cache hit. Instead, it should + // contain a flattened response. Otherwise performance suffers. + // @see \Drupal\rest\EventSubscriber\ResourceResponseSubscriber::flattenResponse() + $cache_items = $this->container->get('database') + ->query("SELECT cid, data FROM {cache_dynamic_page_cache} WHERE cid LIKE :pattern", [ + ':pattern' => '%[route]=rest.%', + ]) + ->fetchAllAssoc('cid'); + foreach ($cache_items as $cid => $cache_item) {This test seems a little brittle. If the cid pattern changes in the future, this test will return an empty array and the foreach will get silently skipped, so the test will still pass. Perhaps there should be a followup to improve on this? Do we really have a situation where we don't know the exact $cid to check for?
We need the wildcard, to fetch the multiple relevant cache items: there's a cache redirect cache item, and an "actual" cache item.
If I add:
.../tests/src/Functional/EntityResource/EntityResourceTestBase.php | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
index 5bcf5fe..04a66f6 100644
--- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -437,6 +437,7 @@ public function testGet() {
->fetchAllAssoc('cid');
foreach ($cache_items as $cid => $cache_item) {
$cached_data = unserialize($cache_item->data);
+ var_dump($cached_data);
if (!isset($cached_data['#cache_redirect'])) {
$cached_response = $cached_data['#response'];
$this->assertNotInstanceOf(ResourceResponseInterface::class, $cached_response);
And then run NodeJsonBasicAuthTest, you get this output:
array(2) {
["#cache_redirect"]=>
bool(true)
["#cache"]=>
array(5) {
["keys"]=>
array(1) {
[0]=>
string(8) "response"
}
["contexts"]=>
array(4) {
[0]=>
string(14) "request_format"
[1]=>
string(5) "route"
[2]=>
string(8) "url.site"
[3]=>
string(16) "user.permissions"
}
["tags"]=>
array(2) {
[0]=>
string(32) "config:rest.resource.entity.node"
[1]=>
string(6) "node:1"
}
["max-age"]=>
int(-1)
["bin"]=>
string(18) "dynamic_page_cache"
}
}
array(5) {
["#markup"]=>
string(0) ""
["#attached"]=>
string(0) ""
["#cache"]=>
array(3) {
["contexts"]=>
array(4) {
[0]=>
string(14) "request_format"
[1]=>
string(5) "route"
[2]=>
string(8) "url.site"
[3]=>
string(16) "user.permissions"
}
["tags"]=>
array(2) {
[0]=>
string(32) "config:rest.resource.entity.node"
[1]=>
string(6) "node:1"
}
["max-age"]=>
int(-1)
}
["#cache_properties"]=>
array(1) {
[0]=>
string(9) "#response"
}
["#response"]=>
object(Drupal\Core\Cache\CacheableResponse)#11508 (7) {
["headers"]=>
object(Symfony\Component\HttpFoundation\ResponseHeaderBag)#1223 (5) {
["computedCacheControl":protected]=>
array(2) {
["no-cache"]=>
bool(true)
["private"]=>
bool(true)
}
["cookies":protected]=>
array(0) {
}
["headerNames":protected]=>
array(3) {
["cache-control"]=>
string(13) "Cache-Control"
["link"]=>
string(4) "Link"
["content-type"]=>
string(12) "Content-Type"
}
["headers":protected]=>
array(3) {
["cache-control"]=>
array(1) {
[0]=>
string(17) "no-cache, private"
}
["link"]=>
array(5) {
[0]=>
string(35) "<http://d8/node/1>; rel="canonical""
[1]=>
string(78) "<http://d8/node/1/delete>; rel="https://drupal.org/link-relations/delete-form""
[2]=>
string(40) "<http://d8/node/1/edit>; rel="edit-form""
[3]=>
string(51) "<http://d8/node/1/revisions>; rel="version-history""
[4]=>
string(68) "<http://d8/node/1>; rel="https://drupal.org/link-relations/revision""
}
["content-type"]=>
array(1) {
[0]=>
string(16) "application/json"
}
}
["cacheControl":protected]=>
array(0) {
}
}
["content":protected]=>
string(800) "{"nid":[{"value":1}],"uuid":[{"value":"7e0ce3b9-082e-45e6-8ede-489ab77277d8"}],"vid":[{"value":1}],"langcode":[{"value":"en"}],"type":[{"target_id":"camelids","target_type":"node_type","target_uuid":"a4050c00-df34-4d74-92f4-6e08e73c8dfb"}],"revision_timestamp":[{"value":123456789}],"revision_uid":[{"target_id":2,"target_type":"user","target_uuid":"068851de-a726-4984-9d42-8578bd427ac2","url":"\/user\/2"}],"revision_log":[],"status":[{"value":true}],"title":[{"value":"Llama"}],"uid":[{"target_id":2,"target_type":"user","target_uuid":"068851de-a726-4984-9d42-8578bd427ac2","url":"\/user\/2"}],"created":[{"value":123456789}],"changed":[{"value":1494525772}],"promote":[{"value":true}],"sticky":[{"value":false}],"revision_translation_affected":[{"value":true}],"default_langcode":[{"value":true}]}"
["version":protected]=>
string(3) "1.0"
["statusCode":protected]=>
int(200)
["statusText":protected]=>
string(2) "OK"
["charset":protected]=>
NULL
["cacheabilityMetadata":protected]=>
object(Drupal\Core\Cache\CacheableMetadata)#7539 (3) {
["cacheContexts":protected]=>
array(2) {
[0]=>
string(8) "url.site"
[1]=>
string(16) "user.permissions"
}
["cacheTags":protected]=>
array(2) {
[0]=>
string(32) "config:rest.resource.entity.node"
[1]=>
string(6) "node:1"
}
["cacheMaxAge":protected]=>
int(-1)
}
}
}
Proposed resolution
Because there's always a cache redirect, assert the number of cache items.
Remaining tasks
User interface changes
None.
API changes
None.
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 2877778-5.patch | 1.42 KB | wim leers |
Comments
Comment #2
wim leersUpdated IS with explanation.
Attached patch makes it more strict.
Comment #3
tedbowOk looking at this as someone who doesn't know the cache system as well this is what I think the intention of the test is.
for this:
It looks like there should be 2 items in $cache_items and 1 of those items should meet these conditions
But if neither of the items passes this condition
Then it will the test will also pass. I assuming that would never happen but again that is because I don't know that cache system as well.
Would it make sense to put flag
$cacheable_response_found = FALSE;Before the
foreachand set it toTRUEif we pass theifin the loop.Then assert
cacheable_response_found === TRUEafter the loop?Comment #4
wim leers#3 good call!
Comment #5
wim leersComment #6
tedbow@wim thanks for the updates.
This looks good now, RTBC!
Comment #7
alexpottCommitted 14a1f0c and pushed to 8.4.x. Thanks!
Conflcts on 8.3.x so not backported.
Comment #9
wim leersThat makes sense, because #2827797: ResourceResponse(Subscriber) + Dynamic Page Cache: making authenticated ResourceResponses significantly faster was also committed to 8.4.x only.