In the constructor of SourcePluginBase the cache_key configuration value for the count cache is used in a surprising way:

$this->cacheKey = !empty($configuration['cache_key']) ? !empty($configuration['cache_key']) : NULL;

As a result when I set a cache key in my migration configurations they all share the same count cache. I assume that it is a typo and instead it should be:

$this->cacheKey = !empty($configuration['cache_key']) ? $configuration['cache_key'] : NULL;

I can provide a patch if that is the correct behaviour.

I have also noticed that the default cache key provided is the same for all migrations sharing a source plugin.

if (!isset($this->cacheKey)) {
  $this->cacheKey = hash('sha256', $this->getPluginId());
}

I am a not sure how to obtain a safe default.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Stefan Freudenberg created an issue. See original summary.

Stefan Freudenberg’s picture

Title: Cache key configuration does not work as expected » Count cache configuration does not work as expected
Stefan Freudenberg’s picture

Issue summary: View changes
Stefan Freudenberg’s picture

Stefan Freudenberg’s picture

Status: Active » Needs review
FileSize
986 bytes

Pease review attached patch addressing the cache key issue.

mikeryan’s picture

Issue tags: +Needs tests

Looks good, but let's have a test demonstrating the failure.

Stefan Freudenberg’s picture

I have added a test to the patch.

Status: Needs review » Needs work

The last submitted patch, 7: count-cache-2723115-7.patch, failed testing.

Stefan Freudenberg’s picture

Status: Needs work » Needs review

Er. Not sure what to do. The test failure looks completely unrelated.

Status: Needs review » Needs work

The last submitted patch, 7: count-cache-2723115-7.patch, failed testing.

mayurjadhav’s picture

Status: Needs work » Needs review
FileSize
2.13 KB

Added updated patch.

vasi’s picture

Issue tags: +Needs followup

This is perfect. We just need a follow-up for the issue with the default cache key.

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests, -Needs followup
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed b0bc0c5 and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed 3993ef3 on 8.2.x
    Issue #2723115 by Stefan Freudenberg, mayurjadhav: Count cache...

  • alexpott committed b0bc0c5 on 8.1.x
    Issue #2723115 by Stefan Freudenberg, mayurjadhav: Count cache...

Status: Fixed » Closed (fixed)

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