Problem/Motivation
This is part of #2507607: [META] Replace Cthulhu-forsaken load plugins with migration builders.
Currently, CCK field values are migrated using a pair of dynamically-loaded migrations (d6_cck_field_values and d6_cck_field_revision) with dedicated source plugins.
Once the builder plugin type is implemented, it will be possible to generate individual migrations for each node type, each detailing which fields should be migrated for that type. This means that the d6_cck_* migrations will be obsolete, but it also means that the responsibility of pulling field values from the database will fall on the d6_node source plugin.
Proposed Resolution
Write a patch that copies CCK field value retrieval logic into the d6_node source plugin (and, by extension, d6_node_revision). For now, this functionality should be in core alongside the existing CCK handling, which will be removed when the builder plugin type lands.
This will also serve as the model for handling fieldable entities in the Drupal 7 migration path -- entity source plugins should be responsible for retrieving the field values. (In the Drupal 7 migration path, this will probably be implemented as a trait since any entity type can have fields. But for Drupal 6, it can be confined to the node source plugin.)
Remaining Tasks
Review and commit the patch.
API/UI Changes
None.
Comment | File | Size | Author |
---|---|---|---|
#42 | interdiff-2538000-28-42.txt | 22.88 KB | phenaproxima |
#42 | 2538000-42.patch | 31.73 KB | phenaproxima |
#28 | interdiff-2538000-25-28.txt | 6.1 KB | phenaproxima |
#28 | 2538000-28.patch | 7.63 KB | phenaproxima |
#25 | interdiff-2538000-19-25.txt | 436 bytes | phenaproxima |
Comments
Comment #1
phenaproximaComment #3
phenaproximaNote to self: don't try to do logic in the constructor. It leads to the dark side. (At least in the case of migrations, where just installing one will instantiate the source and destination plugins.)
Comment #5
phenaproximaC'mon, testbot, don't be like that...
Comment #6
phenaproximaAdded a test, and fixed a few bugs revealed by it.
Comment #8
phenaproximaOK -- I'm totally abandoning my previous approach, which was to make the d6_field plugin the canonical source for field and field instance data. It's just too painful to try to invoke a source plugin from within another source plugin -- Migrate's architecture makes this extraordinarily difficult to do, much less test.
This new patch simply has d6_node read CCK information directly from the DB, no middleman required. All the methods I added for this have individual unit tests (in NodeTest) and could easily be moved into a trait if needed.
The changes between this and #6 are seismic enough that an interdiff would be more confusing than revealing, so I haven't included one. Consider this patch a reboot of the issue.
Comment #10
phenaproximaAs they said in the original 1968 version of The Producers: "[Here's] to failure!"
Comment #12
phenaproximaOkay, well, the tests are passing locally for me now...
Comment #15
quietone CreditAttribution: quietone commentedre-roll.
Comment #16
quietone CreditAttribution: quietone commentedI was going to change assertSame to assertIdentical and possibly add more tests. However, I'm not able to reach any breakpoint in the test. How do I run this test?
Comment #17
quietone CreditAttribution: quietone commentedComment #18
quietone CreditAttribution: quietone commentedRIght, just learned that PHPUnit doesn't have assertIdentical.
Comment #19
BerdirassertSame() in phpunit is the same as assertIdentical() in simpletest.
Comment #20
quietone CreditAttribution: quietone commentedAdded some tests
Comment #21
benjy CreditAttribution: benjy commentedAs long as we can migrate nodes without the the CCK module we can combine those. Looks like it's been handled by the moduleExists() call?
What is meant by this in the IS?
What's the @ for?
Comment #22
phenaproximaWhen I wrote that, I meant to say that the dedicated CCK migrations (and CCK plugins) will be removed from core shortly after builders land, so we need to get this patch in ASAP.
The ?: operator will produce an E_NOTICE if $this->fieldInfo[$node_type] is undefined, so the @ is there to silence that. It's a more concise way of saying
isset($this->fieldInfo[$node_type]) ? $this->fieldInfo[$node_type] : []
. In PHP 7, the same thing will be doable with the ?? operator.Comment #23
benjy CreditAttribution: benjy commentedI feared as much, I don't think this is a pattern we use in core.
Comment #24
phenaproximaThis blocks #2549013: Remove load plugins and is therefore a Migrate-critical blocker.
Comment #25
phenaproximaCorrected the syntax.
Comment #26
benjy CreditAttribution: benjy commentedFew small things:
Does this need an issue?
Missing functiond oc.
Might be worth expanding these
Comment #27
phenaproximaThe d6_node builder also needs to be extended to merge CCK fields into the d6_node migration variants: see https://www.drupal.org/node/2549013#comment-10240207.
Comment #28
phenaproximaLooks like it's already there, so I think this is good to go.
#26-1: Nope; I put that functionality into this new patch.
#26-2: Fixed.
#26-3: Most of those aren't needed for the test, so I just turned them into empty arrays.
I also removed several direct tests of protected methods on the D6 Node source; @benjy gave me a come-to-Jesus explanation of why directly testing protected methods is wrong.
Just to be clear, this completely obviates the need for the d6_cck_field_values and d6_cck_field_revision migrations. One of the only blockers left before we can kick load plugins out.
Comment #32
mikeryanSo, this is dying due to memory consumption:
In my local environment, also with a memory_limit of 320M, it works fine. Two obvious possibilities:
I've submitted #2281393: D6->D8 Blocks - Custom titles not imported for retest as a control - if it succeeds, that suggests #1, if it fails it suggests #2. If it's #2, that raises the urgency of #2503999: Large volume entity migrations run out of memory.
Comment #33
mikeryanWell #2281393: D6->D8 Blocks - Custom titles not imported failed, but with an entirely different error - MigrateDrupal6Test passed, no memory problem, which suggests that the patch here is somehow triggering more memory consumption. But, as I said, it works locally with the same memory_limit...
Comment #34
webchickThis is blocking #2549013: Remove load plugins which in turn blocks Comment/Taxonomy content migrations for D7 => D8.
Comment #35
benjy CreditAttribution: benjy at PreviousNext commentedWhen we added builders they were meant to be alongside load plugins, I don't see why we *have* to remove load plugins to do comment/taxonomy, the existing API should work to complete those D7/D8 migrations we still have LoadTermNode although as an example.
It's a shame we did the builders and attempted to remove load plugins in 8.0 that has taken a long time and held up many of the D7 migrations.
Comment #36
mikeryanphenaproxima can speak to the specifics of how the load plugins are blocking the D7 migrations, but my understanding was always that the intention of builders was to get rid of the load plugins. The genesis of the builders was #2507607: [META] Replace Cthulhu-forsaken load plugins with migration builders, after all.
Comment #40
phenaproximaComment #41
benjy CreditAttribution: benjy at PreviousNext commentedSure, the plan is to get rid of load plugins but that's more work and if time is short we could not do that. builders transform the migrations before they're run, load plugins can still do that at run time today so they are both able to solve the term/cck problems in their own way. However, if there is time to get builders done and remove load plugins great.
Comment #42
phenaproximaHere's a version that removes the d6_cck_field_values and d6_cck_field_revision migrations, which are obviated by this patch and which will make it easier to kill load plugins once this lands.
Comment #43
mikeryanMight be good to document the magic 6001 schema version here (presumably CCK changed its schema in such a way we don't want to try to support the previous version?).
This seems problematic if there's a field whose name is the prefix for another field name - this is an issue for migrate_d2d as well (#2078913: D5 to D7 migration fails if one source CCK field is a substring of another). See below...
$db sounds like it's referencing a database - perhaps $db_schema to be clear?
Can we use '__' as the separator here, and use that above to recognize when a column is prefixed by the right field?
Comment #44
phenaproxima1. I would have if I knew what the deal was with it, but I lifted that almost directly out of the CckFieldValues source, which doesn't document it either.
2. Shouldn't happen; the values are queried separately for each field, so by the time they reach prepareRow() $values should look like this:
3. I felt that it read better as "DB table exists" instead of "DB schema table exists", but I can rename it if you want.
4. Can't change the separator, because I'm adding a column that actually exists in the DB table. field_foo_value will exist, but field_foo__value won't. :)
Comment #45
mikeryanIt does help to have the CCK removal in here, so we can see that the node CCK code didn't come out of thin air, the logic has moved (and it also helps to see the loc-- benefit).
I've tested this with my own site using migrate_upgrade - my fields migrated successfully within the node migrations, without those separate field-only migrations, that's great! Code looks good - I'm ready to see this go in!
Comment #46
webchickHot damn!
Read through the non-deleted parts of this patch, nothing much stood out.
Asked a few minor questions in IRC, got them answered, this looks good to go, and unblocks all kinds of other stuff, so let's get 'er done!
Committed and pushed to 8.0.x. Thanks!
Comment #48
phenaproximaFollow-up issue created for preserving the assertions from MigrateCckFieldValuesTest and MigrateCckRevisionTest: #2557557: Move old CCK assertions into MigrateNodeTest
Comment #49
quietone CreditAttribution: quietone commentedAwesome!
For the first ever my tiny d6 site, 5 nodes and some CCK fields, actually migrated. Previously, the fields would be created but not the value. Today I have values! And for a file field too!
For me, it feels like I can do some serious testing now.
Comment #50
webchickWow, that's awesome!! :D Great news, quietone, and thanks so much for all your help!
Comment #51
quietone CreditAttribution: quietone commentedSeems I spoke to soon. File uploads are working but not file fields.