- Issue created by @prudloff
- last update
11 months ago Composer require-dev failure - Status changed to Needs review
11 months ago 11:08am 15 January 2024 - 🇦🇹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
11 months ago 5:00pm 6 February 2024 - 🇦🇹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
10 months ago 4:44pm 12 February 2024 - 🇦🇹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
4 months ago 2:46pm 19 August 2024 - 🇦🇹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.
- 🇫🇷France alexl
The cache plugin appears on views that are not search_api views.
- 🇦🇹Austria drunken monkey Vienna, Austria
@alexl: Thanks, should be fixed!
- 🇦🇹Austria drunken monkey Vienna, Austria
@kasperg: *ping*
Is anyone else interested in this and wants to test and confirm it works?
- 🇩🇰Denmark kasperg
@drunken monkey: Sorry about not getting back to you sooner. The patch required Search API 1.33, our related project is still on 1.31.
In any case: I have tested the patch and it looks good to me.
I have:
- Created a view of an index with a field based on a datetime field
- Configured the view to show entities where the field has a value greater than now
- Configured the view to use the new tag and time based caching plugin with 5 minutes cache lifetime
- Rendered the view to fill the cache
- Created a new entity where the datetime field is 3 minutes in the future
- Rerendered the view to see that the new entity shows up. This should show that tag based caching still works.
- Waited 10 minutes to ensure that the cache lifetime should be exceeded
- Rerendered the view to see that the new entity no longer shows up. This should show that time based caching also works. - 🇦🇹Austria drunken monkey Vienna, Austria
Good to hear, thanks for reporting back!
And no worries about the delay, as long as the issue doesn’t just sink into oblivion a few months more is no problem.
So, merged. Thanks again, everyone! -
drunken monkey →
committed 2c813fa8 on 8.x-1.x authored by
prudloff →
Issue #3414725 by prudloff, drunken monkey, kasperg: Added the “Time and...
-
drunken monkey →
committed 2c813fa8 on 8.x-1.x authored by
prudloff →
Automatically closed - issue fixed for 2 weeks with no activity.