Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#46 | some_migrations_have-2597680-46.patch | 14.9 KB | paryank |
#9 | page-article.jpg | 165.64 KB | Dries |
Comments
Comment #2
neclimdulComment #3
mikeryanLooks good, we need to be validating references with the migration plugin in general, but bumping back to "Needs work" until we get tests.
Comment #4
neclimdulDon't have tests yet but this needed a reroll.
Comment #5
heykarthikwithuto test the reroll.
Comment #6
benjy CreditAttribution: benjy at PreviousNext commentedPatch makes sense to me. NW for tests.
Comment #8
mikeryanQueued an 8.1.x test, let's see if we need another reroll.
Comment #9
Dries CreditAttribution: Dries commentedI'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.
Comment #10
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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.
Comment #11
benjy CreditAttribution: benjy at PreviousNext commentedIf 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?
Comment #14
mikeryanComment #15
heddnComment #16
Derimagia CreditAttribution: Derimagia as a volunteer and at Mindgrub Technologies commentedAdded 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.
Comment #17
Derimagia CreditAttribution: Derimagia as a volunteer and at Mindgrub Technologies commentedAdding test only patch
Comment #18
joelpittet@Derimagia looks like you accidentally uploaded the fix and removed the tests (aka reverse of what it looks like you intended)
Comment #19
heddnComment #20
vinothg CreditAttribution: vinothg commentedWe are working on this issue at DrupalCon Baltimiore at mentored core sprint table.
Comment #21
stimalsina CreditAttribution: stimalsina as a volunteer and commentedWe have a cleanly applying patch from #16. Here's a new patch with only the tests.
Comment #22
vinothg CreditAttribution: vinothg commentedBoth patch #16 and #21 are same.
Comment #23
stimalsina CreditAttribution: stimalsina as a volunteer and commentedIssuing a new patch because the last patch had the yml files.
Comment #24
stimalsina CreditAttribution: stimalsina as a volunteer and commentedComment #25
stimalsina CreditAttribution: stimalsina as a volunteer and commentedComment #26
stimalsina CreditAttribution: stimalsina as a volunteer and commentedComment #27
paryank CreditAttribution: paryank at The College Board commented@stimalsina Patch #22 breaks the following Drupal coding standards. Please fix it.
Comment #28
timodwhit CreditAttribution: timodwhit commentedCleaned up and marking for review.
Comment #29
timodwhit CreditAttribution: timodwhit commentedCleaned up and marking for review.
Comment #30
stimalsina CreditAttribution: stimalsina as a volunteer and commentedRerolling the patch with coding standards.
Comment #34
vinothg CreditAttribution: vinothg commentedThere were new 5 test cases, and all of them failed as expected. Re-roll the patch again with both code + test cases.
Comment #35
stimalsina CreditAttribution: stimalsina as a volunteer and commentedRerolling the patch which includes the yml files from #17.
Comment #36
heddnNit: 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.
Comment #37
vinothg CreditAttribution: vinothg commentedComment #38
vinothg CreditAttribution: vinothg commentedComment #39
stimalsina CreditAttribution: stimalsina as a volunteer and commentedRe-rolling for a clean patch. Also, includes comment fixes.
Comment #40
stimalsina CreditAttribution: stimalsina as a volunteer and commentedComment #41
Derimagia CreditAttribution: Derimagia as a volunteer and at Mindgrub Technologies commented@heddn Understandable, the idea of the tests and the comment was mainly copied from the MigrateTermNodeTest test
Comment #42
vinothg CreditAttribution: vinothg commentedThanks 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.
Comment #43
heddn+1 on RTBC.
Comment #44
catchThis should be using the 'migration_lookup' plugin now that we've deprecated 'migration'.
Comment #45
paryank CreditAttribution: paryank at The College Board commentedI've updated the the migration templates to use 'migration_lookup' plugin instead of 'migration'.
Comment #46
paryank CreditAttribution: paryank at The College Board commentedOops! Submitted a blank file. Uploading the new patch with corrections.
Comment #47
heddnChanging status so tests can run
Comment #48
vinothg CreditAttribution: vinothg commentedI 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.
Comment #50
paryank CreditAttribution: paryank at The College Board commentedI 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.
Comment #51
heddnI re-queued the tests from #46. They are passing so back to RTBC.
Comment #54
Gábor HojtsyReviewed 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.