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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ndobromirov created an issue. See original summary.

ndobromirov’s picture

Here 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?

Wim Leers’s picture

Title: Invalidate jsonapi cache tags » Invalidate JSON:API Extras' cache tags when
Issue tags: +D8 cacheability

@wim, am I understanding the direction correctly?

Yes!


  1. +++ b/src/ResourceType/ConfigurableResourceTypeRepository.php
    @@ -57,6 +57,17 @@ class ConfigurableResourceTypeRepository extends ResourceTypeRepository {
    +  /**
    +   * List of default cache tags added by the repository.
    +   *
    +   * @var array
    +   */
    

    Please use @inheritdoc. You'll find many examples of that inc ore and contrib :)

  2. +++ b/src/ResourceType/ConfigurableResourceTypeRepository.php
    @@ -57,6 +57,17 @@ class ConfigurableResourceTypeRepository extends ResourceTypeRepository {
    +    'jsonapi_resource_types',
    

    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 :)

e0ipso’s picture

Actually, perhaps JSON:API Extras can make that PHP version assumption!? I think it's reasonable to require PHP 7 now :)

I agree, PHP7 is a reasonable requirement.

ndobromirov’s picture

#4 :D BIG +1!

How about 7.1, as 7.0 is already EOL...

ndobromirov’s picture

So 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.

rhristov’s picture

Status: Active » Needs review
FileSize
977 bytes

Applying 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.

The last submitted patch, 7: 3019729-7.patch, failed testing. View results

ndobromirov’s picture

Title: Invalidate JSON:API Extras' cache tags when » Invalidate JSON:API Extras' cache tags when неедед
rhristov’s picture

Title: Invalidate JSON:API Extras' cache tags when неедед » Invalidate JSON:API Extras' cache tags when needed

The tests are failing because of https://www.drupal.org/project/jsonapi_extras/issues/3020237 , but once fixed they will succeed.

ndobromirov’s picture

Issue summary: View changes
ndobromirov’s picture

Status: Needs review » Reviewed & tested by the community

I've done some manual testing and this seems to work correctly with the latest parch from @rhristov.
Does it need something more?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 3019729-2.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
347 bytes
817 bytes

#3020237: [Regression] Broken with latest jsonapi 2.0-rc3 landed, retesting.

+++ b/composer.json
@@ -18,6 +18,7 @@
+        "php": "^7.0",

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.

The last submitted patch, 7: 3019729-7.patch, failed testing. View results

  • rhristov authored cc4a2e1 on 8.x-3.x
    Issue #3019729 by Wim Leers, ndobromirov, rhristov, e0ipso: Invalidate...
e0ipso’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for this!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

ndobromirov’s picture

This 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.

  /**
   * {@inheritdoc}
   */
  public function __construct(
    EntityTypeManagerInterface $entity_type_manager, 
    EntityTypeBundleInfoInterface $entity_bundle_info, 
    EntityFieldManagerInterface $entity_field_manager, 
    CacheBackendInterface $cache
  ) {
    parent::__construct($entity_type_manager, $entity_bundle_info, $entity_field_manager, $cache);

    $this->cacheTags = array_merge($this->cacheTags, [
      'config:jsonapi_extras.settings',
      'config:jsonapi_resource_config_list',
    ]);
  }
Wim Leers’s picture

#19++

Note this patch was committed prematurely because #3018287: ResourceTypeRepository computes ResourceType value objects on *every request* hadn't landed yet upstream!

ndobromirov’s picture

So now that the other one is committed, Are we re-opening this or opening a new one?

e0ipso’s picture

Status: Closed (fixed) » Needs work

Re-opening.

ndobromirov’s picture

Status: Needs work » Needs review
FileSize
1.34 KB

Here is a patch for the proposed solution in #19.

Status: Needs review » Needs work

The last submitted patch, 23: 3019729-23.patch, failed testing. View results

ndobromirov’s picture

Oops missing use statements :D

ndobromirov’s picture

Added them :)...

ndobromirov’s picture

Status: Needs work » Needs review

The last submitted patch, 26: 3019729-26.patch, failed testing. View results

ndobromirov’s picture

FileSize
2.06 KB

Found 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.

ndobromirov’s picture

Ping.

ndobromirov’s picture

Status: Needs review » Reviewed & tested by the community

Ping again :D.

The patch should work. Moving to RTBC.

  • e0ipso committed 77cf0e3 on 8.x-3.x authored by ndobromirov
    Issue #3019729 by ndobromirov, Wim Leers, rhristov, e0ipso: Invalidate...
e0ipso’s picture

Thanks for the pings and the awesome work! This should be merged now.

e0ipso’s picture

Status: Reviewed & tested by the community » Fixed
Wim Leers’s picture

🥳

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Mykola Dolynskyi’s picture

in 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