Problem/Motivation
In the issue Facets summary block always cached the FacetsSummaryBlock has been marked uncacheable with the UncacheableDependencyTrait. According to that issue, the summary was always cached and the solution to that was to make the block cacheable in it's entirety "for now" in 2016. Fast forward to now and this issue has never been resolved.
The uncacheability of this block caused issues on our Varnish cache, especially since `X-Drupal-Dynamic-Cache` ends up as `UNCACHEABLE` and therefor our purge implementation will not pass through the `Cache-Tags`.
Steps to reproduce
Add a Facets summary block to the page, note that the entire page is uncacheable.
Proposed resolution
When reverting the patch that has been added in the aforementioned issue, I can't seem to reproduce the reported issue. I suggest to re-introduce the `url.query` context and make the block cacheable again.
Remaining tasks
- Collect cache metadata from active facets while rendering facets summary.
- Add corresponding facet summary cache tags into the Search API query, reuse Facet approach with
::registerFacet()
-
Provide tests that will cover enabled cache scenarios.
I think this can be achieved by creating new test class and implementing following tests from
\Drupal\Tests\facets_summary\Functional\IntegrationTest
and\Drupal\Tests\facets_summary\Functional\HierarchicalFacetIntegrationTest
, but without UI part:IntegrationTest:
testFramework- admin UI test, can be omitted- testShowSummary
- testViewsCacheDisable, should cover none Search API cache plugins.
- testCount
testBlockDelete- admin UI test, can be omittedconfigureShowCountProcessor- admin UI test, can be omittedconfigureResetFacetsProcessor- admin UI test, can be omitted- testEmptyFacetLinks
- testResetFacetLink
- testResetFacetLink
HierarchicalFacetIntegrationTest
- testHierarchicalFacet
As alternative: change Facet source to cacheable view display and add
$this->drupalLogin($admin)
for the UI parts, and before results check switch back to anonymous, if needed:$this->drupalLogout()
User interface changes
N/a
API changes
Data model changes
Introduce:public Drupal\facets_summary\FacetsSummaryManager::returnBuiltFacet()
which can be used in the getCacheTags, getCacheContext, getCacheMaxAge facet summary block.
Comment | File | Size | Author |
---|---|---|---|
#16 | 3264284.patch | 14.66 KB | mkalkbrenner |
#11 | interdiff_10_11.txt | 4.65 KB | BAHbKA |
#11 | facets-summary-enable-caching-3264284-11.patch | 104.62 KB | BAHbKA |
#10 | facets-summary-enable-caching-3264284-10.patch | 104.22 KB | BAHbKA |
Issue fork facets-3264284
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
aken.niels@gmail.comComment #6
aken.niels@gmail.comPlease review thoroughly, don't take my word for the cacheability of this block. I am not able to reproduce, but maybe someone else can. I've added an MR with a revert commit.
Comment #7
BAHbKA CreditAttribution: BAHbKA as a volunteer commentedComment #8
BAHbKA CreditAttribution: BAHbKA as a volunteer commentedComment #10
BAHbKA CreditAttribution: BAHbKA as a volunteer commentedEnable cache support for the facets summary block. Facets summary cache metadata is sum of all facets cache metadata assigned to it:
Please review.
Comment #11
BAHbKA CreditAttribution: BAHbKA at FFW commentedTo tell the true, don't know what has happened: dummy cache context class stoped receiving params, So I've split it in tow classes.
Comment #12
BAHbKA CreditAttribution: BAHbKA as a volunteer commentedComment #14
bob.hinrichs CreditAttribution: bob.hinrichs commentedWould like to try this but it is not applying. Is set up like all my other module patches. Any idea why? Facets module we have is 2.0.5.
"extra" : {
"patches": {
"drupal/facets": {
"Facets summary should be cacheable": "https://www.drupal.org/files/issues/2022-05-30/facets-summary-enable-cac..."
},
Comment #15
mkalkbrennerThe caching implementation in factes already went through bug fixing iterations. The two biggest issues were
So the patch needs to be reviewed and adjusted based on the current implementation, for example a SearchApiSubscriber::alterQuery() method must merge the cache metadata to the query at runtime.
Comment #16
mkalkbrennerHere's a patch that bases on the latest cache integration in 2.0.x and 3.0.x.
Comment #21
mkalkbrenner