I tested it several times, last on a fresh D6 installation with Forum enabled, add one folder and one forum article.
Migration worked successful via UI or drush.
But in D8 there are to entities Forums, one empty and one with the migrated taxonomy structure.
the empty one is mandatory and therefore I cannot edit and save the forum article :-(
the other are filled with the migrated forum.
This is a critical bug because you cannot migrate a D6 Site with a large Forum.
Comment | File | Size | Author |
---|---|---|---|
#31 | 2690001-30.patch | 5.88 KB | quietone |
#30 | interdiff.txt | 1.51 KB | quietone |
#28 | interdiff.txt | 1.26 KB | quietone |
#28 | 2690001-28.patch | 5.75 KB | quietone |
#23 | interdiff.txt | 706 bytes | quietone |
Comments
Comment #2
catchThis looks like a duplicate of #2682705: Migrate process plugin does not save stubs to the idmap, leads to duplicates and broken references, which has a patch.
Comment #3
catchThat patch included some test coverage, but as far as I know we don't have test coverage for the following:
- take a 6.x site with forum/taxonomy references
- migrate to 8.x
- confirm the references are still correct
Since that's the bug that was reported here, re-opening for that extra test coverage.
Comment #4
infopicard CreditAttribution: infopicard as a volunteer commentedI just tested it - same issue :-(
https://dl.dropboxusercontent.com/u/4908150/D6_8_ForumMigError.png
as you can see, first "Forums" is the correct migrated taxonomy and the 2nd "Forums" is the empty but mandatory field in the Forum article : -(
Comment #5
catch@infopicard, just to confirm, you tested it with the 8.1.x branch (either via git clone or dev tarball)?
Comment #6
infopicard CreditAttribution: infopicard as a volunteer commented@catch
sorry for confusion, I tested it twice, 1. against 8.0.5 with patch-30 and 2.nd against 8.1.0beta1, where patch don't work
-> both with same effect. (see above)
I'm not familiar with development and testing procedure and using git or dev tarball :-(
but if I'll get some instructions, I would like to help, because I'm really need this feature and I cannot migrate our larger forum website without getting trouble :-(
Thank you for understanding and feel free to contact me.
Comment #7
hosef CreditAttribution: hosef at Hook 42 commentedComment #8
quietone CreditAttribution: quietone as a volunteer commentedAdd tag.
Comment #9
vasi CreditAttribution: vasi at Evolving Web commentedWe already have a round-trip test for term references: \Drupal\Tests\taxonomy\Kernel\Migrate\d6\MigrateTermNodeTest .
The problem here is actually that when we migrate vocabularies, we deduplicate them. In case the D8 site already has a vocabulary 'foo', a D6 vocabulary 'foo' will be renamed 'foo1'. But this does the wrong thing for forums, where we definitely want to migrate into the existing 'forums' vocabulary.
I guess we're deduping in this case just because D6 vocabs don't have machine_names. But what we really want isn't "make sure this vocab has a globally unique name". It's "make sure this vocab has a unique name amongst migrated vocabs". Maybe we can have a new kind of dedupe, that checks if the "duplicate" is in the IdMap?
Comment #10
vasi CreditAttribution: vasi at Evolving Web commentedOk, here's a fix for the taxonomy duplication. But there's more!
We'll definitely need to test these, and find a solution.
In particular, is there any good existing way to munge field names in our migrations?
Comment #12
hosef CreditAttribution: hosef at Hook 42 commentedI'm unassigning this because my system is too far ahead to setup a working D6 forum.
Comment #13
mikeryanEdit: please disregard my misguided rant, I missed that these issues are not trying to do what I was railing about...
Thinking about this some more, especially in light of #2716133: Roles are duplicated instead of updated existing roles (another issue where we want to avoid deduping in at least some instances)...In the general case, code alone can not determine whether, when it sees a 'foobar' on the source side and a 'foobar' on the destination side, they represent the same thing to the site-builder (and should be mapped as-is) or whether they should represent different things (and thus the source-side foobar should become a new thing on the destination side). Ideally, the tools would allow the migrator to decide for each source item whether to map it to an existing destination item, or create a new one - and indeed, migrate_d2d in D7 provided this option for both roles and vocabularies. And it is my intent to provide that functionality in the D8 version of migrate_d2d, regardless of the default core behavior.So, the question is, what should the default core behavior be - specifically the uncustomized "upgrade" scenario. This scenario is assuming an "empty" Drupal 8 site - one which has been installed from some install profile, and has any desired modules enabled, without any substantial configuration or content creation beyond that. Our problem here is that site isn't quite empty - the standard profile creates an administrator role and tags vocabulary, the forum module adds a forums vocabulary, etc. At least in core, the things that create conflict are carryovers from older versions of Drupal and most definitely *should* be mapped directly without deduping. And, more broadly, if you've created a new Drupal 8 site to hold your old Drupal 6/7 content and configuration, if something is named the same on both sides it's almost certainly because they have the same meaning and role for you.
Or, another angle on this - the core upgrade process is intended to replicate the old site as precisely as possible in D8, and thus should not rename things if at all avoidable.Thus, my opinion is that the default vocabulary and role migrations in core should *not* dedupe - they should overwrite the "conflicting" entities.Thoughts?Comment #14
mikeryan*sigh* - guess I needed to look more closely at the actual vocabulary migrations as they are at the moment, deduping is only being done for D6 to make sure machine names generated from similar human-facing names don't conflict.
<emily_litella>Never mind.</emily_litella>
Comment #16
quietone CreditAttribution: quietone as a volunteer commentedPatch no longer applies, so reroll.
Comment #18
quietone CreditAttribution: quietone as a volunteer commentedFrom #10,
This variable is not used in the either the D6 or the D7 test fixture. It will have to be added.
Comment #19
quietone CreditAttribution: quietone as a volunteer commentedComment #20
quietone CreditAttribution: quietone as a volunteer commentedChanged the input array to lookupSourceID to be an array keyed by field name. It just might pass tests now. But, it will still need work as per #10 and #18 regarding forum_containers.
Comment #21
quietone CreditAttribution: quietone as a volunteer commentedWorks better with the patch.
Comment #23
quietone CreditAttribution: quietone as a volunteer commentedSilly to forget to fix DedupeEntityTest.
Comment #24
quietone CreditAttribution: quietone as a volunteer commentedThis will dedupe if the entity is not in the id map. But it doesn't check the status of that migration. Should it?
Comment #25
phenaproximaThis makes sense if I squint at it, but it's quite difficult to parse. Can we use Prophecy here instead, purely for readability reasons?
That's my only complaint. Beyond that, I think this patch makes sense.
Comment #26
heddnMarking NW for #25. And novice because should be a fairly easy introduction to PHPUnit, if someone wants to pick this up.
Comment #27
iMiksuLets see if we can do something about #25 today.
For reference I've found this page: https://www.drupal.org/docs/8/phpunit/using-prophecy
Comment #28
quietone CreditAttribution: quietone as a volunteer commentedChanged to Prophecy as per #25.
Comment #29
phenaproximaI doubt this is going to scale well on very large migrations, but until ID maps support looking up a large set of source IDs at once, we might be stuck with it. Would like to get @mikeryan or @neclimdul's opinion about this.
I don't understand why the query mock is being included in the value map. This needs a comment at least.
I'd rather we didn't assert on exactly how many times the method was called. If we must, can we assert that it's called at least 3 times, rather than exactly 3 times? That's coupling the test a little too closely to the internal implementation than I'm comfortable with.
Same here.
Comment #30
quietone CreditAttribution: quietone as a volunteer commented2. Added some comments. Better?
3-4. Removed the number of times the method will be called.
Comment #31
quietone CreditAttribution: quietone as a volunteer commentedHmm, I was sure uploaded the patch.
Comment #32
mikeryanIf I understand correctly, it will iterate only over matches to the value we're deduping, and since we're deduping it there shouldn't normally be more than one in practice (barring a non-migration process creating many dupes) - right? I think the possible scalability issue is edge-casey enough that we don't need to spend a lot of cycles optimizing for it.
Comment #33
quietone CreditAttribution: quietone as a volunteer commented@mikeryan, I agree with your summary.
Comment #34
mikeryanManually tested with a D6 site - the forum taxonomy is properly deduped, so good as far as that goes. But, forum content is still not hooked to the containing forum, and the first view of a forum node after a cache rebuild gets array_flip errors. Looks like vasi's issues in https://www.drupal.org/node/2690001#comment-11290787 weren't addressed:
That being said, this patch does address the vocabulary angle well, and we have an existing issue for the field duplication/array_flip() at #2637286: Migrated forum topics generate array_flip warning in EntityStorageBase, so I say we go ahead and commit this, and pursue the field angle in that issue.
Comment #35
mikeryanLet's have a change record to document the new 'migrated' option on the migration process plugin.
Comment #36
quietone CreditAttribution: quietone as a volunteer commentedStill new to change records, but at least it is a start.
dedupe_entity process plugin now takes optional 'migrated' option
Comment #37
catchCommitted/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!
Comment #41
quietone CreditAttribution: quietone as a volunteer commentedPublished change record.