With migrate_plus, attempting to do a drush migrate-status (which reports available source counts, among other things) breaks due to the exception

SQLSTATE[HY000]: General error: 1366 Incorrect integer value: 'cache' for column 'expire' at row 1: INSERT INTO {cache_migrate} (cid, created, expire, tags, checksum, data, serialized) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6); Array
(
    [:db_insert_placeholder_0] => 53bb9b0b38ac812554abeb88ea6d56760d07ec147a75e8761c4f57ecc298218f
    [:db_insert_placeholder_1] => 1432158458.659
    [:db_insert_placeholder_2] => cache
    [:db_insert_placeholder_3] => 
    [:db_insert_placeholder_4] => 0
    [:db_insert_placeholder_5] => i:2430;
    [:db_insert_placeholder_6] => 1
)

SourcePluginBase caches counts with

      $this->getCache()->set($this->cacheKey, $count, 'cache');

But the signature on CacheBackendInterface::set() is

  public function set($cid, $data, $expire = Cache::PERMANENT, array $tags = array());

I'm not sure why this is just becoming apparent now, the last change to that interface was January, but it's definitely broken.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeryan’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
1.09 KB

This really needs a fail test (why is the existing cached source count test not failing?), but capturing the fix that I need right now...

mikeryan’s picture

Issue tags: -Needs tests
FileSize
1.01 KB
2.09 KB

OK, so the problem here is SourcePluginBase passing a string where the cache set() interface expects an integer. The reason the tests didn't pick this up is because they're using NullBackend, which doesn't do anything with the bad argument. ApcuBackend looks like the simplest cache backend that will choke on the bad argument.

The last submitted patch, 2: migration_count_caching-2492429-2-FAIL.patch, failed testing.

mikeryan’s picture

Results as expected - ready for manual review.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, and it's green. I approve.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Yep this is just broken. Committed c523c99 and pushed to 8.0.x. Thanks!

  • alexpott committed c523c99 on 8.0.x
    Issue #2492429 by mikeryan: Migration count caching broken
    
neclimdul’s picture

Status: Fixed » Needs work

This forces a requirement on APCU for tests to pass. We need to do some sort of mock or @require.

  • alexpott committed 626f64d on 8.0.x
    Revert "Issue #2492429 by mikeryan: Migration count caching broken"...
alexpott’s picture

I've revert this so we can do the necessary mocking to avoid #8.

mikeryan’s picture

Here we go, mocking the cache interface so we can validate we're setting proper arguments.

The last submitted patch, 11: migration_count_caching-2492429-11-FAIL.patch, failed testing.

mikeryan’s picture

Yep, the one that was supposed to fail did - ready for manual (re)review.

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

interdiff looks great, works without apcu, RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 967b16f on 8.0.x
    Issue #2492429 by mikeryan, neclimdul: Migration count caching broken
    

Status: Fixed » Closed (fixed)

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