Problem/Motivation

We need an upgrade path for the D7 Aggregator module.

Remaining Tasks

Write migrations to handle the following:

  • Any variables maintained by aggregator need to be moved into configuration
  • Categories, feeds, and feed items need to be migrated

Aggregator's data structure in D7 seems to be very similar to what it was in D6, so it may be possible to reuse or generalize the existing code from the D6 upgrade path.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

svendecabooter’s picture

Assigned: Unassigned » svendecabooter
Status: Active » Needs review
FileSize
19.51 KB

Here is a patch that migrates Aggregator feeds and feed items to Drupal 7.
Includes tests and unit tests.

The source plugin that was used for the D6 migration, has been made generic so it works for both D6 & D7.
I'm not sure if other classes can be generalised, since the D6 table {aggregator_feed} contains 1 field less than the D7 & D8 variant.

phenaproxima’s picture

Status: Needs review » Needs work

Looks pretty good after a quick scan-through, but a few relatively minor issues:

  1. +++ b/core/modules/aggregator/migration_templates/d6_aggregator_item.yml
    @@ -3,7 +3,7 @@ label: Aggregator items
    -  plugin: d6_aggregator_item
    +  plugin: aggregator_item
    

    This will need to be codified in an update hook (aggregator_update_N().)

  2. +++ b/core/modules/aggregator/migration_templates/d7_aggregator_item.yml
    @@ -0,0 +1,24 @@
    +    source:
    +      - fid
    

    Nit: Should be source: fid.

  3. +++ b/core/modules/aggregator/src/Plugin/migrate/source/AggregatorItem.php
    @@ -0,0 +1,57 @@
    +    $query = $this->select('aggregator_item', 'ai')
    +      ->fields('ai', array('iid', 'fid', 'title', 'link', 'author',
    +        'description', 'timestamp', 'guid'))
    +      ->orderBy('iid');
    +    return $query;
    

    Two things -- first, let's make this a single return statement instead of doing a separate return $query; second, I'd rather we do fields('ai') instead of listing the fields individually.

  4. +++ b/core/modules/aggregator/src/Plugin/migrate/source/d7/AggregatorFeed.php
    @@ -0,0 +1,74 @@
    +    $query = $this->select('aggregator_feed', 'af')
    +      ->fields('af', array(
    +        'fid',
    +        'title',
    +        'url',
    +        'refresh',
    +        'checked',
    +        'queued',
    +        'link',
    +        'description',
    +        'image',
    +        'etag',
    +        'modified',
    +        'block',
    +      ));
    +
    +    $query->orderBy('fid');
    +    return $query;
    

    Same here -- fields('af') and make the entire thing a single statement.

  5. +++ b/core/modules/aggregator/src/Tests/Migrate/d7/MigrateAggregatorFeedTest.php
    @@ -0,0 +1,50 @@
    + * @group migrate_drupal_7
    

    Should be in the aggregator @group.

  6. +++ b/core/modules/aggregator/src/Tests/Migrate/d7/MigrateAggregatorFeedTest.php
    @@ -0,0 +1,50 @@
    +  static $modules = array('aggregator');
    

    Nit: public static

  7. +++ b/core/modules/aggregator/src/Tests/Migrate/d7/MigrateAggregatorFeedTest.php
    @@ -0,0 +1,50 @@
    +  public function testAggregatorFeedImport() {
    +    /** @var Feed $feed */
    

    The @var comment should have the fully-qualified name of the class or interface.

  8. +++ b/core/modules/aggregator/src/Tests/Migrate/d7/MigrateAggregatorItemTest.php
    @@ -0,0 +1,69 @@
    + * Upgrade aggregator items.
    + *
    + * @group migrate_drupal_7
    + */
    +class MigrateAggregatorItemTest extends MigrateDrupal7TestBase {
    

    @group aggregator, and the comment should say something like "Tests migration of aggregator items."

  9. +++ b/core/modules/aggregator/src/Tests/Migrate/d7/MigrateAggregatorItemTest.php
    @@ -0,0 +1,69 @@
    +  static $modules = array('aggregator');
    

    public static

  10. +++ b/core/modules/aggregator/src/Tests/Migrate/d7/MigrateAggregatorItemTest.php
    @@ -0,0 +1,69 @@
    +    // Add some id mappings for the dependant migrations.
    +    $id_mappings = array(
    +      'd7_aggregator_feed' => array(
    +        array(array(1), array(1)),
    +      ),
    +    );
    +    $this->prepareMigrations($id_mappings);
    +
    +    $entity = entity_create('aggregator_feed', array(
    +      'fid' => 1,
    +      'title' => 'Drupal Core',
    +      'url' => 'https://groups.drupal.org/not_used/167169',
    +      'refresh' => 900,
    +      'checked' => 1389919932,
    +      'description' => 'Drupal Core Group feed',
    +    ));
    +    $entity->enforceIsNew();
    +    $entity->save();
    

    Don't do this -- just call $this->executeMigration('d7_aggregator_feed'). I'm trying to move us away from the use of prepareMigrations().

  11. +++ b/core/modules/aggregator/src/Tests/Migrate/d7/MigrateAggregatorItemTest.php
    @@ -0,0 +1,69 @@
    +  public function testAggregatorItem() {
    +    /** @var Item $item */
    

    Again, the @var needs the fully qualified class name.

  12. +++ b/core/modules/aggregator/tests/src/Unit/Plugin/migrate/source/AggregatorItemTest.php
    @@ -0,0 +1,55 @@
    +  // The fake Migration configuration entity.
    +  protected $migrationConfiguration = array(
    +    // The ID of the entity, can be any string.
    +    'id' => 'test',
    +    'source' => array(
    +      'plugin' => 'aggregator_item',
    +    ),
    +  );
    

    The inline comments should be removed.

  13. +++ b/core/modules/aggregator/tests/src/Unit/Plugin/migrate/source/AggregatorItemTest.php
    @@ -0,0 +1,55 @@
    +  protected $databaseContents = array('aggregator_item' => array(array(
    +      'iid' => 1,
    +      'fid' => 1,
    +      'title' => 'This (three) weeks in Drupal Core - January 10th 2014',
    +      'link' => 'https://groups.drupal.org/node/395218',
    +      'author' => 'larowlan',
    +      'description' => "<h2 id='new'>What's new with Drupal 8?</h2>",
    +      'timestamp' => 1389297196,
    +      'guid' => '395218 at https://groups.drupal.org',
    +    ),
    +  ));
    

    Nit, but please format the value of $databaseContents to match other tests. Or, better yet: it looks like databaseContents and expectedResults are identical, so it's preferable to simply set $this->databaseContents['aggregator_item'] = $this->expectedResults in the setUp() method, rather than repeating code.

  14. +++ b/core/modules/aggregator/tests/src/Unit/Plugin/migrate/source/d7/AggregatorFeedTest.php
    @@ -0,0 +1,69 @@
    +    foreach ($this->expectedResults as $k => $row) {
    +      $this->databaseContents['aggregator_feed'][$k] = $row;
    +    }
    

    Should be $this->databaseContents['aggregator_feed'] = $this->expectedResults. No need for the foreach.

phenaproxima’s picture

I'm not sure if other classes can be generalised, since the D6 table {aggregator_feed} contains 1 field less than the D7 & D8 variant.

For a difference that minor, it's acceptable to use $this->getModuleSchemaVersion('system') in the source plugin to check which major of Drupal is being sourced. Generalizing FTW!

svendecabooter’s picture

Status: Needs work » Needs review
FileSize
21.55 KB

Attached a new version of the patch, with the comments by phenaproxima tackled, except for the first one.
I have no clue what code should go into the aggregator_update_N() hook... any pointers here?

I also made the AggregatorFeed source plugin more generic, so it can be used by both the D6 & D7 migration.

Perhaps unrelated to this patch, but I noticed that a bunch of the improvements suggested by phenaproxima (e.g. remark 6, 9, 10, 14, ...) are also in the D6 version of the aggregator migration code. Would it be within the scope of this issue to clean that up as well?

phenaproxima’s picture

I have no clue what code should go into the aggregator_update_N() hook... any pointers here?

Take a look at block_content_update_8002() -- it does much the same thing. Any migrate patches which change existing migration templates need to supply update hooks because we need to support the beta-to-beta update path that was recently introduced into core.

Perhaps unrelated to this patch, but I noticed that a bunch of the improvements suggested by phenaproxima (e.g. remark 6, 9, 10, 14, ...) are also in the D6 version of the aggregator migration code. Would it be within the scope of this issue to clean that up as well?

Yes, it's in scope. Clean all the things!

svendecabooter’s picture

Updated patch in attachment, containing:

* update hook for beta-to-beta update path, updating the source plugin consolidation in the D6 migration templates
* code cleanup in D6 aggregator migration files, based on feedback in #2

The last submitted patch, 4: upgrade_path_for-aggregator-d7-2500469-4.patch, failed testing.

svendecabooter’s picture

Assigned: svendecabooter » Unassigned
phenaproxima’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/aggregator/aggregator.install
    @@ -22,3 +22,31 @@ function aggregator_requirements($phase) {
    + * Generalizes the source plugin in d6_aggregator_feed & d6_aggregator_item
    + * migrations.
    

    Nit: s/&/and

  2. +++ b/core/modules/aggregator/aggregator.install
    @@ -22,3 +22,31 @@ function aggregator_requirements($phase) {
    +      $template = $templates->getTemplateByName('d6_aggregator_feed');
    +      $migration
    +        ->set('source', $template['source'])
    +        ->save(TRUE);
    

    This is too brute-force -- if the migration exists, it may have source DB credentials already embedded in it. Setting source to $template['source'] as we do here would overwrite that. If all we're doing is changing the plugin ID, let's do set('source.plugin', 'aggregator_feed').

  3. +++ b/core/modules/aggregator/aggregator.install
    @@ -22,3 +22,31 @@ function aggregator_requirements($phase) {
    +      $template = $templates->getTemplateByName('d6_aggregator_item');
    +      $migration
    +        ->set('source', $template['source'])
    +        ->set('process', $template['process'])
    +        ->save(TRUE);
    

    Ditto this. Better to make smaller changes. In this particular instance, the change to the process pipeline is unnecessary. It's OK if source: fid is a list; that was just a style thing. It will work either way.

  4. +++ b/core/modules/aggregator/src/Plugin/migrate/source/AggregatorFeed.php
    @@ -0,0 +1,62 @@
    +      $fields['queued'] = $$this->t('Time when this feed was queued for refresh, 0 if not queued.');
    

    $this, not $$this :)

  5. +++ b/core/modules/aggregator/src/Tests/Migrate/d6/MigrateAggregatorConfigsTest.php
    @@ -13,7 +13,7 @@
    - * @group migrate_drupal_6
    + * @group aggregator
    

    This should stay in the migrate_drupal_6 group. It's confusing as hell, but the Drupal 7 tests stay in their module group and the Drupal 6 tests go in migrate_drupal_6 so that they will be found by MigrateDrupal6Test (of which there is no Drupal 7 version, nor will there be).

  6. +++ b/core/modules/aggregator/src/Tests/Migrate/d6/MigrateAggregatorFeedTest.php
    @@ -11,13 +11,13 @@
    - * @group migrate_drupal_6
    + * @group aggregator
    

    Should be in the migrate_drupal_6 group. Sorry for the misunderstanding!

  7. +++ b/core/modules/aggregator/src/Tests/Migrate/d6/MigrateAggregatorItemTest.php
    @@ -11,13 +11,13 @@
    - * @group migrate_drupal_6
    + * @group aggregator
    

    Ditto.

  8. +++ b/core/modules/aggregator/src/Tests/Migrate/d7/MigrateAggregatorFeedTest.php
    @@ -0,0 +1,50 @@
    +    $this->assertNotNull($feed->uuid());
    

    Why are we asserting this?

svendecabooter’s picture

Status: Needs work » Needs review
FileSize
26.03 KB

Feedback taken into account in this new version of the patch.
Regarding #8: this was duplicated from the D6 version of MigrateAggregatorFeedTest. Should it be removed in both tests?

phenaproxima’s picture

Few small things, but other than that this is squarely RTBC.

  1. +++ b/core/modules/aggregator/aggregator.install
    @@ -22,3 +22,27 @@ function aggregator_requirements($phase) {
    +
    +/**
    + * Generalizes the source plugin in d6_aggregator_feed and d6_aggregator_item
    + * migrations.
    + */
    +function aggregator_update_8001() {
    

    I am so, so, so sorry for backpedaling on this, but it looks like we won't actually need this update hook after all. Discussing with @benjy, @catch and @alexpott, it looks like the migration system, being experimental in 8.0.0, will not need to support update paths yet. See #2569469: [Policy, no patch] No need to provide an upgrade path for Migrations.

  2. +++ b/core/modules/aggregator/src/Plugin/migrate/source/AggregatorFeed.php
    @@ -0,0 +1,62 @@
    +      ->orderBy('fid');
    

    I don't think we need to order by fid.

  3. +++ b/core/modules/aggregator/src/Tests/Migrate/d7/MigrateAggregatorFeedTest.php
    @@ -0,0 +1,50 @@
    +    $this->assertNotNull($feed->uuid());
    

    Yeah, we might as well remove this. It doesn't add any value that I'm aware of. If it's in the D6 test, feel free to kick it out of there as well :)

  4. +++ b/core/modules/aggregator/src/Tests/Migrate/d7/MigrateAggregatorFeedTest.php
    @@ -0,0 +1,50 @@
    +    $this->assertIdentical('0', $feed->modified->value);
    +  }
    +}
    

    Nit: Please add an extra line before the end of the class.

  5. +++ b/core/modules/aggregator/src/Tests/Migrate/d7/MigrateAggregatorItemTest.php
    @@ -0,0 +1,51 @@
    +    $this->assertIdentical('395218 at https://groups.drupal.org', $item->getGuid());
    +
    +  }
    

    Nit: There's an extra line.

svendecabooter’s picture

Assigned: Unassigned » svendecabooter
Status: Needs review » Needs work
svendecabooter’s picture

Updated patch to fix remarks by phenaproxima.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

This rocks the socks off the block. Whole-heartedly RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: upgrade_path_for-aggregator-d7-2500469-13.patch, failed testing.

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Testbot WTF.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

I'm sorry to say...

phenaproxima’s picture

Re-rolled.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

In the land of constant changes, the rerolled patch is king.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work

Er, wait -- somehow the re-rolled patch looks wrong. What the h-e-double hockey sticks?

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

OK, back to RTBC.

It's jarring, but the difference between #13 and the re-roll in #19 appears to be my Git behaving differently from @svendecabooter's. Git spins me right 'round. Like a record.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed ede8671 on 8.0.x
    Issue #2500469 by svendecabooter, phenaproxima: Migration path for...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

The last submitted patch, 13: upgrade_path_for-aggregator-d7-2500469-13.patch, failed testing.