Upgrade path for Statistics 7.x and 6.x

@quietone: There is already a migration in progress for the statistics settings (see the child issue), but we do need a migration for the node_counter table, if you're up for it.

Self-assigning for review.

This is not ready for prime time. Sorry, y'all!

1. +++ b/core/modules/statistics/migration_templates/d7_statistics_node_counter.yml
   @@ -0,0 +1,19 @@
   +  nid:
   +    plugin: migration
   +    migration: d7_node
   +    source: nid
   

   Two observations here. First, I'm not sure it will work if we just pass d7_node as the migration, seeing as how it has a deriver and encompasses many migrations, one per node type. We should be sure this will look in every d7_node derivative.

   Second, if the migration plugin fails, it will return NULL. We should add an additional process plugin here that skips the row if nid is empty.

2. +++ b/core/modules/statistics/migration_templates/d7_statistics_node_counter.yml
   @@ -0,0 +1,19 @@
   +migration_dependencies:
   +  required:
   +    - d7_node
   

   Again, we need to be sure this is respecting all d7_node derivatives.

3. +++ b/core/modules/statistics/src/Plugin/migrate/destination/NodeCounter.php
   @@ -0,0 +1,85 @@
   +use Drupal\ban\BanIpManagerInterface;
   

   Wat. We're not using this for anything.

4. +++ b/core/modules/statistics/src/Plugin/migrate/destination/NodeCounter.php
   @@ -0,0 +1,85 @@
   + * Destination for blocked IP addresses.
   

   No it's not! :)

5. +++ b/core/modules/statistics/src/Plugin/migrate/destination/NodeCounter.php
   @@ -0,0 +1,85 @@
   +  /**
   +   * Constructs a BlockedIP object.
   +   *
   +   * @param array $configuration
   +   *  Plugin configuration.
   +   * @param string $plugin_id
   +   *  The plugin ID.
   +   * @param mixed $plugin_definition
   +   *  The plugin definiiton.
   +   * @param \Drupal\migrate\Entity\MigrationInterface $migration
   +   *  The current migration.
   +   */
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration) {
   +    parent::__construct($configuration, $plugin_id, $plugin_definition, $migration);
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration = NULL) {
   +    return new static(
   +      $configuration,
   +      $plugin_id,
   +      $plugin_definition,
   +      $migration
   +    );
   +  }
   

   I don't think we need any of this; we're not injecting any dependencies that I can see. We probably also don't need ContainerFactoryPluginInterface either, in that case.

6. +++ b/core/modules/statistics/src/Plugin/migrate/destination/NodeCounter.php
   @@ -0,0 +1,85 @@
   +      'nid' => $this->t('The nid of the node for these statistics.'),
   

   Should be 'the ID of the node to which these statistics apply'.

7. +++ b/core/modules/statistics/src/Plugin/migrate/destination/NodeCounter.php
   @@ -0,0 +1,85 @@
   +  public function import(Row $row, array $old_destination_id_values = array()) {
   +    $nid = $row->getDestinationProperty('nid');
   +    $totalcount = $row->getDestinationProperty('totalcount');
   +    $daycount = $row->getDestinationProperty('daycount');
   +    $timestamp = $row->getDestinationProperty('timestamp');
   +
   +    // @todo Save the statistics.
   

   So...we're not saving anything?

Also, there be no tests in them thar hills.

Looks damn near perfect to me. Just a few nits and it's RTBC once my and @dexter42's comments are addressed.

1. +++ b/core/modules/statistics/src/Plugin/migrate/destination/NodeCounter.php
   @@ -0,0 +1,99 @@
   +  public function import(Row $row, array $old_destination_id_values = array()) {
   

   $old_destination_id_values should be [], not array().

2. +++ b/core/modules/statistics/src/Plugin/migrate/destination/NodeCounter.php
   @@ -0,0 +1,99 @@
   +      ->fields([
   +        'nid',
   +        'daycount',
   +        'totalcount',
   +        'timestamp',
   +      ], [
   +        $row->getDestinationProperty('nid'),
   +        $row->getDestinationProperty('daycount'),
   +        $row->getDestinationProperty('totalcount'),
   +        $row->getDestinationProperty('timestamp'),
   +      ])
   

   I think I'd rather this were passed as a single associative array, where the database columns are the keys.

3. +++ b/core/modules/statistics/src/Plugin/migrate/source/NodeCounter.php
   @@ -0,0 +1,43 @@
   + * @MigrateSource(
   + *   id = "node_counter",
   + * )
   

   Should a source provider maybe be mentioned here?

4. +++ b/core/modules/statistics/tests/src/Kernel/Migrate/d6/MigrateNodeCounterTest.php
   @@ -0,0 +1,76 @@
   + * Upgrade node counter.
   

   Should say "Tests the migration of node counter data to Drupal 8."

5. +++ b/core/modules/statistics/tests/src/Kernel/Migrate/d6/MigrateNodeCounterTest.php
   @@ -0,0 +1,76 @@
   +    /** @var StatisticsViewsResult $statistics */
   

   This class name should be fully qualified, which means we can drop the use statement at the top of the file.

6. +++ b/core/modules/statistics/tests/src/Kernel/Migrate/d7/MigrateNodeCounterTest.php
   @@ -0,0 +1,72 @@
   + * Upgrade node counter.
   

   Should be rephrased as "Tests the migration of node counter data to Drupal 8."

7. +++ b/core/modules/statistics/tests/src/Kernel/Migrate/d7/MigrateNodeCounterTest.php
   @@ -0,0 +1,72 @@
   +    /** @var StatisticsViewsResult $statistics */
   

   This should be fully qualified, dropping the use statement at the top of the file.

8. +++ b/core/modules/statistics/tests/src/Kernel/Plugin/migrate/source/NodeCounterTest.php
   @@ -0,0 +1,66 @@
   + * @covers \Drupal\statistics\Plugin\migrate\source\NodeCounter
   + * @group statistics
   

   I believe the coding standards require an empty line between these two annotations.

Thanks, @gaurav.kapoor! Looks good. I'm returning this to NW until the rest of the points are addressed, but it's definitely getting there.

#28.5 and #28.7 are not quite done yet -- the class name in the comment must be the fully qualified class name. So,

/** @var StatisticsViewsResult $statistics */

needs to be changed to:

/** @var \Drupal\statistics\StatisticsViewsResult $statistics */

Looks very good. Really nice work -- I see nothing overly wrong here!

1. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal6.php
   --- /dev/null
   +++ b/core/modules/statistics/migration_templates/d6_statistics_node_counter.yml
   

   Core migrations must now live in the migrations directory, not migration_templates.

2. +++ b/core/modules/statistics/migration_templates/d6_statistics_node_counter.yml
   @@ -0,0 +1,23 @@
   +id: d6_statistics_node_counter
   

   I think we could combine the d6_statistics_node_counter and d7_statistics_node_counter migrations into a single one. All we'd need to do is:

   1. The 'migration' parameter for the nid lookup needs to be [d6_node, d7_node].
   2. The migration dependencies should be optional, not required, and list both d6_node and d7_node.
   3. Tag the migration with both 'Drupal 6' and 'Drupal 7'.

   If this is insane, then we don't need to do it. Just suggesting it.

3. +++ b/core/modules/statistics/migration_templates/d6_statistics_node_counter.yml
   @@ -0,0 +1,23 @@
   +      plugin: migration
   

   The 'migration' plugin is now 'migration_lookup'.

4. +++ b/core/modules/statistics/src/Plugin/migrate/destination/NodeCounter.php
   @@ -0,0 +1,96 @@
   +    $this->connection
   +      ->insert('node_counter')
   +      ->fields([
   +        'nid' => $row->getDestinationProperty('nid'),
   +        'daycount' => $row->getDestinationProperty('daycount'),
   +        'totalcount' => $row->getDestinationProperty('totalcount'),
   +        'timestamp' => $row->getDestinationProperty('timestamp'),
   +      ])
   +      ->execute();
   

   Does Statistics provide a service, or API function, we could use for this? Rather than directly writing to the database? If so, that'd be preferable.

5. +++ b/core/modules/statistics/tests/src/Kernel/Migrate/d6/MigrateNodeCounterTest.php
   @@ -0,0 +1,76 @@
   +    $this->assertEquals($total_count, $statistics->getTotalCount());
   +    $this->assertEquals($day_count, $statistics->getDayCount());
   +    $this->assertEquals($timestamp, $statistics->getTimestamp());
   

   Let's use assertSame() here instead.

6. +++ b/core/modules/statistics/tests/src/Kernel/Migrate/d7/MigrateNodeCounterTest.php
   @@ -0,0 +1,72 @@
   +    $this->assertEquals($total_count, $statistics->getTotalCount());
   +    $this->assertEquals($day_count, $statistics->getDayCount());
   +    $this->assertEquals($timestamp, $statistics->getTimestamp());
   

   Same here.

@phenaproxima I would be interested to hear your opinion on the best solution to this?

Counts are integers by definition. It makes no sense for a count of any kind to be a string -- the inconsistency is usually due to PHP's loose typing rules, and the database drivers not bothering to cast return values, since they don't necessarily have any idea what type of data is contained in various columns.

So, if we're dealing with counts, we should always cast them to integers, and use assertSame().

My initial feeling about them is that the messages are incorrect, and that they should say "module not found" when the assertion fails rather than "module found".

My understanding, which may be inaccurate, is that they can be either. They're only displayed if an assertion fails anyway. So if the message is positive, you'll know what failed; and, if it's negative, you'll know what failed. It's just a difference of phrasing -- your choice! :)

Really looking fantastic. Only a couple more points, then this is completely ready to go.

1. +++ b/core/modules/statistics/migrations/statistics_node_counter.yml
   @@ -0,0 +1,25 @@
   +label: Drupal 6 Node Counter
   

   Since this migration is generic for D6 and D7, it should probably just be "Node counter".

2. +++ b/core/modules/statistics/src/StatisticsViewsResult.php
   @@ -23,9 +23,9 @@ class StatisticsViewsResult {
      public function __construct($total_count, $day_count, $timestamp) {
   -    $this->totalCount = $total_count;
   -    $this->dayCount = $day_count;
   -    $this->timestamp = $timestamp;
   +    $this->totalCount = (int) $total_count;
   +    $this->dayCount = (int) $day_count;
   +    $this->timestamp = (int) $timestamp;
   

   This is out of scope. I know we need to do it in order to get our tests to pass, but I think we can simply cast the value *in the tests*, rather than here. Let's revert this part of the patch, then open separate follow-up to make this fix in Statistics by itself.

Nice one. Thanks everybody!