Closed (fixed)
Project:
farmOS
Version:
2.x-dev
Component:
Miscellaneous
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
6 Oct 2021 at 12:22 UTC
Updated:
1 Nov 2021 at 16:04 UTC
Jump to comment: Most recent
Comments
Comment #2
m.stentaOh just found this note in my todos related to this:
Very poorly worded, so I'm trying to remember exactly what it means... but IIRC it's related to the fact that group membership affects asset location, but assets in a group can also have a separate location from the group (if they have a movement log after the group's movement log). I think I noticed that this was not updating properly in Views. Needs testing to determine what I was talking about... :-)
Comment #5
paul121 commentedSolved by implementing a
LogEventSubscriberlike we have done in farm_location for invalidating asset cache with movement logs.Solved by invalidating group members' cache tags when the group's location changes.
This is a little bit tricky since we start to depend on (and duplicate) the farm_location
LogEventSubscriber::invalidateAssetCacheOnMovementlogic to determine if a movement log has been updated. To simply things a bit I created a new public static function:public static function isActiveMovementLog(LogInterface $log): boolthat the farm_group log event subscriber can use.Alternatively, *I think* we could include this
isActiveMovementLogfunction on thelog.locationservice itself. That would make the logic even more reusable.Rather than test this behavior by making JSONAPI requests I created a new
FarmEntityCacheTestTraittest trait that provides a couple helper methods for testing invalidation of entity cache tags directly.At first I thought I could use the entity cache bin to check for cache hits/miss in tests:
But when I tried this I was *always* getting a "cache hit". I saw that these cache entries have a different cache tag (
asset_values) in the DB so I think they are used for something else (lower level) than we need.I settled on simply creating a new cache entry in the default cache bin that is dependent on the entity's cache tags. This is more similar to how the drupal render cache works anyways, each entry is dependent on other cache tag(s):
I refactored the existing farm_location tests to use this approach for testing cache tag invalidations.
Comment #6
paul121 commentedThis has led me to discover another more general bug re: caching... logs that are "done" with a timestamp in the future add another level of complication here. This is especially important for requests via the API. Consider the following:
jsonapi_normalizationcache bin)Not sure how important this is. It seems like it *could be* semi-straightforward to support this... Drupal has the concept of cache tags and the cache "max-age". *In theory*, when a planned log is created, we could set the asset's cache max-age to be the timestamp of that log. That way anything cached from the asset (and respecting its max-age) would be rebuilt appropriately. Of course, I'm not sure if we're able to specify an individual asset's cache max age... gotta sign off. This would probably be best to tackle in a separate issue.
Comment #7
paul121 commentedJust tacking on a couple commits to more accurately reflect what the LogEventSubscribers are doing.
Comment #8
m.stentaComment #9
m.stentaAh yea - that is a good point. We should open a new issue for that. I suppose some cron-based logic is the only way to handle that - and even then it would be hard to get it perfect. It seems like it would be a minor edge-case issue, in either case. In practice, logs with a timestamp in the future won't typically be marked as "done".
Comment #10
m.stentaOh oops didn't read your full comment before responding... that's interesting re: setting the cache max age.
Here is a new issue for that: #3244374: Asset cache is not invalidated when a future "done" log timestamp passes
Comment #12
m.stentaThanks for all the work on this @paul121! Merged.