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:

    1. testFramework - admin UI test, can be omitted
    2. testShowSummary
    3. testViewsCacheDisable, should cover none Search API cache plugins.
    4. testCount
    5. testBlockDelete - admin UI test, can be omitted
    6. configureShowCountProcessor - admin UI test, can be omitted
    7. configureResetFacetsProcessor - admin UI test, can be omitted
    8. testEmptyFacetLinks
    9. testResetFacetLink
    10. testResetFacetLink

    HierarchicalFacetIntegrationTest

    1. 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.

Issue fork facets-3264284

Command icon 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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nielsva created an issue. See original summary.

aken.niels@gmail.com’s picture

aken.niels@gmail.com’s picture

Status: Active » Needs review

Please 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.

BAHbKA’s picture

Title: Facets summary should be cacheable? » Facets summary should be cacheable, in case facets are using cacheable source.
Issue summary: View changes
Issue tags: +cache
Related issues: +#2939710: Add support for "Search API (tags based)" caching in Views
BAHbKA’s picture

Version: 8.x-1.x-dev » 2.0.x-dev
Status: Needs review » Needs work

BAHbKA’s picture

Enable cache support for the facets summary block. Facets summary cache metadata is sum of all facets cache metadata assigned to it:

  • In case facet doesn't have an active value, only process metadata is included to the summary e.g. data that is used to build facet query.
  • In case facet has an active value, cacheabillity from all stages will be included to the summary cache: pre_query, post_query, sort and build.
  • In case all facets attached to the summary are cacheable - summary is cacheable
  • In case if one of facets is un cacheable, whole summary block will omit cache e.g. max-age=0

Please review.

BAHbKA’s picture

To tell the true, don't know what has happened: dummy cache context class stoped receiving params, So I've split it in tow classes.

BAHbKA’s picture

Status: Needs work » Needs review

The last submitted patch, 10: facets-summary-enable-caching-3264284-10.patch, failed testing. View results

bob.hinrichs’s picture

Would 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..."
},

mkalkbrenner’s picture

Status: Needs review » Needs work

The caching implementation in factes already went through bug fixing iterations. The two biggest issues were

  • Getting Cache Metadata must not trigger searches
  • Cache metadata gathered at runtime must be merged onto the query, for example max-age 0 in case of an error

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.

mkalkbrenner’s picture

Version: 2.0.x-dev » 3.0.x-dev
Status: Needs work » Needs review
FileSize
14.66 KB

Here's a patch that bases on the latest cache integration in 2.0.x and 3.0.x.

  • mkalkbrenner committed 45ac682 on 3.0.x
    Issue #3264284 by BAHbKA, mkalkbrenner: Facets summary should be...

  • mkalkbrenner committed 6443435 on 2.0.x
    Issue #3264284 by BAHbKA, mkalkbrenner: Facets summary should be...

  • mkalkbrenner committed d66b15c on 3.0.x
    Issue #3264284 by BAHbKA, mkalkbrenner: Facets summary should be...

  • mkalkbrenner committed be0ba94 on 2.0.x
    Issue #3264284 by BAHbKA, mkalkbrenner: Facets summary should be...
mkalkbrenner’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.