Suggested commit message:

Issue #2121299 by andypost, bdone, benjy, penyaskito, chx, claudiu.cristea, damiankloip, gregboggs, InternetDevels, jessehs, jhedstrom, marvil07, mikeryan, pcambra, Xano, YesCT, axoplasm, kgoel, dclavain, jeckman, alvar0hurtad0, Drupali, fastangel, gloob, jhedstrom, joshtaylor, mpgeek, alexpott: Migrate Drupal 6 to Drupal 8

Problem/Motivation

Now the upgrade path has been replaced with the Migration API we need to provide migrations from Drupal 6 to Drupal 8.

Proposed resolution

Provide default migrations from Drupal 6 to Drupal 8 including CCK and a few additional CCK field modules such as date.

How To Review

This patch has 82 migrations that consist of:

Process Plugins

Some migrations have process plugins which apply processing to specific fields that are been imported. For example the FileUri process plugin generates the correct stream wrapper string. These and other files can be reviewed here.

MigrateDrupal6Test

This is one monster test that runs every single migration. We squashed a lot of bugs writing this test and it catches many issues which are sometimes disguised when an individual migration test sets something up "just right" in the setUp method. Individual tests are still useful in our opinion because debugging MigrateDrupal6Test is a special ... experience best described here.

Remaining tasks

Review the patch.

User interface changes

n/a

API changes

n/a

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

chx’s picture

We have split migrate into migrate and migrate_drupal modules so that after the migration from an older Drupal version is done all those migration can be removed from the system as they won't be needed any more.

chx’s picture

Issue summary: View changes

Filed the meta issue for D6->D8 migrations and began to file the children of that.

benjy’s picture

Title: The IMP initiative: Migrate in Core » Migrate in Core: Drupal 6 to Drupal 8
Issue summary: View changes
Status: Active » Needs review
FileSize
639.73 KB

09aadac

dawehner’s picture

I wonder whether you could break up the patch (not on an issue base but just in a way that you can actually use dreditor).
For example you could create one for the source, one for the tests and put the migration + dump bits into another one.

chx’s picture

So theoretically -- yes we perhaps could split -- we have unit tests for the sources so those can stand alone but the migrations-dumps-test belong together otherwise we are committing untested code and that's probably unwise. Especially MigrateDrupal6Test tends to break like every second day when this or that API changes a bit. It practically covers almost all CRUD in Drupal 8 right now.

chx’s picture

Ah, sorry didn't comprehend #5 , just separate files, I will try to do some git magic, yes.

chx’s picture

Patch split is rolled by:

git diff upstream/8.x `cat sources.txt|tr "\n" " "` > 2121299_8_sources.patch
git diff upstream/8.x `cat dumps.txt|tr "\n" " "` > 2121299_8_dumps.patch
git diff upstream/8.x `cat migrations.txt|tr "\n" " "` > 2121299_8_migrations.patch
git diff upstream/8.x `cat tests.txt|tr "\n" " "` > 2121299_8_tests.patch
git diff upstream/8.x `cat rest.txt|tr "\n" " "` > 2121299_8_rest.patch

the txts are attached in a zip; they don't need to be reviewed.

Status: Needs review » Needs work

The last submitted patch, 8: 2121299_8_tests.patch, failed testing.

dawehner’s picture

Thank you very much! I will find some time this night.

chx’s picture

Status: Needs work » Needs review

Obviously *Test.php won't run or pass; it misses the bases but if it wouldn't the dumps and the migrations aren't there.

The last submitted patch, 8: 2121299_8_rest.patch, failed testing.

chx’s picture

FileSize
5.8 KB
52.54 KB

Another note: the "rest" contained a few things that are not in the original. Fixed, hopefully.

chx’s picture

chx’s picture

Issue summary: View changes
chx’s picture

Issue summary: View changes
dawehner’s picture

First small bit before the browser crashed (100KB is kind of a limit)

  1. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Form/MigrateDrupalRunForm.php
    @@ -0,0 +1,142 @@
    +        'title' => t('Running migrations'),
    ...
    +        'init_message' => t('Processing migration @num of @max.', array('@num' => '1', '@max' => count($migration_ids))),
    

    FormBase provides $this->t()

  2. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Form/MigrateDrupalRunForm.php
    @@ -0,0 +1,142 @@
    +        'title' => t('Running migrations'),
    

    We can use $this->t() here.

  3. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Form/MigrateDrupalRunForm.php
    @@ -0,0 +1,142 @@
    +        'init_message' => t('Processing migration @num of @max.', array('@num' => '1', '@max' => count($migration_ids))),
    

    This feels like a usecase for form plural, isn't it?

  4. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/process/d6/FieldSettings.php
    @@ -0,0 +1,68 @@
    \ No newline at end of file
    

    newline

  5. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/process/d6/FieldTypeDefaults.php
    @@ -0,0 +1,39 @@
    +        throw new MigrateException(sprintf('Lookup failed for %s', var_export($value, TRUE)));
    

    A little bit more context in the exception message would be nice.

  6. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/process/d6/FileUri.php
    @@ -0,0 +1,38 @@
    +    return $is_public ? "public://$uri" : "private://$uri";
    

    Just curious: Aren't there other schemas?

  7. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/process/d6/UserPicture.php
    @@ -0,0 +1,65 @@
    + * Split the 'administer nodes' permission from 'access content overview'.
    ...
    +class UserPicture extends ProcessPluginBase implements ContainerFactoryPluginInterface {
    

    This seems to be a leftover from c&p code.

+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/d6/MigrateCommentVariableDisplayBase.php
@@ -0,0 +1,79 @@
+/**
+ * Base class for Drupal 6 comment variables to Drupal 8 entity display tests.
+ */
+class MigrateCommentVariableDisplayBase extends MigrateDrupalTestBase {

I guess we can make this testclass abstract?

dawehner’s picture

+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/source/VariableMultiRow.php
+ * Drupal 6 multiple variables source from database.

+ *   id = "variable_multirow"

Is there a rule when we use a d6 prefix and when not?

b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/source/d6/AggregatorItem.php

+  }
+}

Just in case you want to clean up some of the classes ... the coding standard suggests to add a newline between the new curly braces at the end.

+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/source/d6/Book.php
+
+namespace Drupal\migrate_drupal\Plugin\migrate\source\d6;
+use Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase;
+

... and a new line between the namespace and the use. This is absolute no requirement to change here, just a suggestion.

+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/source/d6/FieldInstancePerViewMode.php

+ * A base class for field instances which all require the same data and fields.

+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/source/d6/FieldInstancePerFormDisplay.php

+/**
+ * A base class for field instances which all require the same data and fields.

There seems to be a little bit c&p going on.

+ * Drupal 6 profile fields source from database.

This is damn cool that we support that migration.

+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/source/d6/Term.php

+      // @todo: working, but not is there support for distinct() in FakeSelect?
+      ->distinct();
+    if (isset($this->configuration['vocabulary'])) {
+      $query->condition('vid', $this->configuration['vocabulary'], 'IN');

Tried to understand that comment. Maybe something like: This works, but we cannot test that, because there is no support for distinct() in FakeSelect, yet.

+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/source/d6/ViewMode.php

+ * A base class for field instances which all require the same data and fields.

Another c&p of existing documentation.

+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/source/d6/VocabularyPerType.php

+ * Drupal 6 vocabularies source from database.

it is not really obvious for me, what this plugin is supposed to do :(

+++ b/core/modules/migrate_drupal/tests/Drupal/migrate_drupal/Tests/source/d6/MenuTest.php

+namespace Drupal\migrate_drupal\Tests\source\d6;


class TestMenu extends Menu {
+  public function setDatabase(Connection $database) {
+    $this->database = $database;
+  }
+  public function setModuleHandler(ModuleHandlerInterface $module_handler) {
+    $this->moduleHandler = $module_handler;
+  }
+}

I guess we could reduce the amount of code here by using a trait for these two methods but just for the test classes. This would also allow us to kill the use statements.
On top of that there is also no real need to re-specify the namespace.

benjy’s picture

FileSize
25.13 KB
675.26 KB

#17

  1. MigrateDrupalRunForm - This is not in my patch, must have been in the smaller chunks by accident.
  2. as above
  3. as above
  4. Done.
  5. Done.
  6. I'm not aware of any other schemas in D6 but we should ask someone who can give us a definite answer.
  7. Removed.
  8. Made MigrateCommentVariableDisplayBase abstract.

#18

  1. VariableMultiRow and Variable are special cases right now as they're re-used by both the D7 and D6 migrations. The comment was wrong so I've fixed that.
  2. I went through all the classes apart from the tests and ensured the empty line was there.
  3. Added space between use and namespace.
  4. Fixed comments.
  5. Fixed comment.
  6. Fixed comment.
  7. I tried to make the VocabularyPerType comment clearer.
  8. Do we really need to use a trait to reduce a few lines of code in a test? If so, an example of what your thinking would be good.

ca1fcce

Status: Needs review » Needs work

The last submitted patch, 19: 2121299_19.patch, failed testing.

chx’s picture

> I'm not aware of any other schemas in D6 but we should ask someone who can give us a definite answer.

Am I allowed to be the "someone"? Drupal 6 didn't have wrappers; it only had public and private. Consult https://api.drupal.org/api/drupal/includes%21file.inc/function/file_crea... for example.

benjy’s picture

Status: Needs work » Needs review
FileSize
645.04 KB

Migrations needed moving into install as per: https://drupal.org/node/2234799

Status: Needs review » Needs work

The last submitted patch, 22: 2121299_22.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
2.48 KB
645.02 KB

A migration had been changed in the sandbox and it broke the test. interdiff attach shows them changes.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Noone is able to review that properly anyway :p

penyaskito’s picture

+1 to RTBC.

chx’s picture

Issue summary: View changes
penyaskito’s picture

Issue summary: View changes
penyaskito’s picture

Issue summary: View changes
effulgentsia’s picture

Priority: Normal » Major
Issue tags: +beta target

Site owners with D6 sites being able to test the D8 beta with their real content would be pretty awesome.

penyaskito’s picture

Issue summary: View changes
jibran’s picture

There are a lot of doc issue and @todos missing links every time I tried to review it but I am unable to complete the review. So can we please create a followup doc fix novice issue for this.

webchick’s picture

Since this patch is a Really Big Deal™, we pinged Dries to see if he wanted to take a crack at reviewing/committing it. If he doesn't get to it by Friday, I can take a look. Not explicitly assigning to him though since it's not like he has a monopoly on reviewing/committing, I just personally won't have time before Friday myself.

alexpott’s picture

Assigned: Unassigned » alexpott

Reviewing...

alexpott’s picture

FileSize
61.62 KB
650.34 KB

Ok I've looked at every line of this patch - posting some minor tidy ups - yes if we picked every nit this patch would take ages. I've just gone for what glared at me whilst looking at all 645k. I'm happy to commit this is this is green as my amendments are totally minor - especially if one of the migration team does a quick review of the interdiff and +1's.

chx’s picture

I read these changes; they are OK (if the bot agrees).

alexpott’s picture

Issue summary: View changes

Adding myself to the suggested commit message due to the patch in #35

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Let's get people testing this!

Committed 66800ac and pushed to 8.x. Thanks!

  • Commit 66800ac on 8.x by alexpott:
    Issue #2121299 by andypost, bdone, benjy, penyaskito, chx, claudiu....
donquixote’s picture

We notice this did not conflict with #2083547: PSR-4: Putting it all together. Great!

However, all the drupal_get_path() will cause trouble when these newly introduced files move to PSR-4.
The solution is to use dirname(__DIR__) . '/Dumps/XYZ.php' instead of drupal_get_path('module', 'migrate_drupal') . '...'.

This is done here: #2248149: Use __DIR__ instead of drupal_get_path() for local PSR-0/PSR-4 directories.

donquixote’s picture

Status: Fixed » Closed (fixed)

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

Status: Closed (fixed) » Needs review

Hajdzik queued 35: 2121299.35.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 35: 2121299.35.patch, failed testing.

chx’s picture

Status: Needs work » Closed (fixed)