Derive statistics module migrations per node type

Looking great! I basically only have nits:

1. +++ b/core/modules/statistics/src/Plugin/migrate/StatisticsNodeCounterDeriver.php
   @@ -0,0 +1,59 @@
   +/**
   + * Deriver for Drupal 7 statistics node counter migrations based on node types.
   + */
   +class StatisticsNodeCounterDeriver extends DeriverBase {
   

   Could use an

   @see \Drupal\node\Plugin\migrate\D7NodeDeriver
   

   to make it clear we're matching the behavior there.

2. +++ b/core/modules/statistics/src/Plugin/migrate/StatisticsNodeCounterDeriver.php
   @@ -0,0 +1,59 @@
   +    catch (DatabaseExceptionWrapper $e) {
   +    }
   

   Let's copy the comment from \Drupal\node\Plugin\migrate\D7NodeDeriver that justifies this catching-without-acting-on-it:

         // Once we begin iterating the source plugin it is possible that the
         // source tables will not exist. This can happen when the
         // MigrationPluginManager gathers up the migration definitions but we do
         // not actually have a Drupal 7 source database.
   

3. +++ b/core/modules/statistics/src/Plugin/migrate/source/NodeCounter.php
   @@ -14,11 +14,18 @@
   +  const JOIN = '[nc].[nid] = [n].[nid]';
   

   Interesting pattern!

   I don't think anything in Drupal core uses this.

   If it's a new const, it's going to need to be documented.

   I think that in this case it'd be simpler to just remove the const and put the string directly in the innerJoin() parameter?

4. +++ b/core/modules/statistics/src/Plugin/migrate/source/NodeCounter.php
   @@ -38,6 +45,9 @@ public function fields() {
   +    if (isset($this->configuration['node_type'])) {
   +      $ids['nid']['alias'] = 'n';
   +    }
   

   Hm … interesting!

   While this is not wrong, I do think this unnecessarily complex: there's nothing wrong with always returning the alias here. That's what all other @MigrateSource plugins in Drupal core do! 😊

5. +++ b/core/modules/statistics/tests/src/Kernel/Migrate/d6/MigrateNodeCounterTest.php
   @@ -61,7 +61,7 @@ public function testStatisticsSettings() {
   -    $this->executeMigration('statistics_node_translation_counter');
   +    $this->executeMigrations(['statistics_node_translation_counter']);
   

   For future reviewers/committers: this is to ensure that the derived migrations get executed.

   (Yes, this is weird. But it's pre-existing weirdness in the migration system's test infrastructure, so out of scope here to fix/improve.)

1. +++ b/core/modules/statistics/src/Plugin/migrate/StatisticsNodeCounterDeriver.php
   @@ -0,0 +1,59 @@
   +    $source = static::getSourcePlugin($base_plugin_definition['source']['plugin']);
   +    assert($source instanceof DrupalSqlBase);
   ...
   +      $node_types_used = $source->getDatabase()->select('node_counter', 'nc')
   +        ->fields('n', ['type'])
   +        ->groupBy('n.type');
   +      $node_types_used->join('node', 'n', '[n].[nid] = [nc].[nid]');
   +      $node_types = array_keys(
   +        $node_types_used
   +          ->execute()
   +          ->fetchAllAssoc('type', \PDO::FETCH_ASSOC)
   

   If you get the node_counter source plugin, then you will be able to use its "base" query:

   $source = static::getSourcePlugin('node_counter');
   ...
   $used_node_types_query = $source->query()
     ->fields('n', ['type'])
     ->groupBy('n.type');
   $used_node_types_query->join('node', 'n', '[n].[nid] = [nc].[nid]');
   

2. +++ b/core/modules/statistics/src/Plugin/migrate/StatisticsNodeCounterDeriver.php
   @@ -0,0 +1,59 @@
   +          ->fetchAllAssoc('type', \PDO::FETCH_ASSOC)
   ...
   +        $derivative_definition['migration_dependencies']['optional'][$dependency_index] .= ":$node_type";
   

   $dependency_index might be FALSE when array_search does not find anything. Before adding the node type derivative ID, you should check that $dependency_index is not FALSE.

3. +++ b/core/modules/statistics/src/Plugin/migrate/StatisticsNodeCounterDeriver.php
   @@ -0,0 +1,59 @@
   +    return $this->derivatives;
   +
   +  }
   

   Remove this empty line.

4. +++ b/core/modules/statistics/src/Plugin/migrate/source/NodeCounter.php
   @@ -38,6 +45,9 @@ public function fields() {
   +    if (isset($this->configuration['node_type'])) {
   +      $ids['nid']['alias'] = 'n';
   +    }
   

   If you use 'nc' as alias, then you don't need this condition. (Was mentioned in #9.4.)

Re #9.5:
Actually, that executes all derivative migrations of statistics_node_translation_counter, and imho it isn't a weirdness. Simply, MigrateTestBase::executeMigration() executes the migration whose (full) ID matches, while MigrateTestBase::executeMigrations() discovers every (possible) derivatives as well, because it uses $manager->createInstances($id); internally.

@narendraR, thanks for improving the migration system.

Moving to migration system and tagging for an IS update.

I think @quietone wanted to do this :)

Can you please also post the interdiff?

No more remarks. Thoroughly tested, works great! 👍

I think @quietone wanted to do this :)

Well, by the standard benchmark that we only add what is needed for the core migrations one we could argue that this is not needed in core. However, there is a migrate meeting tomorrow at a time when I can attend. I will ask there.

+++ b/core/modules/statistics/src/Plugin/migrate/StatisticsNodeCounterDeriver.php
@@ -0,0 +1,66 @@
+      // MigrationPluginManager gathers up the migration definitions but we do

This is minor. Instead of 'gathers up' use 'discovered'.

Seeing the change in getIds got me thinking about the effect of this on existing sites. When this patch is applied any existing statistics_node_counter (not configured) will still run but the results will go to new tables. That means there will be a map/message pair of tables that are no longer accessible via drush. We should probably mention that in a CR.

I didn't make it to the migrate meeting in time but I did check with the other maintainers. Not surprising, mikelutz was quite clear that this is not needed in core (where the core migrations support the 1 click UI upgrade system), that it is not solving any known bug in core and adds complication.

Like him, I recall the decision being made that adding derivers would be done in contrib, probably migrate_upgrade. That would make this a won't fix.

Setting to NR to allow for comments.

That would make this a won't fix...Setting to NR to allow for comments.

That was from @quietone about 2 months ago. Is it time to close this?

Yes, it need to be closed and opened as an issue in migrate upgrade. It poses no benefits to core migrate.

fix to avoid unnecessary derivatives.