- Issue created by @pwolanin
- π³πΏNew Zealand quietone
Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies β .
- π¬π§United Kingdom catch
If we add a cache context to a query, then only variation cache / render cache will actually cause that context to be calculated (once it's bubbled up to something that going to be written to cache), until that point, it's just a string in an array like
['contexts' => 'user']
, so we should be fine to add contexts and tags directly and not need a callback system. - π¬π§United Kingdom catch
I think we need something like ::addCacheableMetadata() and ::getCacheableMetadata() on the query objects (entity and database), and then the code that executes the query is responsible for getting the cacheablemetadata and merging it into the render array.
Does it makes for
\Drupal\Core\Database\Query\Select
(and maybe\Drupal\Core\Database\Query\SelectExtender
) and\Drupal\Core\Entity\Query\QueryBase
to implement\Drupal\Core\Cache\RefinableCacheableDependencyInterface
and use\Drupal\Core\Cache\RefinableCacheableDependencyTrait
? There's nogetCacheableMetadata()
, but there are getter methods for context, tags, and max-age, and you can also then merge the query object as a cacheable dependency to any other cacheable dependency object.- Merge request !12535Issue #3516034: Add cacheable metadata to entity query and DB select query objects. β (Open) created by godotislate
Put up draft MR 12535 implementing #6. Added a kernel test which essentially tests what
Drupal\Tests\node\Functional\NodeAccessCacheabilityTest::testNodeAccessCacheabilitySafeguard()
does, but without rendering. There probably should be additional test coverage for more generic usage.A couple questions came to mind:
- Should
Drupal\Core\Database\Connection\SelectInterface
andDrupal\Core\Entity\Query\QueryInterface
extendRefinableCacheableDependencyInterface
? It would eliminate the need to doinstanceof
checks, but there are BC considerations - Implementations of
EntityStorageInterface::loadByProperties()
use entity queries under the hood. Should there be an optional second parameter onloadByProperties()
to capture cacheable metadata?
- Should
Should there be an optional second parameter on loadByProperties() to capture cacheable metadata?
Then there's the EntityRepository method
loadEntityByUuid
method that calls EntityStorage::loadByProperties, and possible other rabbit holes, so I don't know if it makes sense to add an additional parameter everywhere, at least not in scope for this issue.- π¬π§United Kingdom catch
#9.1 does the
Select
class count as a base class here? It doesn't have theBase
keyword at the end but that's probably because it was originally added in c.2008. If it does, then I think we can add the interface to SelectInterface directly because Select will then implement the methods for any subclasses.#9.2 based on #10 that sounds like good material for a follow-up but we should open one :)
Yes, it looks like
Select
is essentially a base class.SelectExtender
also implementsSelectInterface
, but it is essentially a base class as well. And for entity queries,QueryBase
is the base class forQueryInterface
. Updated the MR to have the respective interfaces extendRefinableCacheableDependencyInterface
and added a CR [#3533258].The SQL entity query class merges the cache metadata from the underlying Select object, so this works for content entities. Config entity queries or other entity types using key value storage don't capture any cacheable metadata from SQL key value queries. Doing so would add complexity to key value and this seems like an edge case.
Follow up per #11: β¨ [PP-1] Make it possible to capture cacheable metadata from underlying entity queries in EntityStorageInterface::loadByProperties() Active .
Probably still needs some more test, so leaving in NW and the Needs tests tag.
OK, added more general tests. I also removed the NodeAccessCacheability kernel test I'd added before, because I think it's duplicating other tests mostly at this point, and instead added asserts in the functional test to test that the query tags get added to the render array when the query is not running in a render context.
Additionally, I found dead/broken test code #2878483: loadEntityByUuid() should skip access checks β in query alter hook in EntityTestHooks.php, and I removed all that and reused the hook for tests needed here.
One last thing:
It occurred to me that maybe entity query objects should include the corresponding entity type list cache tags and entity type list cache contexts. However, when I tried that the list cache contexts for the Node entity type are['user.node_grants:view']
, which would kinda defeat the point of all the logic in the query alter hook only to add those contexts conditionally.Separately I was also wondering if the page cache contexts should be added to
PagerSelectExtender
, but I haven't thought that through completely.I think the MR is ready for review regardless.