Suggested commit message:

Issue #2213451 by andypost, bdone, benjy, penyaskito, chx, claudiu.cristea, damiankloip, gregboggs, InternetDevels, jessehs, jhedstrom, marvil07, mikeryan, pcambra, Xano, YesCT: Update Migrate API in core

Problem/Motivation

This patch brings everything in the migrate module up-to-date and incudes any changes and dependent files from migrate_drupal. Our plan is to have this API committed and then create separate issues for Drupal 6/7 to Drupal 8 migrations which are in the migrate_drupal module.

The patch was generated from the IMP sandbox with the following commands:

git diff upstream/8.x core/modules/migrate > 2208061_16.patch
git diff --diff-filter=M upstream/8.x core/modules/migrate_drupal >> filename.patch
git diff upstream/8.x -- `cat d6_files` >> filename.patch

d6_files.txt is attached, these are new/deleted files and they are necessary to get the changed files in migrate_drupal to pass the tests.

Remaining tasks

  1. Get this in.
  2. Submit a patch including all the D6 to D8 migrations. We are finished with this one (sneak peek) and that gives us confidence in the changes submitted here.
  3. Submit a patch including all the D7 to D8 migrations, just begun work. The data storage changed little compared Drupal 6+CCK and the completed Drupal 6 work includes CCK (and Profile) which gives us confidence that we will be able to tackle Field API easily.
  4. In parallel, create an UI in migrate_drupal. It is ~80% complete. Expected to be completed by the time this gets in. Another UI is expected in contrib.

User interface changes

This patch does not provide any UI changes. The main UI will be provided by migrate_drupal. It is being worked on.

API changes

The following API additions/changes are included:

  1. A dependency system supporting both soft and hard dependencies.
  2. A mechanism for terminating a process pipeline early
  3. A mechanism for skipping a row during process
  4. New entity + entity revision destination plugins and derivatives
  5. Every other destination plugin necessary (I can think of UrlAlias off head)
  6. A proxying password service for migrating MD5'd user passwords. This is borderline migrate_drupal material but we thought it was generic enough to be mainlined.
  7. The source and destination IDs are now in the plugins and not the migrations.
  8. New annotations.
  9. Countless cleanups, minor fixes, doxygen fixes etc
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

First bit: Note: Creating such big patches is a pain for everyone using dreditor. It is nearly impossible to use, sorry.

  1. +++ b/core/modules/migrate/lib/Drupal/migrate/Annotation/MigrateDestination.php
    @@ -0,0 +1,46 @@
    +
    +  /**
    +   * A class to make the plugin derivative aware.
    +   *
    +   * @var string
    +   *
    +   * @see \Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator
    +   */
    +  public $derivative;
    +
    

    Is there a special reason to not put that into the Plugin base annotation? Each plugin type potentially has derivatives

  2. +++ b/core/modules/migrate/lib/Drupal/migrate/Annotation/MigrateSource.php
    @@ -0,0 +1,33 @@
    +
    ...
    +/**
    + * Defines a migration destination plugin annotation object.
    + *
    + * @Annotation
    + */
    +class MigrateSource extends Plugin {
    
    +++ b/core/modules/migrate/lib/Drupal/migrate/Entity/Migration.php
    @@ -191,21 +177,18 @@ class Migration extends ConfigEntityBase implements MigrationInterface {
    +   * These migrations must be already executed before this migration can run.
    ...
    +  protected $requirements = array();
    +
    +  /**
    +   * These migrations, if ran at all, must be executed before this migration.
    +   *
    +   *@var array
    +   */
    +  public $dependencies = array();
    

    I wonder whether we can improve the variable name to figure out this difference.

  3. +++ b/core/modules/migrate/lib/Drupal/migrate/Entity/Migration.php
    @@ -301,11 +284,56 @@ protected function getHighWaterStorage() {
    +      $required_migrations = entity_load_multiple('migration', $this->requirements);
    

    Let's use at least \Drupal::entityManager now. There is simply no reason to not do that now but require another followup.

  4. +++ b/core/modules/migrate/lib/Drupal/migrate/Entity/Migration.php
    @@ -301,11 +284,56 @@ protected function getHighWaterStorage() {
    +    catch (\Exception $e) {
    +      return FALSE;
    +    }
    

    Do we really want to catch all exceptions? This could be problematic

  5. +++ b/core/modules/migrate/lib/Drupal/migrate/Entity/MigrationInterface.php
    @@ -84,10 +84,17 @@ public function getDestinationPlugin();
    +  /**
    +   * Save the highwater mark.
    +   *
    +   * @return int
    +   */
       public function saveHighwater($highwater);
     
    

    If we update the docs we should do it properly. No idea what $highwater should be, and what the returned int could be.

  6. +++ b/core/modules/migrate/lib/Drupal/migrate/MigrateExecutable.php
    @@ -85,6 +86,33 @@ class MigrateExecutable {
    +  protected $memoryThreshold = 0.85;
    

    I do like that this is moved away from the migration class but we maybe should remove the documentation that you can replace it now.

  7. +++ b/core/modules/migrate/lib/Drupal/migrate/MigrateExecutable.php
    @@ -85,6 +86,33 @@ class MigrateExecutable {
    +  /**
    +   * The time limit when executing the migration.
    +   *
    +   * @var array
    +   */
    +  public $limit = array();
    

    Wait: Time exists in multiple dimensions? ;)

  8. +++ b/core/modules/migrate/lib/Drupal/migrate/MigratePassword.php
    @@ -0,0 +1,84 @@
    +/**
    + * Replaces the original 'password' service in order to prefix the MD5 re-hashed
    + * passwords with the 'U' flag. The new salted hash is recreated on first login
    + * similarly to the D6->D7 upgrade path.
    + */
    +class MigratePassword implements PasswordInterface {
    

    Interesting usage!!

  9. +++ b/core/modules/migrate/lib/Drupal/migrate/MigratePassword.php
    @@ -0,0 +1,84 @@
    +   * {@inheritdoc}
    +   */
    +  public function hash($password) {
    +    $hash = $this->originalPassword->hash($password);
    +
    +    // Allow prefixing only if the service was asked to prefix. Check also if
    +    // the $password pattern is conforming to a MD5 result.
    +    if ($this->enabled && preg_match('/^[0-9a-f]{32}$/', $password)) {
    +      $hash = 'U' . $hash;
    +    }
    +
    +    return $hash;
    +  }
    
    +++ b/core/modules/migrate/lib/Drupal/migrate/MigrateServiceProvider.php
    @@ -0,0 +1,30 @@
    +   */
    +  public function alter(ContainerBuilder $container) {
    +    $container->setDefinition('password_original', $container->getDefinition('password'));
    +    $container->setDefinition('password', $container->getDefinition('password_migrate'));
    +  }
    

    This sould really break in case someone decided to write their own totally different password service. Additional I don't know why this is part of migrate not migrate_drupal.

  10. +++ b/core/modules/migrate/lib/Drupal/migrate/MigrationStorageController.php
    @@ -0,0 +1,97 @@
    +    /** @var \Drupal\migrate\Entity\MigrationInterface $migration */
    +    foreach ($migrations as $migration) {
    

    We could switch those two lines to have the docs in a bit better context place.

  11. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/MigrateDestinationPluginManager.php
    @@ -0,0 +1,67 @@
    +  /**
    +   * An associative array where the keys are the enabled modules and themes.
    +   *
    +   * @var array
    +   */
    +  protected $providers;
    +
    +  /**
    

    Can't really imagine that there are usecases for themes, but yeah you probably never know.

  12. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/MigrateEntityDestinationFieldInterface.php
    @@ -0,0 +1,28 @@
    + * @file
    + * Contains
    

    Nihilistic documentation

  13. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/MigrateProcessPluginManager.php
    @@ -0,0 +1,42 @@
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function createInstance($plugin_id, array $configuration = array(), MigrationInterface $migration = NULL) {
    +    $index = serialize($configuration);
    +    if (!isset($this->storage[$migration->id()][$plugin_id][$index])) {
    +      $this->storage[$migration->id()][$plugin_id][$index] = parent::createInstance($plugin_id, $configuration, $migration);
    +    }
    +    return $this->storage[$migration->id()][$plugin_id][$index];
    +  }
    +
    +}
    

    What is the reason we do that here? This could/should be explained.

  14. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/ComponentEntityDisplayBase.php
    @@ -0,0 +1,72 @@
    +    // TODO: Implement fields() method.
    

    Given that this is the storm default text i wonder whether this was intended.

  15. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/Config.php
    @@ -50,14 +64,19 @@ public static function create(ContainerInterface $container, array $configuratio
       /**
    +   * Throw an exception because config can not be rolled back.
    +   *
        * @param array $destination_keys
    +   *   The array of destination ids to roll back.
        *
        * @throws \Drupal\migrate\MigrateException
        */
    @@ -66,22 +85,17 @@ public function rollbackMultiple(array $destination_keys) {
    

    These kind of documentation is really helpful. Thank you!

chx’s picture

> Is there a special reason to not put that into the Plugin base annotation? Each plugin type potentially has derivatives

we wanted to establish the requirements check in the annotation and the class Drupal\Component\Annotation\Plugin does not have $derivative on it.

> What is the reason we do that here? This could/should be explained.

The original intent was... to avoid re-instantiating the same class over and over again, plugins are usually stored in migrate entity properties. However, because of the iterator plugin and stubbing the getProcessPlugin takes a $process array argument. Either we could hash that array and store the resulting process plugins in the migrate entity or we can just do that we did here and have the process plugin manager keep them. Probably storing on the migrate entity by hash is better because then we can avoid re-normalizing as well.

chx’s picture

> This sould really break in case someone decided to write their own totally different password service. Additional I don't know why this is part of migrate not migrate_drupal.

Because it's not uncommon for legacy systems to have MD5 passwords (for eg WordPress uses phpass since March 2008 (version 2.5) but it still supports plain MD5 passwords). The service is not doing anything just proxies through unless enableMd5Prefixing is called before user_save so there's no harm in just sitting there. Finally, breaking in case of a different password service: if it was defined before us, we save the service definition and call it so that case is covered. If someone overrides the service after migrate then we can't migrate passwords so the user migration giving up is also correct.

dawehner’s picture

--- a/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/DestinationBase.php
+++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/DestinationBase.php
@@ -9,9 +9,42 @@
 namespace Drupal\migrate\Plugin\migrate\destination;
 
 use Drupal\Core\Plugin\PluginBase;
+use Drupal\migrate\Entity\MigrationInterface;
 use Drupal\migrate\Plugin\MigrateDestinationInterface;
+use Drupal\migrate\Plugin\RequirementsInterface;
 
-abstract class DestinationBase extends PluginBase implements MigrateDestinationInterface {
+abstract class DestinationBase extends PluginBase implements MigrateDestinationInterface, RequirementsInterface {

Any reason why many methods in this class aren't implemented?

--- a/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/Entity.php
+++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/Entity.php

+  protected static function getEntityType($plugin_id) {
+    // Remove entity:
+    return substr($plugin_id, 7);
   }

Can't we just use $this->getDerivativeId() instead? This should result in the same bits.

--- /dev/null
+++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/EntityConfigBase.php

+   * @param Row $row

Please ensure to have absolute namespace path.

index 0000000..8e4d14b
--- /dev/null
+++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/EntityContentBase.php

+  public function import(Row $row, array $old_destination_id_values = array()) {
+    if ($all_instances = $this->fieldInfo->getInstances($this->storageController->getEntityTypeId())) {
+      /** @var \Drupal\Field\Entity\FieldInstanceConfig [] $instances */
+      $instances = array();
+      if ($bundle_key = $this->getKey('bundle')) {
+        $bundle = $row->getDestinationProperty($bundle_key);
+        if (isset($all_instances[$bundle])) {
+          $instances = $all_instances[$bundle];
+        }
+      }
+      foreach ($instances as $field_name => $instance) {
+        $field_type = $instance->getType();
+        if ($this->migrateEntityFieldPluginManager->getDefinition($field_type)) {
+          $destination_value = $this->migrateEntityFieldPluginManager->createInstance($field_type)->import($instance, $row->getDestinationProperty($field_name));
+          // @TODO: check for NULL return? Add an unset to $row? Maybe needed in
+          // exception handling? Propagate exception?
+          $row->setDestinationProperty($field_name, $destination_value);
+        }
+      }
+    }
+    $entity = $this->getEntity($row, $old_destination_id_values);
+    return $this->save($entity, $old_destination_id_values);
+  }

Some one-liners in there would be cool to explain what all this code is doing.

--- /dev/null
+++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/EntityRevision.php


+  /**
+   * {@inheritdoc}
+   */
+  public function getIds() {
+    if ($key = $this->getKey('revision')) {
+      $ids[$key]['type'] = 'integer';
+      return $ids;
+    }
+    throw new MigrateException('This entity type does not support revisions.');
+  }
+

Lovely exception!

--- /dev/null
+++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/EntityUser.php

+   */
+  public function import(Row $row, array $old_destination_id_values = array()) {
+    if ($this->password) {
+      if ($this->password instanceof MigratePassword) {
+        $this->password->enableMd5Prefixing();
+      }
+      else {
+        throw new MigrateException('Password service has been altered by another module, aborting.');
+      }
+    }
+    $ids = parent::import($row, $old_destination_id_values);
+    if ($this->password) {
+      $this->password->disableMd5Prefixing();
+    }
+
+    return $ids;
+  }

As written above this is kinda problematic. Isn't there another way to do it, like some temporary override during the migration process.

index fa731ff..b9988e9 100644
--- a/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/id_map/Sql.php
+++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/id_map/Sql.php
@@ -8,11 +8,12 @@

 
   /**
+   * Initialize the plugin.
+   */
+  protected function init() {
+    if (!$this->initialized) {
+      $this->initialized = TRUE;
+      // Default generated table names, limited to 63 characters.
+      $machine_name = str_replace(':', '__', $this->migration->id());
+      $prefixLength = strlen($this->getDatabase()->tablePrefix()) ;
+      $this->mapTableName = 'migrate_map_' . Unicode::strtolower($machine_name);
+      $this->mapTableName = Unicode::substr($this->mapTableName, 0, 63 - $prefixLength);
+      $this->messageTableName = 'migrate_message_' . Unicode::strtolower($machine_name);
+      $this->messageTableName = Unicode::substr($this->messageTableName, 0, 63 - $prefixLength);
+      $this->ensureTables();
+    }
+  }

I really wonder whether we maybe should do something better than just truncating, the problem is that this might lead to non-unique table names. Do you have an opinion about that? How did you dealt with that
in Drupal6/7?

+++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/process/Migration.php
@@ -0,0 +1,137 @@

+class Migration extends ProcessPluginBase implements  ContainerFactoryPluginInterface {

Nitpick alarm: two spaces

   // If the lookup didn't succeed, figure out which migration will do the
+      // stubbing.
+      if ($self) {
+        $migration = $this->migration;
+      }
+      elseif (isset($this->configuration['stub_id'])) {
+        $migration = $migrations[$this->configuration['stub_id']];
+      }
+      else {
+        $migration = reset($migrations);
+      }
+      $destination_plugin = $migration->getDestinationPlugin();
+      // Only keep the process necessary to produce the destination ID.
+      $process = array_intersect_key($migration->get('process'), $destination_plugin->getIds());
+      // We already have the source id values but need to key them for the Row
+      // constructor.
+      $source_ids = $migration->getSourcePlugin()->getIds();
+      $values = array();
+      foreach (array_keys($source_ids) as $index => $source_id) {
+        $values[$source_id] = $source_id_values[$migration->id()][$index];
+      }
+      $stub_row = new Row($values, $source_ids);
+      $stub_row->stub(TRUE);
+      // Do a normal migration with the stub row.
+      $migrate_executable->processRow($stub_row, $process);
+      $destination_ids = $destination_plugin->import($stub_row);

WOW!

index 0efc013..7b1c92f 100644
--- a/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/source/SourcePluginBase.php
+++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/source/SourcePluginBase.php
@@ -12,6 +12,9 @@

+    $resultHook = $this->getModuleHandler()->invokeAll('migrate_prepare_row', array($row, $this, $this->migration));
+    $resultNamedHook = $this->getModuleHandler()->invokeAll('migrate_'. $this->migration->id() . '_prepare_row', array($row, $this, $this->migration));
+    // If any of the hooks returned false, we want to skip the row.

We usually don't use camelCase for local variables.

+    if (($resultHook && in_array(FALSE, $resultHook)) || ($resultNamedHook && in_array(FALSE, $resultNamedHook))) {

Should we maybe use strict mode here?

--- /dev/null
+++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/source/d7/Node.php
@@ -0,0 +1,66 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\migrate\Plugin\migrate\source\d7\Node.
+ */
+
+namespace Drupal\migrate\Plugin\migrate\source\d7;
+
+use Drupal\migrate\Plugin\migrate\source\SqlBase;
+
+/**
+ * Drupal 7 node source.
+ *
+ * @PluginID("drupal7_node")
+ */
+class Node extends SqlBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function query() {
+    return $this->database->select('node', 'n')
+     ->fields('n', array('nid', 'vid', 'language', 'title', 'uid',
+       'status', 'created', 'changed', 'comment', 'promote', 'sticky',
+       'tnid', 'translate'))
+     ->condition('n.type', $this->sourceType)
+     ->orderBy('n.changed');
+
+  }
+
+  public function getCurrentKey() {
+    // TODO: Implement getCurrentKey() method.
+  }
+
+  public function fields() {
+    // TODO: Implement fields() method.
+  }
+
+  public function getIgnored() {
+    // TODO: Implement getIgnored() method.
+  }
+
+  public function getProcessed() {
+    // TODO: Implement getProcessed() method.
+  }
+
+  public function resetStats() {
+    // TODO: Implement resetStats() method.
+  }
+
+  /**
+   * (PHP 5 &gt;= 5.1.0)<br/>
+   * Count elements of an object
+   * @link http://php.net/manual/en/countable.count.php
+   * @return int The custom count as an integer.
+   * </p>
+   * <p>
+   * The return value is cast to an integer.
+   */
+  public function count() {
+    // TODO: Implement count() method.
+  }
+
+
+}

There are so many TODOs in there, we should at least document in the issue summary why we do that.

--- a/core/modules/migrate/lib/Drupal/migrate/Row.php
+++ b/core/modules/migrate/lib/Drupal/migrate/Row.php

   /**
+   * Returns the raw destination. Rarely necessary.
+   *
+   * For example calling setDestination('foo:bar', 'baz') results in
+   * @code
+   * $this->destination['foo']['bar'] = 'baz';
+   * $this->rawDestination['foo.bar'] = 'baz';
+   *
+   * @return array
+   *   The raw destination values.
+   */
+  public function getRawDestination() {
+    return $this->rawDestination;
+  }

It would be great to @endcode as well. It is great to document this here!

index 02ba13a..59c7680 100644
--- a/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/Dump/Drupal6TextSettings.php
+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/Dump/Drupal6TextSettings.php
@@ -7,29 +7,20 @@

+      ->key(array('name' => 'teaser_length'))
+      ->fields(array('value' => 'i:456;'))
+      ->execute();

There are a couple of places where we use serialize and a couple of places where we don't. Is there a reason behind that?

--- a/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/d6/MigrateContactConfigsTest.php
+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/d6/MigrateContactConfigsTest.php

+  /**
+   * Tests migration of aggregator variables to aggregator.settings.yml.
+   */
+  public function testContactSettings() {

This seems to be a copy and paste error from some earlier patches. This just appeared in the diff due to the introduction of the setup method.

dawehner’s picture

The original intent was... to avoid re-instantiating the same class over and over again, plugins are usually stored in migrate entity properties. However, because of the iterator plugin and stubbing the getProcessPlugin takes a $process array argument. Either we could hash that array and store the resulting process plugins in the migrate entity or we can just do that we did here and have the process plugin manager keep them. Probably storing on the migrate entity by hash is better because then we can avoid re-normalizing as well.

I prefer the measurement-based performance approach but sure, you can just do whatever you like here.

benjy’s picture

FileSize
381.11 KB
11.71 KB

Edit: Thanks for the review dawehner.

#1

  1. @chx answered this in #2
  2. I'm open to suggestions for renaming but I think what we have works.
  3. done
  4. Yes this is intended so we can show a requirements error message rather than an exception.
  5. Highwater is the same as in Drupal 7. It's a timestamp that indicates what has already been imported. I added some more docs
  6. Docs removed for both the time and memory properties that are now in MigrateExecutable.
  7. $limit is an array containing a unit and value key.
  8. .
  9. @chx answered this in #3.
  10. done.
  11. .
  12. fixed.
  13. This is explained in #2. We're going to refactor and remove MigrateProcessPluginManager altogether.
  14. Added a comment to say that it was intentional.
  15. .

#4

  1. They come from the interface but most destinations don't need them.
  2. There is no $this, we could if getDerivativeId() was static.
  3. Fixed.
  4. Added a few comments.
  5. .
  6. I think this was addressed in #3.
  7. The Drupal 7 contrib module handled this the same way.
  8. Fixed.
  9. .
  10. Fixed camel case.
  11. Not sure what you mean? We check the array of results to see if it contains FALSE. Do you mean for the first check to be is_array($result_hook)...?
  12. That D7 source shouldn't even be there, deleted the whole file.
  13. Added @endcode.
  14. We used serialize when it is a big string/array and the serialized value when it was just one value.
  15. Fixed comment.
benjy’s picture

FileSize
381.61 KB
5.52 KB

This patch removes MigrateProcessPluginManager as mentioned in #6 to address the point raised in #1.13

benjy’s picture

FileSize
26.6 KB
393.96 KB

Fixed some comments and formatting errors picked up by PHPStorms "Inspect Code" feature.

benjy’s picture

FileSize
395.28 KB
29.61 KB

A few changes here, mainly:

  1. Variables now get their Drupal 8 defaults if they're set to NULL. This is particularly important for the URL protocol whitelist which was set to NULL before and made Drupal really unhappy. This highlights a huge hole in our testing which we have communicated previously: we can test what we intended to migrate actually migrates to where we think it should be but a) whether Drupal 8 uses it is anyone's guess (ie we can read the system.site CMI file and check the value of slogan but testing whether the slogan ends up in the theme where it should be would require essentially doubling the entire test suite which I believe noone wants. This is mitigated for entities by using the Entity API but raw CMI objects are not covered by any API and they are problematic. b) more, whether the source database structure and even more importantly our little dump contents match a "typical" site in the wild is anyone's guess. In this particular case, we ran afoul of b).
  2. Added a "store null" plugin configuration option for the Config destination when we want to store NULL instead of the default. Yes, it's in the class doxygen.
  3. Changed getDerivativeDefinition inline with core.
  4. Renamed getEntityType to getEntityTypeId to be inline with core
  5. Migration using missing (not installed) entity type destination do not fatal any more. This was intended before but the implementation was baroque and broken. The new implementation happens to be as simple as it gets and as a plus, it works.
  6. The block migration does not blow up if custom_block is disabled. This was also intended (as visible in the custom_block: false describing a soft dependency) but the d6_block_plugin_id process plugin still unconditionally required
    custom_block. Opsie.
  7. Removed some D7 files from migrate_drupal which are now in the IMP drupal7 branch.
chx’s picture

Issue summary: View changes
chx’s picture

Issue summary: View changes
andypost’s picture

Issue tags: +Needs change record

Suppose this needs change notice because
1) API changed: plugin-types, new interfaces, format of yml-files
2) handbook pages are outdated

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

The changes in the interdiffs seems pretty fine. Yeah we could iterate until the end here but this looks like a good step forward now.
I do agree with andypost but last time I was proven to be wrong on the change record side of things.

webchick’s picture

Priority: Normal » Major

Escalating to major, since it's a Migrate unblocker, but due to how many changes are crammed into this patch, it would take me at least 4 hours to do final review, which means it won't happen for at least a week. :\ Hoping one of the other maintainers can get to it before then, or else that the patch is split up functional lines to make each chunk more like a 15-30 min review process, which I could pick away at over the next few days when I'm not traveling.

chx’s picture

I need policy clarified. Are we writing 8-8 change notices or not? berdir told me we don't now andypost + dawehner says we do.

dawehner’s picture

My main goal is to be pragmatic. Noone will write migrations for contrib modules already, so why should we. On the other hand you could always argue that it is wrong to special case everything.

xjm’s picture

In general, as dawehner says, we're pragmatic.

  • We write brief 8-to-8 change records for major 8-to-8 changes that impact core and contrib code that's under development, and wherever possible link off to actual API and handbook documentation so we don't need to keep overhauling it.
  • With 7-to-8 BC breaks, we add a change record for every tiny thing. For 8-to-8, we don't sweat the small or non-impactful stuff except to ensure existing change records get updated where need be.
  • Part of the task of drafting change records is to find existing change records that should be checked for updates, and ensure they're added to the issue. So, https://drupal.org/node/2141805 should be updated to include a reference to this issue in its issues field, and any relevant changes can be incorporated there.
chx’s picture

I have updated https://drupal.org/node/2141805 . So we need another...??

xjm’s picture

Issue tags: -Needs change record

No, IMO that's plenty, so long as the handbook docs get updated. Thanks chx.

It's really unfortunate that this is such a huge patch. I can't imagine it was reviewed completely in 13 comments given its size and impact. @dawehner's review effort is awesome, but shouldn't something of this scale be reviewed by more than one developer?

chx’s picture

The patch is definitely teamwork and is already reviewed by either benjy or me (and each other). I seriously doubt there's a single line of code that was seen by only one developer. I definitely did re-review, for example, all the tests very recently when I reworked them to become MigrateDrupal6Test, penyaskito yesterday was working a lot with the entity destination -- you get the picture.

dawehner’s picture

One central problems of such mega-patches is that you never know whether changes are in scope or not, which makes it hard to figure out
the important parts of the patch. It would be just great if you could avoid such patches in the future ( I know you have something even bigger planned)

chx’s picture

I dearly badly hope we never need to do an API patch of this size again -- the API really should be done-ish as it migrates D6 just fine.

As for the next patch, we can split out the plugins first and then it's going to be YAML-Test-dump times 82 or so in one 600kb patch without any concepts or out-of-scope, just the boring parts. Plus MigrateDrupal6Test which is a real fun metatest. I am so proud of that one.

dawehner’s picture

I dearly badly hope we never need to do an API patch of this size again -- the API really should be done-ish as it migrates D6 just fine.

This is good to hear, thank you!

chx’s picture

Note that the commit message only includes that worked on these parts. The Drupal 6 migration was also worked on by axoplasm, kgoel, dclavain, jeckman, alvar0jurtad0, Drupali, fastangel gloob, jhedstrom, joshtaylor and mpgeek. People were already noticing the lack of attribution -- I would never ever do that.

xjm’s picture

Assigned: Unassigned » Dries

Alright, thanks! Assigning to Dries for review.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2213451_9.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
395.25 KB

Core removed the need for empty .module files and migrate_drupal.module used to be empty before this patch so the patch conflicted.

alexpott’s picture

Issue tags: +beta target

One of the aims of beta is to have the API as complete as possible - therefore it makes sense for this to be a beta target.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

To add to @alexpott's comments in #28, I feel that getting this in and then the D6 to D8 migrations before beta will give us invaluable feedback from developers who will be much more likely to start testing the migrate API.

Also, back to RTBC.

int’s picture

Why assigning to Dries for review? He is review at this moment? He asked to? Only he can do it?

This issue are RTBC for 6 days...

I do not want to be misunderstood, but I think assigning to dries, made everyone stop working/reviewing on this.

benjy’s picture

@int if you have a review or comments to add please put them in this issue.

The issue was set to RTBC and has been assigned to Dries for final review/committing but that doesn't mean you can't still chip in if you have something to add.

Dries’s picture

Assigned: Dries » Unassigned
Status: Reviewed & tested by the community » Fixed

Great work! Glad to see progress on the migrate path. I just committed it to 8.x.

xjm’s picture

@int, yes, the issue was assigned to Dries for review following discussion with him about the issue in a meeting. It was already RTBC, so there was no work to interrupt. :)

Status: Fixed » Closed (fixed)

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