Migrate in core: patch #1

+++ b/core/modules/migrate/lib/Drupal/migrate/Entity/MigrationInterface.php
@@ -0,0 +1,77 @@
+/**

+++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/MigratePluginManager.php
@@ -0,0 +1,60 @@
+ * Contains \Drupal\migrate\MigraterPluginManager.
...
+   * Constructs a MigraterPluginManager object.

The class for this file is MigratePluginManager, not MigraterPluginManager. Typo.

I understand your feeling about doxygens. But if you're not willing to tackle it yourself (understandable) than you should try to gather some people to fix it. The current state is not so good. Lot's of functions don't have any documentation on it. I don't say everything should be *perfect* but we shouldn't commit this as we demanded better docs on patches way bigger than this one...

I have the feeling there are a bunch of people willing to help on this so it shouldn't be hard to find the right person.

I'll take a first stab at docs stuff.

At the same time, others can do reviews besides the docs.

+++ b/core/modules/migrate/tests/Drupal/migrate/Tests/MigrateTestCase.php
@@ -0,0 +1,65 @@
+abstract class MigrateTestCase extends UnitTestCase {
...
+  /**
+   * Provide meta information about this battery of tests.
+   */
+  public static function getInfo() {
+    return array(
+      'name' => 'Migrate test',
+      'description' => 'Tests for migrate plugin.',
+      'group' => 'Migrate',
+    );

Not really needed

@YesCT, I started working on doxygen here: #2125083: Remove unused $keep variable from migrate sources

I wanted to attach files, but the page wont load. hope this saves. :)
it is just a diff on my changes to the DrupalMessage class.
seemed like drupalmessage would be general for anyone, but extended the migrate message interface. guessing it was named drupal message since it used drupal set message, not cause it was meant to be generic. anyway, posting this for preliminary feedback before I get too far into it.

http://privatepaste.com/af5ee6f77e

I would prefer the two be committed together and I'm not sure working on docs in another issue is a good idea.

(for example, when I went to do the docs, I changed more than just docs. I renamed a method.)

I dont really know what the best workflow is here, but I lean toward just keeping them in the same issue.

migrate_instrument_start() and migrate_instrument_stop() both appear to be undefined.

8: 2125717_5.patch queued for re-testing.

OK. I'm willing to try something new. using the sandbox, pushed the changes from #24, which can be seen http://drupalcode.org/sandbox/chx/2105305.git/commit/13f7703 . And, I guess I'll comment on #2125083: Remove unused $keep variable from migrate sources as I proceed.

I'm not sure about having multiple issues regarding the same sandbox, but I guess it can serve to try and focus the commenting on particular subjects. [edit: OH! duh, I see the other issue is in the imp project/sandbox issue queue, and this is the core issue. Sorry.]

For reviewers, this means you should clone from the sandbox, not look at the patch. Something that has worked pretty well for config_translation is to post a new patch every few days to make it easier to get outside eyes on.

Yes, the patch has a lot of TODOs but this patch is so important to get a proper upgrade path, which is critical
both for the success of Drupal 8 and Drupal in general. By letting this in the current state we will have a lot of more potential contributors
to all the upgrade migrations we need.

1. +++ b/core/modules/migrate/lib/Drupal/migrate/MigrateExecutable.php
   @@ -0,0 +1,526 @@
   +  public function getSource() {
   ...
   +      $this->source = new Source($this->migration, $this);
   +    }
   +    return $this->source;
   +  }
   +
   ...
   +   * The rollback action to be saved for the current row.
   +   *
   +   * @var int
   +   */
   +  public $rollbackAction;
   

   Note: we are mixing up the order of properties and methods here.

2. +++ b/core/modules/migrate/lib/Drupal/migrate/MigrateExecutable.php
   @@ -0,0 +1,526 @@
   +  public function import() {
   

   Given that this is probably the enter of all debugging for people, we should concentrate to document that properly in one of the next core issues.

3. +++ b/core/modules/migrate/lib/Drupal/migrate/MigrateMessageInterface.php
   @@ -0,0 +1,19 @@
   +interface MigrateMessageInterface {
   

   At least a little bit of documentation in this file would be cool

4. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/MigrateIdMapInterface.php
   @@ -0,0 +1,169 @@
   +   * @TODO: YUCK THIS IS SQL BOUND!
   ...
   +  public function getQualifiedMapTable();
   

   I guess this is just called by some sql specific code somewhere? In a future patch you could try to move that to a sql specific interface.

5. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/MigrateProcessInterface.php
   @@ -0,0 +1,36 @@
   +   * @param $value
   ...
   +   * @param MigrateExecutable
   ...
   +   * @param Row $row
   

   Longterm nitpick: use absolute class references.

6. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/MigrateSourceInterface.php
   @@ -0,0 +1,45 @@
   +
   +  public function __toString();
   

   Future improvements: We could think about adding some kind of arbitary method for it. There was a lot of interesting dicussion about this on http://pooteeweet.org/blog/2231

7. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/Config.php
   @@ -0,0 +1,87 @@
   + * Persist data to the config system.
   

   It seems to be that someone copy and pasted some documentation

8. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/DestinationBase.php
   @@ -0,0 +1,53 @@
   +
   

   Future improvements: We should document why all this methods is implemented here with an empty implementation

9. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/process/Get.php
   @@ -0,0 +1,55 @@
   + * Contains \Drupal\migrate\Plugin\migrate\process\CopyFromSource.
   ...
   + * This plugin copies from the source to the destination.
   ...
   + * @PluginId("get")
   ...
   +class Get extends PluginBase implements MigrateProcessInterface {
   

   Future improvements: Documentat that one better ... i guess the previous comment was leftover from CopyFromSource.

10. +++ b/core/modules/migrate/lib/Drupal/migrate/Tests/MigrateTestBase.php
    @@ -0,0 +1,55 @@
    +class MigrateTestBase extends WebTestBase {
    

    This is really awesome, as it will allow a lot of additional future tests.

+++ b/core/modules/migrate/lib/Drupal/migrate/Tests/MigrateTestBase.php
@@ -0,0 +1,59 @@
+    foreach ($files as $file) {
+      if (substr($file, -3) == '.gz') {
+        $file = "compress.zlib://$file";
+        require $file;
+      }
+      $class = 'Drupal\migrate\Tests\Dump\\' . basename($file, '.php');
+      $class::load($database);
+    }

I'm concerned that hardcoding the namespace means that only the migrate module with be able to provide tests.

If you want to complain about unfairness than you should complain about VDC.

I really think that we should rather improve the nitpicks you could apply everywhere over time. We should care more about the features
rather than the potential dept opened by that code. We will always improve things.

Here are just some nitpicks, which I would have not posted when the patch would have been green.

1. +++ b/core/modules/migrate/migrate.api.php
   @@ -0,0 +1,37 @@
   +use Drupal\migrate\Entity\MigrationInterface;
   +use Drupal\migrate\Plugin\MigrateSourceInterface;
   +use Drupal\migrate\Row;
   

   As all the other ones, not important, but usually we use absolute classes in .api.php files.

2. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/D6VariableTest.php
   @@ -0,0 +1,74 @@
   +class D6VariableTest extends MigrateSqlSourceTestCase {
   

   Missing @group Drupal

3. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/GetTest.php
   @@ -0,0 +1,89 @@
   +/**
   + * @group migrate
   + */
   +class GetTest extends MigrateProcessTestCase {
   
   +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/MigrateProcessTestCase.php
   @@ -0,0 +1,38 @@
   +
   +class MigrateProcessTestCase extends MigrateTestCase {
   

   Missing @group Drupal, @group Migrate

Yeah, it is green again!

dreditor is playing up for me, fortunately I didn't get too far with the patch before trying to paste.

I really like the YAML format for migrations, it's very self-documenting looking at the variables one and having the process stuff in there is fantastic.

Lots of nitpicks which got lost by dreditor, I haven't done an in-depth review.

With the caveat that I've not reviewed properly, I do agree this should go in ASAP. By definition this doesn't block the release, and it's going to have to be exempted from API changes at least right up until release candidate most likely. So we only gain visibility and the ability to split up work by having it in core.

Explicitly moving over to Dries since this is brand new stuff. If dreditor doesn't crash on me I'll try to review later, but don't hold it up on me doing so.

I spent about 90 minutes on this patch. It's not nearly enough time to understand everything that is going on in the patch. Like catch, I saw a lot of small problems in the patch (e.g. documentation mistakes). I was just focused on trying to understand the basic architecture and approach so I didn't document these. Even with the limited time, I feel like I understand the overall approach taken and feel comfortable with going in this direction.

My initial reaction is that the YML files are an interesting and clever way of defining migration steps. It looks very verbose though; a lot of information seems to be repeated in the YML files (for example, in the source portion and again in the process portion). It is a DX concern I have, but doesn't have to hold up this patch. I feel it would be easier to get this patch committed, and iterate on this later.

Committed and pushed to 8.x. Great work!

PS: Absolutely love the Migration API documentation at https://drupal.org/node/2127611. It would be great to document the "map" process plugin, since it is used in the iterator plugin example, but without its own docs, made it hard to understand the example.

woot!

Yay!! Great work everybody. So happy to see it in the core.

Here is a latest example of change notice New configuration translation user interface module added.

As a contrib module developer, here is what I'd want in a change notice: "In the past we wrote hook_update_7000() functions to facilitate the upgrade from Drupal 6 to Drupal 7, but now for Drupal 8, you have to do XXX." (Where XXX is link to the Migrate API documentation).

@dsnopek - Aren't we still doing module-specific updates inside hook_update_N() functions, though? My understanding was that migrations would be more for entities, blocks, types, etc...

[Edit: Hmm, but if the database is fresh, we couldn't do that anyways, so I guess all my existing hook_update_N() implementations for D8 will need to be converted.]

@dsnopek, @geerlinguy:

For major upgrades, everything will need a migration handler, no hook_update_N(). This does mean that any existing hook_update_N() in modules will need to be re-written.

For 8.x-8.x updates, hook_update_N() will still be used (at least for the time being). However if you make a schema change, you'd be expected to write a source and destination for that schema change ideally as well - so that it's possible to migrate from/to that version of the schema (i.e. 6.x to the new schema).

A change notice along the lines of what @dsnopek says seems OK, however I'm not sure things are ready at all yet for contrib module authors to start writing migrations for their modules - chx is that even worth trying or should we tell people to hold off for now?

So basically, every contrib module needs to write a migrate piece. What happens when module x is replaced by moduley or modulez, both of which do what x did but each also has their own unique other features?

I have a particular nasty case in mind, namely Ubercart on D6 to Commerce on D8 (or maybe Ubercart will also produce a D8 version as they did for D7. Yes there is a migrate from Ubercart D6 or D7 to Commerce D7 as a starting point for the migration, but both Ubercart and Commerce are sort of like Drupal in having a core and their own contrib features which are not handled by the migrate code.

Hi chx,

I have added the Change notice:
https://drupal.org/node/2141805

What happens when module x is replaced by moduley or modulez, both of which do what x did but each also has their own unique other features?

Module y and module z would both need to write destination classes for data from module x.

The sources for module x would be the same, so could be shared between y and z.

@nikhilsukul: Thanks! I made some minor changes too. :-)

Thanks for starting the change notice!

It's missing an important component though, which is the "this is what you used to do in D7, and here's what you now do in Drupal 8." (see https://drupal.org/node/1935708 for an example.) It's fine if we need to further refine the examples as more of Migrate gets in, and it's fine if it's only one example and not a comprehensive guide (that should go under the Migration API handbook), but we definitely need something to throw contrib authors a bone here.

It is a world of a difference.

That's exactly why we (contrib devs) need something :)

I know how upgrades went pre-migrate-in-core, and I know how migration classes worked in Drupal 6 and 7, but I don't know how to migrate things like variables into configuration items, etc. I think, at this point, even the most basic example (like how to upgrade one variable using the old way using hook_update_7000() in the .install file, and how to migrate one variable into config in the new way) would be a great starting point.

Plus, it would make sure we can iron out any possible major DX WTFs before migrate starts getting layers of polish.

Thanks for all the hard work on this, and know that it's very much appreciated!

I think a variable conversion would be a great example - the hook_update_N() with update_variables_to_config(), then the YAML file for comparison. This is also something a lot of modules are going to need to do.

Modules that have custom storage that's neither entities nor variables in Drupal 7 are going to need to read up on writing source/destination classes, but that's a more limited set of developers and a link to the general documentation (and the core migrations once we have them) should be plenty.

We're also going to have to document the workflow for overall migrations at some point (i.e. making it clear that when a module schema changes during a minor release, new source/destination classes need to be written to match that schema if it's incompatible with the previous one, as well as hook_update_N() for minor updates), but that probably needs its own issue to thrash out a bit.

Why in core?

I don't know how many installs are upgrades, and how many are fresh installs, but people who are smart enough to do an upgrade/migration, are smart enough to download and enable a module to do this, and then disable/remove it, when it is done.

@iantresman, Migrate in Core is the newest Core Initiative. The time for debate is considerably past.

That said, the basic notion is that traditional use of update hooks is bad, that Migrate will be cleaner, and it is in core because it is going to take the place of the core upgrade path. Not everything in the Migrate contrib module will be going into Core, just the essential APIs and mechanisms to upgrade Drupal 6 and Drupal 7 sites to Drupal 8.

+++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/source/SqlBase.php
@@ -0,0 +1,192 @@
+        foreach ($this->migration->get('sourceIds') as $field_name => $field_schema) {
+          if (isset($field_schema['alias'])) {
+            $field_name = $field_schema['alias'] . '.' . $field_name;
+          }
+          $map_join .= "$delimiter$field_name = map.sourceid" . $count++;
+          $delimiter = ' AND ';
+        }
+
+        $alias = $this->query->leftJoin($this->migration->getIdMap()->getQualifiedMapTable(), 'map', $map_join);

This code fails to do the necessary escaping, in particular that last line.

Which causes #3227361: Fix SqlBase::initializeIterator()'s cross-database joining when using particular DB/table names.