Need to provide more information as to what the errors are. Basically they assume the type exists but for various reasons that may not be the case. For example if a custom or contrib migration is filtering a type like a node profile so it can be migrated to a different place. A couple simple migrate process steps fix this.

Proposed Solution:
The proposed solution would be to skip on error and mark the migration as failed when the destination node type doesn't exist, instead of mapping the bundle to node_type without checking If that bundle exist or not.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

Issue summary: View changes
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
3.52 KB
mikeryan’s picture

Status: Needs review » Needs work

Looks good, we need to be validating references with the migration plugin in general, but bumping back to "Needs work" until we get tests.

neclimdul’s picture

Don't have tests yet but this needed a reroll.

heykarthikwithu’s picture

Status: Needs work » Needs review

to test the reroll.

benjy’s picture

Status: Needs review » Needs work

Patch makes sense to me. NW for tests.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

Queued an 8.1.x test, let's see if we need another reroll.

Dries’s picture

FileSize
165.64 KB

I'm not sure if my problem is related but ... After I did a test migration of the content on buytaert.net from Drupal 7 to Drupal 8, I noticed that (1) I have a content type 'article' and (2) that node/1 became an 'article'. I don't have a content type 'article' on my Drupal 7 site and node/1 is of type 'page'. The type 'Article' might have been created by the default Drupal 8 install profile, but the conversion of node/1 from page to article might have been done by the migrate module? See screenshot.

effulgentsia’s picture

I was not able to reproduce #9 via a fresh D7 installation. When I tried a fresh D7 Standard profile install and created several 'page' nodes, they all (including node/1) migrated to 'page' nodes onto a fresh D8 Standard profile install. So there might be something peculiar about the buytaert.net database that caused the problem in #9.

benjy’s picture

but the conversion of node/1 from page to article might have been done by the migrate module?

If you're using the standard upgrade path than nid's are preserved which would have overwritten node 1 on the destination site with all the incoming data, this should have included the node type but that is a little weird.

If you delete that node before starting the migration do you have the same outcome? Any other issues around node types?

EDIT: I mean delete the node on the destination site, presuming you had an article there to begin with?

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

Issue tags: +migrate-d6-d8
heddn’s picture

Issue tags: +Baltimore2017
Derimagia’s picture

Status: Needs work » Needs review
FileSize
13.99 KB

Added tests to each migration for testing to make sure it will not migrate anything which has dependencies on node types which haven't been migrated.

Derimagia’s picture

joelpittet’s picture

@Derimagia looks like you accidentally uploaded the fix and removed the tests (aka reverse of what it looks like you intended)

heddn’s picture

Status: Needs review » Needs work
vinothg’s picture

We are working on this issue at DrupalCon Baltimiore at mentored core sprint table.

stimalsina’s picture

vinothg’s picture

Both patch #16 and #21 are same.

stimalsina’s picture

Issuing a new patch because the last patch had the yml files.

stimalsina’s picture

stimalsina’s picture

stimalsina’s picture

Status: Needs work » Needs review
paryank’s picture

Status: Needs review » Needs work

@stimalsina Patch #22 breaks the following Drupal coding standards. Please fix it.


FILE: core/modules/file/tests/src/Kernel/Migrate/d6/MigrateUploadEntityDisplayTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
----------------------------------------------------------------------
 52 | ERROR   | Doc comment short description must be on a single
    |         | line, further text should be a separate paragraph
 59 | WARNING | Line exceeds 80 characters; contains 86 characters
----------------------------------------------------------------------

FILE: core/modules/file/tests/src/Kernel/Migrate/d6/MigrateUploadEntityFormDisplayTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AND 3 WARNINGS AFFECTING 4 LINES
----------------------------------------------------------------------
 29 | WARNING | Line exceeds 80 characters; contains 82 characters
 51 | WARNING | Line exceeds 80 characters; contains 85 characters
 52 | ERROR   | Doc comment short description must be on a single
    |         | line, further text should be a separate paragraph
 59 | WARNING | Line exceeds 80 characters; contains 91 characters
----------------------------------------------------------------------

FILE: core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateVocabularyEntityDisplayTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AND 3 WARNINGS AFFECTING 4 LINES
----------------------------------------------------------------------
 33 | WARNING | [x] A comma should follow the last multiline array
    |         |     item. Found: 'd6_vocabulary_field_instance'
 52 | WARNING | [ ] Line exceeds 80 characters; contains 84
    |         |     characters
 53 | ERROR   | [ ] Doc comment short description must be on a single
    |         |     line, further text should be a separate paragraph
 60 | WARNING | [ ] Line exceeds 80 characters; contains 90
    |         |     characters
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

FILE: core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateVocabularyEntityFormDisplayTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AND 3 WARNINGS AFFECTING 4 LINES
----------------------------------------------------------------------
 33 | WARNING | [x] A comma should follow the last multiline array
    |         |     item. Found: 'd6_vocabulary_entity_display'
 57 | WARNING | [ ] Line exceeds 80 characters; contains 89
    |         |     characters
 58 | ERROR   | [ ] Doc comment short description must be on a single
    |         |     line, further text should be a separate paragraph
 65 | WARNING | [ ] Line exceeds 80 characters; contains 95
    |         |     characters
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

FILE: core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateVocabularyFieldInstanceTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AND 3 WARNINGS AFFECTING 5 LINES
----------------------------------------------------------------------
 32 | WARNING | [x] A comma should follow the last multiline array
    |         |     item. Found: 'd6_vocabulary_field'
 63 | ERROR   | [x] Inline comments must end in full-stops,
    |         |     exclamation marks, colons, question marks, or
    |         |     closing parentheses
 70 | WARNING | [ ] Line exceeds 80 characters; contains 91
    |         |     characters
 71 | ERROR   | [ ] Doc comment short description must be on a single
    |         |     line, further text should be a separate paragraph
 78 | WARNING | [ ] Line exceeds 80 characters; contains 90
    |         |     characters
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

timodwhit’s picture

Cleaned up and marking for review.

timodwhit’s picture

Status: Needs work » Needs review

Cleaned up and marking for review.

stimalsina’s picture

Rerolling the patch with coding standards.

The last submitted patch, 23: some_migrations_have-2597680-test_only_22.patch, failed testing.

The last submitted patch, 28: some_migrations_have-2597680-test_only_28.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 30: some_migrations_have-2597680-test_only_30.patch, failed testing.

vinothg’s picture

There were new 5 test cases, and all of them failed as expected. Re-roll the patch again with both code + test cases.

stimalsina’s picture

Rerolling the patch which includes the yml files from #17.

heddn’s picture

  1. +++ b/core/modules/file/tests/src/Kernel/Migrate/d6/MigrateUploadEntityDisplayTest.php
    @@ -46,4 +47,23 @@ public function testUploadEntityDisplay() {
    +    // that it failed, so record that in the map table.
    

    Nit: failure is a little strong. We could just just say it didn't occur. Because someone could have intentionally ignored it. Which is the more common scenario anyway. I actually don't think we want to fail this, rather just not ever migrate it. Plus I think this needs a re-roll.

vinothg’s picture

Issue summary: View changes
vinothg’s picture

Issue summary: View changes
stimalsina’s picture

Re-rolling for a clean patch. Also, includes comment fixes.

stimalsina’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Needs work » Needs review
Derimagia’s picture

@heddn Understandable, the idea of the tests and the comment was mainly copied from the MigrateTermNodeTest test

vinothg’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for fixing the code review comments @stimalsina.

I reviewed the patch #39 for Drupal coding standards check, checked for logic flaws and proper comments.

I applied this patch towards 8.4.x branch it applies cleanly and looks good to me.

Marking it as RTBC.

heddn’s picture

+1 on RTBC.

catch’s picture

Status: Reviewed & tested by the community » Needs work

This should be using the 'migration_lookup' plugin now that we've deprecated 'migration'.

paryank’s picture

Assigned: Unassigned » paryank
FileSize
0 bytes

I've updated the the migration templates to use 'migration_lookup' plugin instead of 'migration'.

paryank’s picture

Oops! Submitted a blank file. Uploading the new patch with corrections.

heddn’s picture

Assigned: paryank » Unassigned
Status: Needs work » Needs review

Changing status so tests can run

vinothg’s picture

Status: Needs review » Reviewed & tested by the community

I checked the patch, it applies cleanly on 8.4.x branch, all the occurrences of migration has been replaced with migration_lookup and passed the coding standards test.

Marking it as RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 46: some_migrations_have-2597680-46.patch, failed testing.

paryank’s picture

Status: Needs work » Needs review

I looked into the test results for failure in #49. I didn't find anything related to this patch. It looks like the test failed by some core commit yesterday, but now it is passing again. I am changing the status to "Needs review" to run the tests again.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

I re-queued the tests from #46. They are passing so back to RTBC.

  • Gábor Hojtsy committed 9b44a02 on 8.3.x
    Issue #2597680 by stimalsina, neclimdul, paryank, Derimagia, timodwhit,...

  • Gábor Hojtsy committed 699606e on 8.4.x
    Issue #2597680 by stimalsina, neclimdul, paryank, Derimagia, timodwhit,...
Gábor Hojtsy’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs tests

Reviewed myself and the changes look good to me. Thanks for the extensive test coverage across different types of related data. The concern from @catch has also been addressed.

Status: Fixed » Closed (fixed)

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