Problem/Motivation

We need to provide an upgrade path from 7.x for the core Statistics module.

Remaining Tasks

We need migrations, with tests, to cover:

  • Any variables maintained by Statistics need to be moved into configuration
  • The contents of the node_counter table may need to be migrated

Comments

quietone’s picture

Status: Active » Needs work
FileSize
894 bytes

Just making a start.

phenaproxima’s picture

@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.

quietone’s picture

Yea, I saw that after I posted and I was just leaving for the day. Not sure about node_counter table.

hussainweb’s picture

Status: Needs work » Postponed
Related issues: +#1446932: Improve statistics performance by adding a swappable backend
FileSize
3.41 KB

I started working on this and realized it would be much easier if there was a service backing this. This is currently being handled in #1446932: Improve statistics performance by adding a swappable backend. I am postponing this issue on that and posting a patch I started with. Once that is in, we can continue working on the patch.

quietone’s picture

Issue tags: +migrate-d7-d8

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

Status: Postponed » Active

No longer postponed.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

DamienMcKenna’s picture

Adding the main D7-D8 issue as the parent issue.

DamienMcKenna’s picture

Status: Active » Needs review

Lets nudge testbot to take a look at the patch (I'm sure it'll fail, but it's good to follow the standard process).

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Self-assigning for review.

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

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.

quietone’s picture

There doesn't seem to be a d6 migration for this. Can that be included here?

Jo Fitzgerald’s picture

FileSize
3.96 KB
3.68 KB

Let's move this on a step.

Done: 3, 4, 5, 6, 7
To do: 1, 2

Still needs tests so no point in running past the testbot.

quietone’s picture

Title: Upgrade path for Statistics 7.x » Upgrade path for Statistics 7.x and 6.x
Issue tags: +migrate-d6-d8

On the last migrate call we thought that d6 and d7 statistics were pretty much the same, so marking this for d6 as well.

Tytest’s picture

I originally noted the loss of statistics during my migration. Can anyone tell me how to copy over the views count directly from the old D6 Database to D8 manually?

Jo Fitzgerald’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
10.01 KB
12.97 KB
  • Extended to also handle D6.
  • Added tests.

I'm still not sure how to address #13.1 & #13.2

Status: Needs review » Needs work

The last submitted patch, 18: 2500521-18.patch, failed testing.

mikeryan’s picture

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.

Don't have time to dig it up at the moment, but I seem to recall looking at a similar scenario and finding that the derivatives are automatically expanded... somewhere.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
3 KB
13.01 KB

Moved NodeCounterTest.php to the correct location.

While debugging through the code I can confirm that it does 'magically' expand the derivatives, but don't ask me why/how!

heddn’s picture

Issue tags: +Needs manual testing

Not necessarily critical if someone can dig up where the derivatives get expanded, but another way to prove this is by running a manual test. Tagging and marking Novice. To test this, attempt to migrate d6 of d7 statistics data into D8 and see if the numbers for nodes come over correctly.

heddn’s picture

Issue tags: +Novice

*Now* adding the novice tag.

dexter42’s picture

Hi... one note - when there are translated nodes (e.g. English and German), there are two nodes in D6, but only one node in D8. I assume the statistics have to be summed up.

dexter42’s picture

Btw, in the destination plugin in import function should be following return statement:

return [$row->getDestinationProperty('nid')];

otherwise such row will be marked as failure.... As I have in the meantime this patch heavily customized to make workaround for my translations, I am unable to provide patch.

phenaproxima’s picture

Status: Needs review » Needs work

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.

gaurav.kapoor’s picture

Status: Needs work » Needs review
FileSize
12.98 KB
2.42 KB

Fixed 27.1,27.4,27.5,27.6,27.7 and 27.8.

phenaproxima’s picture

Status: Needs review » Needs work

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 */

quietone’s picture

Status: Needs work » Needs review
FileSize
13.08 KB
5.44 KB

Thanks all. Just some little bits left to do.

Fixed 27.2, 27.3, 27.5, 27.7 and #26. Also, I was getting schema errors when running the migration tests something about node.article.menu_ui. Adding 'menu_ui' to the list of modules fixed that. I think that is everything.

Status: Needs review » Needs work

The last submitted patch, 30: 2500521-30.patch, failed testing.

quietone’s picture

Status: Needs work » Postponed
Related issues: +#2569805: For Drupal migration, identify the source module
exception: [Notice] Line 1054 of core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php:
Undefined index: d6_statistics_node_counter

And the same for D7.

This is failing because the new migrations have not been added to the big array of migrations in MigrateUpgradeForm. That array is being removed in #2569805: For Drupal migration, identify the source module. We could reroll here, but then we need to reroll there. No fun in that. So, I am going postpone this on that issue, which will solve the problem of having to maintain that large array of migrations.