Problem/Motivation
During development of a persistent cache for resource configurations provided by the repository on jsonapi level in here: #3018287: ResourceTypeRepository computes ResourceType value objects on *every request*, it turned out that jsonapi_extras is not handling this type of caches consistently.
This follow-up is aimed at adding test coverage for the broken use case that now requires manual testing, as well as cache tags invalidations that are still needed to have the persistent cache in jsonapi invalidated correctly.
Project context:
Drupal: 8.6.3
jsonapi: 2.0-RC3
jsonapi_extras: 3.0.0
Proposed resolution
New tests to show the use case.
New tag to be added on resources list cache invalidation.
Check with @wim who had the initial idea :D...
Remaining tasks
See proposed solution.
TBD...
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
None.
Comment | File | Size | Author |
---|---|---|---|
#29 | 3019729-29.patch | 2.06 KB | ndobromirov |
| |||
#14 | 3019729-14.patch | 817 bytes | Wim Leers |
| |||
#14 | interdiff.txt | 347 bytes | Wim Leers |
#2 | 3019729-2.patch | 751 bytes | ndobromirov |
#7 | 3019729-7.patch | 977 bytes | rhristov |
|
Comments
Comment #2
ndobromirov CreditAttribution: ndobromirov at FFW commentedHere is the initial patch to start things off - adding the new tags on the repository's list.
This is based off the patch from #31 in the parent issue.
@wim, am I understanding the direction correctly?
Comment #3
Wim LeersYes!
Please use
@inheritdoc
. You'll find many examples of that inc ore and contrib :)Too bad we have to repeat this cache tag; with newer PHP versions we could just append to the array!
Actually, perhaps JSON:API Extras can make that PHP version assumption!? I think it's reasonable to require PHP 7 now :)
Comment #4
e0ipsoI agree, PHP7 is a reasonable requirement.
Comment #5
ndobromirov CreditAttribution: ndobromirov at FFW commented#4 :D BIG +1!
How about 7.1, as 7.0 is already EOL...
Comment #6
ndobromirov CreditAttribution: ndobromirov at FFW commentedSo we need to clear that cache tags on any modification of resources in jsonapi_extras. The property name needs to change, as it was renamed in the parent issue.
Comment #7
rhristov CreditAttribution: rhristov at Bulcode commentedApplying a patch that is taking care of invalidation of the resources types list.
Also added as a requirement the PHP version to be equal or higher than 7.0.
Comment #9
ndobromirov CreditAttribution: ndobromirov at FFW commentedComment #10
rhristov CreditAttribution: rhristov at Bulcode commentedThe tests are failing because of https://www.drupal.org/project/jsonapi_extras/issues/3020237 , but once fixed they will succeed.
Comment #11
ndobromirov CreditAttribution: ndobromirov at FFW commentedComment #12
ndobromirov CreditAttribution: ndobromirov at FFW commentedI've done some manual testing and this seems to work correctly with the latest parch from @rhristov.
Does it need something more?
Comment #14
Wim Leers#3020237: [Regression] Broken with latest jsonapi 2.0-rc3 landed, retesting.
This change is made due to my remark in #3.2. But I think my comment was misguided: we still cannot just append to the parent property's array. So IMHO we should drop this change.
Comment #17
e0ipsoThanks for this!
Comment #19
ndobromirov CreditAttribution: ndobromirov at FFW commentedThis would be a better fix to acommodate the latest changes in the parent issue.
I can not make a patch as my project is a bit behind on versions.
But the main idea is to drop the property definition we currently have and overwrite the property from the c-tor and compute the property after the parent is constructed.
Comment #20
Wim Leers#19++
Note this patch was committed prematurely because #3018287: ResourceTypeRepository computes ResourceType value objects on *every request* hadn't landed yet upstream!
Comment #21
ndobromirov CreditAttribution: ndobromirov at FFW commentedSo now that the other one is committed, Are we re-opening this or opening a new one?
Comment #22
e0ipsoRe-opening.
Comment #23
ndobromirov CreditAttribution: ndobromirov at FFW commentedHere is a patch for the proposed solution in #19.
Comment #25
ndobromirov CreditAttribution: ndobromirov at FFW commentedOops missing use statements :D
Comment #26
ndobromirov CreditAttribution: ndobromirov at FFW commentedAdded them :)...
Comment #27
ndobromirov CreditAttribution: ndobromirov at FFW commentedComment #29
ndobromirov CreditAttribution: ndobromirov at FFW commentedFound it :)!
It will work against D8.8, but it's failing against D8.7, as the patch was not committed there yet.
Here is a version with feature detection in place, that will handle the case for both versions of the module, as well as if people decide to patch 8.7 with the latest patch from the parent issue.
Comment #30
ndobromirov CreditAttribution: ndobromirov at FFW commentedPing.
Comment #31
ndobromirov CreditAttribution: ndobromirov at FFW commentedPing again :D.
The patch should work. Moving to RTBC.
Comment #33
e0ipsoThanks for the pings and the awesome work! This should be merged now.
Comment #34
e0ipsoComment #35
Wim Leers🥳
Comment #37
Mykola Dolynskyiin version 8.x-3.20 I am facing the situation that node was edited and drupal renders correct version. But API endpoint still gives old one. I have cleared the cache with drush and redis flush cache done, both don't help