i18n / statistics - node counter not updated for translations

Attaching this to the meta-issue for translated node IDs.

Also postponing on #2912348: Handle entity_references related to Drupal 6 and 7 node translations with different IDs since that will add infrastructure and a pattern that this issue can use.

Postponing on #2912348: Handle entity_references related to Drupal 6 and 7 node translations with different IDs as discussed at the migrate maintainers meeting.

I doesn't seems like this issue needs to be postponed on #2912348: Handle entity_references related to Drupal 6 and 7 node translations with different IDs since it can be done without a follow-up migration.

1. +++ b/core/modules/statistics/migrations/statistics_node_counter_translation.yml
   @@ -0,0 +1,34 @@
   +id: statistics_node_counter_translation
   +label: Node counter translations
   

   I think we should rephrase these a little; IMHO, "node translation counter" makes more sense than "node counter translation", since the counter itself is not being translated in any way.

2. +++ b/core/modules/statistics/src/Plugin/migrate/destination/NodeCounter.php
   @@ -81,15 +81,41 @@ public function fields(MigrationInterface $migration = NULL) {
   +    if (!$result) {
   +      $this->connection
   +        ->insert('node_counter')
   +        ->fields([
   +          'nid' => $nid,
   +          'daycount' => $daycount,
   +          'totalcount' => $totalcount,
   +          'timestamp' => $timestamp,
   +        ])
   +        ->execute();
   +    }
   +    else {
   +      $this->connection
   +        ->update('node_counter')
   +        ->fields([
   +          'nid' => $nid,
   +          'daycount' => $result['daycount'] + $daycount,
   +          'totalcount' => $result['totalcount'] + $totalcount,
   +          'timestamp' => $timestamp > $result['timestamp'] ? $timestamp : $result['timestamp'],
   +        ])
   +        ->condition('nid', $nid)
   +        ->execute();
   +    }
   

   It's too bad that Statistics doesn't provide an API for this, but I suppose that's not a big problem as long as there is test coverage of both the insert and update cases.

I think #8.2 is an observation. Here's a quick fix to 8.1.

Re #8.2: I updated the tests to make it more obvious that the node counters are updated by the new migration after being inserted by the first migration.

(I also renamed the migration yml to reflect the migration id)

+++ b/core/modules/statistics/src/Plugin/migrate/destination/NodeCounter.php
@@ -81,15 +81,41 @@ public function fields(MigrationInterface $migration = NULL) {
+    if (!$result) {
+      $this->connection
+        ->insert('node_counter')
+        ->fields([
+          'nid' => $nid,
+          'daycount' => $daycount,
+          'totalcount' => $totalcount,
+          'timestamp' => $timestamp,
+        ])
+        ->execute();
+    }
+    else {
+      $this->connection
+        ->update('node_counter')
+        ->fields([
+          'nid' => $nid,
+          'daycount' => $result['daycount'] + $daycount,
+          'totalcount' => $result['totalcount'] + $totalcount,
+          'timestamp' => $timestamp > $result['timestamp'] ? $timestamp : $result['timestamp'],
+        ])
+        ->condition('nid', $nid)
+        ->execute();
+    }

We could combine these two queries into a single merge query, to DRY things up a bit. But that's a total fundamentalist nitpick, so feel free to disregard.

Otherwise, I don't see anything to object to. To paraphrase Mortal Kombat, let's finish this.

+++ b/core/modules/statistics/src/Plugin/migrate/destination/NodeCounter.php
@@ -81,15 +81,41 @@ public function fields(MigrationInterface $migration = NULL) {
+    $result = $this->connection
+      ->select('node_counter', 'nc')
+      ->fields('nc')
+      ->condition('nid', $nid)
+      ->execute()
+      ->fetchAssoc();
+
+    if (!$result) {
+      $this->connection
+        ->insert('node_counter')
+        ->fields([
+          'nid' => $nid,
+          'daycount' => $daycount,
+          'totalcount' => $totalcount,
+          'timestamp' => $timestamp,
+        ])
+        ->execute();
+    }
+    else {
+      $this->connection
+        ->update('node_counter')
+        ->fields([
+          'nid' => $nid,
+          'daycount' => $result['daycount'] + $daycount,
+          'totalcount' => $result['totalcount'] + $totalcount,
+          'timestamp' => $timestamp > $result['timestamp'] ? $timestamp : $result['timestamp'],
+        ])
+        ->condition('nid', $nid)
+        ->execute();
+    }

This whole thing should be a single merge query.

Back to RTBC.

Crediting @phenaproxima for reviews and @catch and @masipila for issue management.

Committed and pushed d45dc9d26a to 8.6.x and 63210e8ef3 to 8.5.x. Thanks!

Does this fully resolve the known issues in the handbook on upgrading Statistics from D6/7 to D8? If so, should those sections be removed, or should it wait till the commit ships?

Will be included in 8.5.2. Drupal 8.5.1 is only security fix.