search_api_time cache plugin does not add search_api_list cache tag

Created on 15 January 2024, 10 months ago

Problem/Motivation

When using the search_api_time cache plugin, the view data is only invalidated when the cache expires and not when the index data changes.
According to this comment it is by design: https://git.drupalcode.org/project/search_api/-/commit/23562c3443973683a...

However, it is a bit confusing because it is different than what happens with SQL views (the time cache plugin still adds the node_list cache tag) and it can still be useful to invalidate a view both when the data changes and after a certain amount of time.

Steps to reproduce

  1. Create a search_api view that uses the search_api_time cache plugin.
  2. Edit an indexed node and reindex it.
  3. The view data is not invalidated.

Proposed resolution

I propose adding a new cache plugin that adds both a max-age and the search_api_list cache tag to the view.

🐛 Bug report
Status

Active

Version

1.0

Component

General code

Created by

🇫🇷France prudloff Lille

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @prudloff
  • Merge request !103New search_api_time_tag cache plugin → (Open) created by prudloff
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.4 + Environment: PHP 8.2 & sqlite-3.34
    last update 10 months ago
    Composer require-dev failure
  • Pipeline finished with Failed
    10 months ago
    Total: 44s
    #77399
  • Status changed to Needs review 10 months ago
  • 🇦🇹Austria drunken monkey Vienna, Austria

    Thanks for reporting this problem and already providing a solution!

    I wasn’t aware that the time-based cache plugin for SQL views also added list cache tags, otherwise we would probably have done the same. Anyways, since it’s been working like that for a while now, I agree that we probably shouldn’t change this behavior. Adding a new cache plugin that implements this behavior does seem more sensible.
    However, as with all new features, it would be good to hear from a few others first that would also find this useful. I want to avoid adding features that are too niche.

    Also, before merging this, we’ll definitely need some test coverage. This could probably be largely based on the existing tests for the time-based cache plugin, just with some of the assertions changed/reversed.

  • Status changed to Postponed 9 months ago
  • 🇦🇹Austria drunken monkey Vienna, Austria

    Discussing this now as part of #3327645-18: View recalculated with wrong data from cache , please see there. If we resolve it there, this issue becomes outdated, otherwise I’d move it back to NW.

  • Status changed to Needs work 9 months ago
  • 🇦🇹Austria drunken monkey Vienna, Austria

    Based on 🐛 View recalculated with wrong data from cache Needs review , the current behavior of the time-based cache plugin is intended and should not be changed.
    So you’re right in your approach, the only thing to resolve this issue would be to add a third Search API-specific cache plugin that invalidates based both on cache tags and max-age. I can imagine that that would also be useful to a significant portion of sites. However, hearing from more potential users would of course be great.

    In any case, this still needs tests. Shouldn’t be too hard, though, using the existing ViewsDisplayCachingTest.

  • 🇦🇹Austria drunken monkey Vienna, Austria

    Oh, and please use patches instead of issue forks for development in this project, at least until we get 📌 (Try to) fix the GitLab CI RTBC resolved.

  • 🇩🇰Denmark kasperg

    However, hearing from more potential users would of course be great.

    I would like to see this feature as well.

    [...] this still needs tests. Shouldn’t be too hard, though, using the existing ViewsDisplayCachingTest. [...] please use patches instead of issue forks for development in this project [...]

    I am unsure what the status of this is. The issue mentioned has been fixed and drupal.org now no longer recommends patches.

    In any case here is a patch which contains the change from the original merge request with a new Views cache plugin support both time and cache tags and a first stab at extending ViewsDisplayCachingTest to include a test for this plugin.

    I can change it to a merge request if needed.

  • Status changed to Needs review 3 months ago
  • 🇦🇹Austria drunken monkey Vienna, Austria

    Thanks for moving this along.
    Yes, the problem with testing issue forks has finally been resolved, so we use MRs now. I’ve added your patch to the MR now.

    As for the code, neither the test nor the cache plugin really seemed to work. I’ve now fixed both, I think – please test/review.

  • 🇩🇰Denmark kasperg

    Thanks for the update and your work. I will look into your changes.

  • 🇦🇹Austria drunken monkey Vienna, Austria

    Any update on this?

  • 🇫🇷France alexl

    The cache plugin appears on views that are not search_api views.

  • 🇦🇹Austria drunken monkey Vienna, Austria

    @alexl: Thanks, should be fixed!

Production build 0.71.5 2024