Create a migration config and class for enable end user to migrate Taxonomy Vocabulary from Drupal 7 to Drupal 8

CommentFileSizeAuthor
#69 interdiff-2410875-65-69.txt657 bytesphenaproxima
#69 2410875-69.patch59.25 KBphenaproxima
#65 interdiff-2410875-61-65.txt1 KBphenaproxima
#65 2410875-65.patch58.48 KBphenaproxima
#61 interdiff-2410875-59-61.txt1.27 KBphenaproxima
#61 2410875-61.patch57.3 KBphenaproxima
#59 interdiff-2410875-59.txt955 bytesphenaproxima
#59 2410875-59.patch57.33 KBphenaproxima
#59 2410875-59-FAIL.patch56.25 KBphenaproxima
#57 Screen Shot 2015-09-09 at 1.28.14 PM.png44.52 KBwebchick
#53 interdiff-2410875-49-53.txt1.31 KBphenaproxima
#53 2410875-53.patch55.29 KBphenaproxima
#49 2410875-49.patch55.3 KBphenaproxima
#46 interdiff-2410875-43-46.txt3.3 KBphenaproxima
#46 2410875-46.patch54.66 KBphenaproxima
#43 2410875-43.patch54.26 KBphenaproxima
#41 d6_d7_tax_diffs.txt14.94 KBmikeryan
#38 interdiff-2410875-36-38.txt5.01 KBphenaproxima
#38 2410875-38.patch55.27 KBphenaproxima
#36 2410875-36.patch50.59 KBphenaproxima
#27 taxonomy_diffs.txt23.42 KBmikeryan
#26 interdiff-2410875-24-26.txt508 bytesphenaproxima
#26 2410875-26.patch17.73 KBphenaproxima
#24 interdiff-2410875-22-24.txt3.13 KBphenaproxima
#24 2410875-24.patch17.72 KBphenaproxima
#22 2410875-22.patch18.36 KBphenaproxima
#19 2410875-19.patch21.32 KBphenaproxima
#18 interdiff-2410875-16-18.txt1.1 KBphenaproxima
#18 2410875-18.patch17.85 KBphenaproxima
#16 2410875-16.patch16.98 KBphenaproxima
#16 interdiff-2410875-14-16.txt4.8 KBphenaproxima
#14 2410875-14.patch11.71 KBphenaproxima
#13 2410875-13.patch5.32 KBphenaproxima
#10 2410875-10.patch5.96 KBphenaproxima
#9 2410875-9.patch6.05 KBphenaproxima
#7 2410875-7.patch5.9 KBphenaproxima
#2 migration_files_for-2410875-1.patch2.4 KBmiguelc303
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

miguelc303’s picture

Issue summary: View changes
miguelc303’s picture

Create a migration config and class for enable end user to migrate Taxonomy Vocabulary from Drupal 7 to Drupal 8

-enzo-’s picture

Patch tested against Drupal 8.0.x because the IMP is not totally sync

-enzo-’s picture

Issue summary: View changes
benjy’s picture

miguelc303’s picture

Added organization support to Anexus IT

phenaproxima’s picture

Project: IMP » Drupal core
Version: » 8.0.x-dev
Component: Code » migration system
Status: Active » Needs review
FileSize
5.9 KB

Updated the patch and added a test.

Status: Needs review » Needs work

The last submitted patch, 7: 2410875-7.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
6.05 KB

Round and round we go!

phenaproxima’s picture

FileSize
5.96 KB

Minor test cleanup.

Status: Needs review » Needs work

The last submitted patch, 10: 2410875-10.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Postponed
phenaproxima’s picture

Status: Postponed » Needs review
FileSize
5.32 KB
phenaproxima’s picture

phenaproxima’s picture

Title: Migration Files for Drupal 7 Taxonomy Vocabulary » Migration for Drupal 7 Taxonomy vocabularies and terms

Changing title.

phenaproxima’s picture

Added unit tests of the d7_taxonomy_term and d7_taxonomy_vocabulary source plugins.

Status: Needs review » Needs work

The last submitted patch, 16: 2410875-16.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
17.85 KB
1.1 KB

Whoops...let's try that again.

phenaproxima’s picture

FileSize
21.32 KB
Anonymous’s picture

If you worked on this issue you may be able to quickly help fix #2506001: Taxonomy term values referenced on nodes do not migrate (D6).

phenaproxima’s picture

phenaproxima’s picture

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 22: 2410875-22.patch, failed testing.

phenaproxima’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
17.72 KB
3.13 KB

Fixed a few bugs revealed by the tests.

Status: Needs review » Needs work

The last submitted patch, 24: 2410875-24.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
17.73 KB
508 bytes

OK, this oughta work.

mikeryan’s picture

FileSize
23.42 KB

Here are the diffs between the files in this patch and their D6 equivalents, it's really helpful to identify what changes are due to actual differences between D6 and D7.

mikeryan’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/taxonomy/migration_templates/d7_taxonomy_vocabulary.yml
    @@ -1,14 +1,14 @@
    -      source: name
    +      source: machine_name
    

    Example of a necessary difference, D7 had machine names for vocabularies but D6 did not.

  2. +++ b/core/modules/taxonomy/src/Plugin/migrate/source/d7/Term.php
    @@ -2,21 +2,21 @@
    + * Contains \Drupal\taxonomy\Plugin\migrate\source\Term.
    

    Missing d7 directory.

  3. +++ b/core/modules/taxonomy/src/Plugin/migrate/source/d7/Term.php
    @@ -28,12 +28,9 @@ class Term extends DrupalSqlBase {
    @@ -59,7 +56,7 @@ public function fields() {
    

    D7 adds format to the term data table, should be added to fields().

  4. +++ b/core/modules/taxonomy/src/Plugin/migrate/source/d7/Vocabulary.php
    @@ -2,35 +2,56 @@
    + * Contains \Drupal\taxonomy\Plugin\migrate\source\Vocabulary.
    

    Missing d7.

  5. +++ b/core/modules/taxonomy/src/Plugin/migrate/source/d7/Vocabulary.php
    @@ -2,35 +2,56 @@
    -class Vocabulary extends VocabularyBase {
    +class Vocabulary extends DrupalSqlBase {
    

    Just to be sure I understand this difference - D6 had the VocabularyBase class to deal with the term_node business, but D7 just needs a regular source class derived from DrupalSqlBase because term references are normal fields in D7?

  6. +++ b/core/modules/taxonomy/src/Plugin/migrate/source/d7/Vocabulary.php
    @@ -2,35 +2,56 @@
    -  public function prepareRow(Row $row) {
    

    Ditto - D6 needed prepareRow() but D7 doesn't because of the term_node business, right?

  7. +++ b/core/modules/taxonomy/src/Tests/Migrate/d7/MigrateTaxonomySettingsTest.php
    @@ -2,20 +2,20 @@
    - * Contains \Drupal\taxonomy\Tests\Migrate\d6\MigrateTaxonomyConfigsTest.
    + * Contains \Drupal\taxonomy\Tests\Migrate\d7\MigrateTaxonomySettingsTest.
    

    Why the different file names?

  8. +++ b/core/modules/taxonomy/src/Tests/Migrate/d7/MigrateTaxonomySettingsTest.php
    @@ -32,17 +32,16 @@ class MigrateTaxonomyConfigsTest extends MigrateDrupal6TestBase {
    +  public function testAggregatorSettings() {
    

    Touch of the copypasta here..

  9. +++ b/core/modules/taxonomy/src/Tests/Migrate/d7/MigrateTaxonomyTermTest.php
    @@ -28,81 +29,46 @@ protected function setUp() {
    +  protected function assertEntity($id, $expected_label, $expected_vid, $expected_description = '', $expected_weight = 0, $expected_parents = []) {
    

    Missing docs.

    Just to be clear, the assertEntity()/testTaxonomyTerms() combo here is doing the same thing as testTaxonomyTerms() in D6, this is a refactoring?

  10. +++ b/core/modules/taxonomy/src/Tests/Migrate/d7/MigrateTaxonomyVocabularyTest.php
    @@ -29,28 +30,27 @@ class MigrateTaxonomyVocabularyTest extends MigrateDrupal6TestBase {
    +  protected function assertEntity($id, $expected_label, $expected_description, $expected_hierarchy, $expected_weight) {
    

    Ditto with above term test.

  11. +++ b/core/modules/taxonomy/tests/src/Unit/Migrate/d7/TermTest.php
    @@ -2,16 +2,97 @@
    -class TermTest extends TermTestBase {
    +class TermTest extends MigrateSqlSourceTestCase {
    

    As above, I assume we no longer need the TermTestBase because no more term_node business to deal with?

phenaproxima’s picture

Status: Needs work » Postponed

Because D7 adds the format column to {taxonomy_term_data}, this is blocked by #2384567: Migration Files for Drupal 7 Text Formats & Filters.

webchick’s picture

Issue tags: +Migrate critical

Since this is one of the "big 4" content migrations, escalating to a Migrate critical.

phenaproxima’s picture

Status: Postponed » Needs review

This is now unblocked.

The last submitted patch, 2: migration_files_for-2410875-1.patch, failed testing.

mikeryan queued 26: 2410875-26.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 26: 2410875-26.patch, failed testing.

webchick’s picture

Just some house-keeping, ignore me.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
50.59 KB

We can give this a try...

Status: Needs review » Needs work

The last submitted patch, 36: 2410875-36.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
55.27 KB
5.01 KB

D'oh.

mikeryan’s picture

Status: Needs review » Needs work

Running a test D7 migration:

Migration d7_vocabulary_entity_display did not meet the requirements. Missing migrations d6_vocabulary_field_instance. requirements: d6_vocabulary_field_instance.

Oops!

On the positive side, the vocabularies and themselves appear to be successfully migrated.

Will follow up with a code review shortly...

mikeryan’s picture

Oh, actually d7_vocabulary_entity_display should just go away, right? The D6 version is only needed because of the term_node stuff, in D7 we had proper fields...

mikeryan’s picture

FileSize
14.94 KB

Diffs between D6 and D7 taxonomy handling, at least for the basics of terms and vocabularies themselves (since the term field handling is totally different).

mikeryan’s picture

Apart from d7_vocabulary_entity_display, looks good to me. Since I contributed to the taxonomy migration in the sprint sandbox, though, I'm not the one to RTBC this...

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
54.26 KB

Got rid of the d7_vocabulary_entity_display thing. @mikeryan is correct -- because these are fields, we shouldn't need to do anything special here.

ultimike’s picture

Status: Needs review » Needs work

I reviewed this while chatting with phenaproxima on IRC - I found a couple of things...

  1. +++ b/core/modules/node/src/Plugin/migrate/builder/d6/Node.php
    --- a/core/modules/node/src/Plugin/migrate/builder/d7/Node.php
    +++ b/core/modules/node/src/Plugin/migrate/builder/d7/Node.php
    
    +++ b/core/modules/node/src/Plugin/migrate/builder/d7/Node.php
    @@ -8,12 +8,12 @@
    +use Drupal\migrate_drupal\Plugin\migrate\builder\d6\CckBuilder;
    

    This looks wrong to me...

  2. +++ b/core/modules/node/src/Plugin/migrate/builder/d7/Node.php
    @@ -21,12 +21,11 @@ class Node extends BuilderBase {
       public function buildMigrations(array $template) {
    
    +++ b/core/modules/taxonomy/src/Plugin/migrate/source/Term.php
    @@ -2,41 +2,62 @@
           ->orderBy('tid');
    

    Will this work for hierarchical taxonomies where a parent term has a higher term id than one of its children?

-mike

ultimike’s picture

I just chatted with mikeryan on IRC regarding the hierarchical taxonomy issue:

Stubbing should take care of it - ideally, the less stubbing the better so ordering by pid may help but shouldn't be necessary

-mike

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
54.66 KB
3.3 KB

Moved CckBuilder one level up.

Status: Needs review » Needs work

The last submitted patch, 46: 2410875-46.patch, failed testing.

The last submitted patch, 46: 2410875-46.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
55.3 KB

Let's try that again.

benjy’s picture

This looks good to me, few super small code standards things, that's about it.

  1. +++ b/core/modules/node/src/Plugin/migrate/builder/d6/Node.php
    @@ -17,36 +16,18 @@
    +      $fields[ $info['type_name'] ][ $info['field_name'] ] = $info;
    

    Seems to be some weird spacing here.

  2. +++ b/core/modules/node/src/Plugin/migrate/builder/d7/Node.php
    @@ -21,12 +21,11 @@ class Node extends BuilderBase {
    -    $fields = [];
    +    // Read all field instance definitions in the source database.
    +    $fields = array();
    

    Now you're removing new array syntax for old style :)

  3. +++ b/core/modules/node/src/Plugin/migrate/builder/d7/Node.php
    @@ -21,12 +21,11 @@ class Node extends BuilderBase {
    +      $fields[ $info['entity_type'] ][ $info['bundle'] ][ $info['field_name'] ] = $info;
    

    and here

  4. +++ b/core/modules/taxonomy/migration_templates/d6_taxonomy_term.yml
    @@ -14,13 +14,9 @@ process:
    -    -
    -      plugin: skip_on_empty
    -      method: process
    -      source: parent
    -    -
    -      plugin: migration
    -      migration: d6_taxonomy_term
    

    Didn't this skip the migration call when the parent was 0? Eg, no hierarchy?

  5. +++ b/core/modules/taxonomy/src/Plugin/migrate/source/Term.php
    @@ -2,41 +2,62 @@
    +    if ($this->getModuleSchemaVersion('taxonomy') >= 7000) {
    
    @@ -52,6 +73,10 @@ public function fields() {
    +    if ($this->getModuleSchemaVersion('taxonomy') >= 7000) {
    

    I think we're still fine but I wonder how far we go with this before we just go back to a d6 and a d7 source.

  6. +++ b/core/modules/taxonomy/src/Tests/Migrate/d7/MigrateTaxonomyTermTest.php
    @@ -0,0 +1,87 @@
    +    /** @var \Drupal\taxonomy\TermInterface $entity */
    +    $entity = Term::load($id);
    

    You shouldn't need this if ::load has the correct docs. Maybe an issue in core.

mikeryan’s picture

You shouldn't need this if ::load has the correct docs. Maybe an issue in core.

FWIW, I always have to do that with Migration::load(). Ultimately Entity::load() is called, which has @return static|null. Maybe an issue in PhpStorm - can other tools work out what class is being returned from that "static"?

benjy’s picture

Maybe an issue in PhpStorm - can other tools work out what class is being returned from that "static"?

PHPStorm can definitely work it out when you have return static, I just tested it and it seems it's the |null part throwing phpstorm off.

phenaproxima’s picture

Addressed #50:

  1. Fixed.
  2. I think I prefer [] syntax when it's inline, like foo(['a', 'b', 'c']) :)
  3. Fixed.
  4. The skip_on_empty was unnecessary -- the migration process plugin skips processing on empty anyway.
  5. I think that's about as far as we'll need to go with it.
ultimike’s picture

Status: Needs review » Reviewed & tested by the community

Assuming all tests pass, I think we're good to go here.

-mike

  • webchick committed bc05470 on 8.0.x
    Issue #2410875 by phenaproxima, miguelc303, mikeryan, ultimike, benjy:...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesomesauce!!

I tested this locally, and confirmed that vocabs and tags are being moved over (interestingly, I end up with two "Tags" vocabs; one that was already existing in D8 via Standard profile and the other that was moved in via the migration). However, the tags do not show up on article view, nor does the field itself show up in the form. Adam says that's because of #2563819: [D7] Field display settings do not migrate, so off to review that one next.

Nevertheless, this one is definitely a huge step forward, so...

Committed and pushed to 8.0.x. Thanks!

webchick’s picture

Status: Fixed » Needs work
Issue tags: +Needs tests
FileSize
44.52 KB

Oh, shucks, I spoke too soon. When I applied the patch in #2563819: [D7] Field display settings do not migrate and re-ran the migration, it becomes apparent that instead of doing term associations, the migration is doing node associations. D'oh!

Entities listed under tags are nodes, not terms

So have to revert this for now. Hopefully an easy fix. And obviously we need to fix our test coverage, too. ;)

  • webchick committed fbe9087 on 8.0.x
    Revert "Issue #2410875 by phenaproxima, miguelc303, mikeryan, ultimike,...
phenaproxima’s picture

This oughta fix it. And I've added test assertions to prove it.

EDIT: The problem here is that taxonomy term reference fields are migrated to entity reference fields by the d7_field migration, but the migration wasn't bothering to adjust the field settings (like such as, to set the target entity type). So the fields were referencing nodes, not terms, by default.

mikeryan’s picture

+++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldSettings.php
@@ -24,10 +24,19 @@ class FieldSettings extends ProcessPluginBase {
+          $value['default_image'] = array('uuid' => '');

Not worth rerolling for on its own, but could do [] instead of array() here.

BTW, someone promised he'd rewrite labels according to #2565791: Fix migration labels on this go-round...

phenaproxima’s picture

The last submitted patch, 59: 2410875-59-FAIL.patch, failed testing.

The last submitted patch, 59: 2410875-59.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 61: 2410875-61.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
58.48 KB
1 KB

*pouts*

Status: Needs review » Needs work

The last submitted patch, 65: 2410875-65.patch, failed testing.

The last submitted patch, 59: 2410875-59.patch, failed testing.

The last submitted patch, 59: 2410875-59-FAIL.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
59.25 KB
657 bytes

When I get an ulcer, I'm going to name it testbot.

The last submitted patch, 61: 2410875-61.patch, failed testing.

The last submitted patch, 65: 2410875-65.patch, failed testing.

ultimike’s picture

Status: Needs review » Reviewed & tested by the community

Rock and roll - this looks good.

-mike

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Rock! Committed and pushed to 8.0.x. Thanks!

  • webchick committed 10c624b on 8.0.x
    Issue #2410875 by phenaproxima, miguelc303, mikeryan, webchick, ultimike...

Status: Fixed » Closed (fixed)

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