Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The asset's cache needs to be invalidated after a log's status that assigns a group membership is changed (done/pending).
Similar to #3240950: Invalidate asset cache when new weight observations are saved and #3240951: Asset cache is not invalidated after inventory log status changes
Steps to reproduce
- Assign an asset to a group. View the asset in JSONAPI.
- Change the log with the inventory adjustment to "pending".
- View the asset in JSONAPI, it will show the old group until the asset's cache is rebuilt.
Proposed resolution
TBD
Remaining tasks
Test and implement.
User interface changes
none.
API changes
Correct group membership always shown in JSONAPI.
Data model changes
None.
Issue fork farm-3241078
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:
- 2.x-inventory-log-cache compare
- 2.x-group-membership-cache changes, plain diff MR !34
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 CreditAttribution: paul121 commentedSolved by implementing a
LogEventSubscriber
like 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::invalidateAssetCacheOnMovement
logic 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): bool
that the farm_group log event subscriber can use.Alternatively, *I think* we could include this
isActiveMovementLog
function on thelog.location
service itself. That would make the logic even more reusable.Rather than test this behavior by making JSONAPI requests I created a new
FarmEntityCacheTestTrait
test 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 CreditAttribution: 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_normalization
cache 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 CreditAttribution: 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.