SourcePluginBase should cache based on a hash of the source configuration, not just the plugin id.

No problems here with the implementation. Just some minor nits on on comments.

1. +++ b/core/modules/migrate/tests/modules/migrate_cache_counts_test/src/Plugin/migrate/source/CacheableEmbeddedDataSource.php
   @@ -0,0 +1,32 @@
   +class CacheableEmbeddedDataSource extends EmbeddedDataSource {
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function count($refresh = FALSE) {
   +    return SourcePluginBase::count($refresh);
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function doCount() {
   +    return parent::count(TRUE);
   +  }
   

   I'm a little confused with why we are statically calling SourcePluginBase::count() here. Perhaps an inline comment would clarify.

2. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationSourceCacheTest.php
   @@ -0,0 +1,122 @@
   +    // Verify correct counts are cached.
   +    $this->assertSame(1, $migration_1_source->count());
   +    $this->assertSame(2, $migration_2_source->count());
   

   This doesn't really verify caching as best as it could. However, we do have more testing of that later. Maybe just a comment clarification of what is really being tested is in order.

AFAICT this is a duplicate of #2751829: Default source plugin cache key is insufficiently unique?

Marked #2751829: Default source plugin cache key is insufficiently unique as a duplicate of this.

EDIT: cross-posted, hah!

+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -199,7 +200,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
-    $this->cacheKey = !empty($configuration['cache_key']) ? $configuration['cache_key'] : NULL;
+    $this->cacheKey = $configuration['cache_key'] ?? hash('sha256', Json::encode($configuration));
     $this->idMap = $this->migration->getIdMap();
     $this->highWaterProperty = !empty($configuration['high_water_property']) ? $configuration['high_water_property'] : FALSE;
 
@@ -450,10 +451,6 @@ public function count($refresh = FALSE) {
       return -1;
     }
 
-    if (!isset($this->cacheKey)) {
-      $this->cacheKey = hash('sha256', $this->getPluginId());
-    }

This is also going to result in cache collisions, because now the plugin ID is ignored entirely.

It should be cached by both plugin ID and its configuration.

Now … you might say … that in practice, migration definitions have plugin: <source PLUGIN ID>. But that's not actually necessary.

Furthermore, having a cache ID that is literally just a hash is going to be a DX nightmware.

So I propose to keep your logic but prefix it with the source plugin ID 🤓 WDYT?

So I propose to keep your logic but prefix it with the source plugin ID 🤓 WDYT?

See #3190815-28: Source count caching broken: impossible to enable source count caching for SqlBase-based source plugins (plus, unneeded cache I/O). 🤞

Cool!

This is a blocker for #2598670: Source count caching does not work for SQL sources

I think this closed now that #3190815: Source count caching broken: impossible to enable source count caching for SqlBase-based source plugins (plus, unneeded cache I/O) has landed. Changing to 9.1.x since that issue landed in that branch.