Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The D7 User migration plugin fails to migrate cck fields (e.g. file fields and taxonomy term reference fields) because in the field database table some of the D8 fields (e.g. field_FIELD_NAME_target_id) have different names to D7 (e.g. field_FIELD_NAME_tid) and so require a custom transform function.
Proposed resolution
Use the cck plugin manager in a similar manner to the D7 Node migration plugin because that is able to handle cck fields (e.g. file fields and taxonomy term reference fields).
Remaining tasks
Review and commit.
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#73 | migrate_d7_cck_fields-2673960-73.patch | 12.15 KB | jofitz |
#73 | interdiff_70-73.txt | 1.35 KB | jofitz |
#31 | 71155908.jpg | 93.67 KB | phenaproxima |
Comments
Comment #2
jofitz CreditAttribution: jofitz commentedUse a field-type-specific migration process plugin if there is one available (similar to d7_node).
Comment #3
jofitz CreditAttribution: jofitz commentedComment #4
jofitz CreditAttribution: jofitz commentedI confused a couple of issues (see also #2674088: Unable to migrate D7 image fields) - I have made the adjustments to the Summary, but the patch filenames will look a little odd because they reference image fields rather than taxonomy term reference fields. Oops!
Comment #5
benjy CreditAttribution: benjy at PreviousNext commentedIs this patch missing the actual process plugin itself?
Comment #6
jofitz CreditAttribution: jofitz commented@benjy No, the Taxonomy Term Reference process plugin already exists presumably for use by the D7 Node migration.
Comment #7
benjy CreditAttribution: benjy at PreviousNext commentedOK, great we just need some tests then.
Comment #8
quietone CreditAttribution: quietone as a volunteer commentedComment #10
jofitz CreditAttribution: jofitz commentedUpdated for D8.1.1, i.e. edits to User migration plugin rather than builder.
Comment #11
vasi CreditAttribution: vasi at Evolving Web commentedWe already have Drupal\migrate_drupal\Plugin\migrate\CckMigration that does this sort of processing. Can you build on top of that, instead of copying the code?
Comment #12
vasi CreditAttribution: vasi at Evolving Web commentedComment #13
jofitz CreditAttribution: jofitz at ComputerMinds commentedAdded tests.
(integration of CckMigration to follow)
Comment #16
jofitz CreditAttribution: jofitz at ComputerMinds commentedAdded tests.
(integration of CckMigration to follow)
N.B. Test user language changed from '' (empty string) to 'en' because fields (with a language) cannot be associated with users without a language (see #2671312: No default value for User langcode when migrating D7 users with no language).
Comment #19
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrect patches uploaded.
Comment #22
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrect test failure.
Comment #24
jofitz CreditAttribution: jofitz at ComputerMinds commentedBuild on CckMigration, as suggested by @vasi.
Comment #25
hussainwebFixing two really small nitpicks.
Comment #26
phenaproximaAssigning to myself for review.
Comment #27
jofitz CreditAttribution: jofitz at ComputerMinds commentedHow's the testing going, @phenaproxima? After 13 days I assume it's been a very thorough test :p
Comment #28
phenaproximaDang, this got lost in the flow. Thanks for the reminder, @Jo Fitzgerald. Anyway, this is very close to RTBC. I have one question and a few nitpicks.
I'm not sure about this. What was the reasoning behind adding this dependency?
Let's call this createNodeType(), for clarity's sake.
I think we might want $this->randomMachineName() or similar; IIRC, randomString() can include punctuation and other gibberish, which can lead to breaks and other weirdness in some circumstances.
s/id/ID
Let's use assertSame() here.
And here.
So glad to see that my Deep Space Nine reference is still intact. =P
Comment #29
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-roll patch prior to code review changes.
Comment #30
jofitz CreditAttribution: jofitz at ComputerMinds commentedThanks for the code review.
createType()
from MigrateFieldInstanceTest.php so have kept the function name the same for consistency. Also it is creating CommentTypes too, not just NodeTypes. I haven't made the change, but am happy to do so if required.label
is only the human-readable name sorandomString()
should be perfectly adequate. I haven't made the change, but am happy to do so if required.assertEquals()
toassertSame()
.assertEquals()
toassertSame()
.Comment #31
phenaproximaFor you, my fellow DS9 fan:
EDIT: Punches it in the face. Stupid typos.
Comment #34
jofitz CreditAttribution: jofitz at ComputerMinds commentedMade change requested by phenaproxima (
randomString()
->randomMachineName()
) and corrected embarrassingly simple test fail that should've been caught in local testing.Comment #37
jofitz CreditAttribution: jofitz at ComputerMinds commentedTest now passes - appears to have been a testbot glitch.
Comment #38
jofitz CreditAttribution: jofitz at ComputerMinds commented@phenaproxima would you mind taking a look at this, please? Previously you had said it was close to RTBC, I'm sure it's even closer now - let's push it over the line. Thanks.
Comment #39
mitrpaka CreditAttribution: mitrpaka as a volunteer commented#34 needs reroll
Comment #40
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-roll
Comment #41
phenaproximaAssigning to myself for review and shepherding to commit.
Comment #42
phenaproximaLooking again at this patch, I am wondering why it doesn't even mention taxonomy term references. I guess the IS is outdated?
Comment #43
jofitz CreditAttribution: jofitz at ComputerMinds commentedUpdated the IS coz it was waaay out of date.
Comment #44
phenaproximaDoes this file actually exist in the DB? According to my grepping in drupal7.php (as it currently exists in the 8.3.x branch) it does not. It seems like that might break things down the line, since no file field should be referencing a non-existent file if Drupal is working correctly. Also, does
Let's add a continue here if $field_type is empty, just for the sake of sanity checking. Or throw an exception.
$field_file_target_id should probably be defaulted to NULL, and the doc comment updated accordingly.
Should be
!empty()
to account for falsy values.I would rather this was not an array. Just NULL or an int, as in the doc comment.
Use fetchField() to retrieve the scalar file ID, rather than an array.
Once you change to fetchField(), no need for this.
I think I now understand the problem this is fixing, so once these are fixed and tests are passing, I'd say this is RTBC.
Comment #45
jofitz CreditAttribution: jofitz at ComputerMinds commentedcontinue
added.fetchField()
used instead offetchCol()
.Comment #47
jofitz CreditAttribution: jofitz at ComputerMinds commentedI shoulda seen that coming.
Comment #49
jofitz CreditAttribution: jofitz at ComputerMinds commentedWrong patch
Comment #50
phenaproximaOmigod, a DS9 ASCII art in the tests? @Jo Fitzgerald, you are my new best friend.
Other than that, nothing about the patch strikes me as objectionable. I think this is good to go.
Comment #51
heddnNit: can we convert the ascii art into using vfsStream instead of a static file?
Comment #52
jofitz CreditAttribution: jofitz at ComputerMinds commented@heddn can you explain further, please? The static file was added in response to point 1 in #44 - the file is not actually referenced within this patch it exists because otherwise it "...might break things down the line, since no file field should be referencing a non-existent file if Drupal is working correctly."
Comment #53
heddnhttps://github.com/mikey179/vfsStream is what I'm referring to. It is a virtual filestream for use in phpunit. I used it in http://cgit.drupalcode.org/migrate_source_csv/tree/tests/src/Unit based on recommendations from @phenaproxima.
I see your point though. This is a modification of a fixture. So I think my comment in #51 was misguided. Back to RTBC.
Comment #54
jofitz CreditAttribution: jofitz at ComputerMinds commentedAha, I see. Thanks for explaining (and so promptly too)
Comment #56
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-roll.
Comment #57
jofitz CreditAttribution: jofitz at ComputerMinds commentedSetting back to RTBC in (hope and) expectation of a test pass after a straightforward re-roll.
Comment #59
xjmI love the DS9 ASCII art, but is it GPL?
Comment #60
quietone CreditAttribution: quietone as a volunteer commentedFound the ascii art at https://startrekasciiart.blogspot.co.nz/2011/05/deep-space-nine.html, with this comment about use.
All pictures are by Joshua Bell, unless otherwise noted. Any corrections to the attributions would be welcomed. Permission to re-use any ASCII art on this blog is granted, assuming attribution (if any) is retained.
Comment #61
jofitz CreditAttribution: jofitz at ComputerMinds commentedIs there a standard method for attributing such things, e.g.:
Comment #62
jofitz CreditAttribution: jofitz at ComputerMinds commentedThe patch still applies - must've been a testbot issue.
Unless attribution is needed in the ascii art file then this is ready to go.
Comment #63
mikeryanAttribution is a good community thing, even if not strictly required.
Comment #64
jofitz CreditAttribution: jofitz at ComputerMinds commentedAttributed ascii art.
Comment #65
mikeryanThanks!
Comment #67
jofitz CreditAttribution: jofitz at ComputerMinds commentedRan re-test - all fine.
Comment #68
mikeryanComment #70
jofitz CreditAttribution: jofitz at ComputerMinds commentedSimple re-roll.
Comment #71
phenaproximaSelf-assigning for review.
Comment #72
phenaproximaI think I finally grok this, and I think I see why the test proves that it works. I have only nitpicks, then this is RTBC.
Although...I'm not sure if we should postpone this on the renaming of the cckfield plugins, since that will require this to be rerolled (or vice-versa, depending on which one gets in first).
Should just be @param int, and the description should begin with (optional).
Nit: Should use [] instead of array().
Comment #73
jofitz CreditAttribution: jofitz at ComputerMinds commentedChanges made:
(optional)
at the beginning of the description.[]
instead ofarray()
.I have not changed
@param int|false
because more often than not the value will be false (as returned byfetchField()
when there is no record).Comment #74
phenaproximaGorgeous.
Comment #75
alexpottCommitted and pushed eb271e5 to 8.3.x and 453f74e to 8.2.x. Thanks!