Problem/Motivation
When migrating taxonomy terms from Drupal 7 to 8, the field instance configuration for fields added to the taxonomy_term entity and the data within them is not coming across. I believe #2675470: D7 field instance settings for any fields not on nodes are not migrating covers the migration of field instance configuration for both taxonomy terms, users and other fieldable entities. This issue just covers the migration of the data within those fields for taxonomy terms.
The reason field data is not coming across is that the term migration plugin is extending DrupalSqlBase
rather than FieldableEntity
.
Proposed resolution
* Separate the existing term migration plugin into separate plugins for D6 and D7.
* Extend the D7 plugin from FieldableEntity
* Modify the prepareRow() method to pull in the fields - linking to the taxonomy_vocabulary table is necessary to get the entity bundle (or taxonomy_vocabulary machine_name) for this to work
* Due to the stub creation in the case of child terms being created before their parents, we need to ensure that 'iterator' plugin can handle this situation where $value
is not an array or is NULL.
Remaining tasks
Probably check that other fieldable entities don't have similiar issues.
Create a change record (because taxonomy_term has been split into d6_taxonomy_term & d7_taxonomy_term).
Comments
Comment #2
stella CreditAttribution: stella at Annertech commentedComment #4
stella CreditAttribution: stella at Annertech commentedComment #6
stella CreditAttribution: stella at Annertech for Limerick City & County Council commentedUpdated the migration templates
Comment #8
jofitz CreditAttribution: jofitz at ComputerMinds commentedAdd tests for fieldable taxonomy terms. Separate d6 and d7 tests.
Comment #9
jofitz CreditAttribution: jofitz at ComputerMinds commented* Enable processing of taxonomy term fields.
* Test migration of taxonomy term fields.
Comment #11
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrect errors introduced into other tests.
Comment #12
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #13
vasi CreditAttribution: vasi at Evolving Web commentedThis is great work!
Let's not just silently return an empty array if the input is something totally different, eg: an integer. Can we just check for NULL?
This isn't tested anywhere.
This is also not tested anywhere. Maybe we should do profile fields in a follow up.
This might have the benefit of letting us share code with D7NodeDeriver.
Comment #14
vasi CreditAttribution: vasi at Evolving Web commentedPlease build on top of Drupal\migrate_drupal\Plugin\migrate\CckMigration, instead of building a custom process.
Comment #15
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech for Limerick City & County Council commentedThis needed a re-roll following #2691641: D7 taxonomy description format not migrating being committed.
Comment #16
jofitz CreditAttribution: jofitz at ComputerMinds commentedResponses to @vasi's code review:
Also now based on CckMigration.
Comment #18
jofitz CreditAttribution: jofitz at ComputerMinds commentedUploaded correct patch.
Comment #19
hussainwebFirst doing a reroll.
Comment #20
hussainwebHere are a few tweaks. Mostly, they are changes in array syntax but there is also a missing field declaration. I also made the patch smaller (and easier to review) by detecting renames.
Comment #21
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech for Limerick City & County Council commentedNeeds a re-roll since #2737607: Migration Source Plugins are not extendable because of ambiguous database field names.
Note: the patch in #20 applies cleanly against tag 8.1.7, for anyone who is already using this.
Comment #23
tom friedhof CreditAttribution: tom friedhof at ActiveLAMP commentedHere is a rerolled patch that will apply cleanly against 8.1.8 or 8.1.x branch
Comment #27
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-roll.
Comment #31
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrect re-roll errors.
Comment #32
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #36
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-roll (due to issues with git diff).
Comment #38
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrect re-roll errors.
Comment #40
jofitz CreditAttribution: jofitz at ComputerMinds commentedNow passing all tests, appears to have been a fault in the testbot.
Comment #41
chriscalip CreditAttribution: chriscalip commentedA shallow review test has pass : 1.
Scenario
D7 Install with 5 vocabularies.
Vocabulary asset_category has field field_cap_category_level
Vocabulary asset_status has field field_status_text
Migrating to clean D8 3.x Install.
Vocabulary asset_category has field field_category_level
Vocabulary asset_status has field field_status_text
Using migration map migrate_plus.migration.taxonomy_vocabulary.yml
Screenshots :
a.) 2746671-taxonomy-migration-success.png , shows migration success for taxonomy_term and has no errors.
b.) 2746671-taxonomy-migration-data-check.png , shows field data values migrated also.
Comment #42
chriscalip CreditAttribution: chriscalip commentedAnother successful use of this patch : +1
Scenario
D7 Install with 5 vocabularies.
Vocabulary asset_category has field field_cap_category_level (list)(unlimited)
Vocabulary asset_status has field field_status_icon (imagefield)
Vocabulary conversation_category has field field_tester (entityreference)
Migrating to clean D8 8.2.0-rc1 Install.
Vocabulary asset_category has field field_category_level (list)(unlimited)
Vocabulary asset_status has field field_status_icon (imagefield)
Vocabulary conversation_category has field field_tester (entityreference)
Comment #43
mikeryanCoding standard changes are out of scope here (and best addressed with core-wide scripting, as in #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase).
I'm having trouble following this class, or understanding why we need a custom migration plugin class here. What's special about taxonomy terms as fieldable entities, compared to nodes, that requires this class? Is this trying to accomplish what the deriver classes do for nodes? I see a followup for a deriver, but why not use a deriver in the first place instead of a custom migration class?
At the very least, some comments would be helpful.
I don't see what profiles have to do with taxonomy terms? D7 profile module didn't have any kind of taxonomy reference field as far as I can tell.
If these are going to be entirely separate plugins (each inheriting from DrupalSqlBase), there's no reason to not hard-code the table names instead of having member variables for them. Of course, you could have a common base class sharing them.
Ah, right, here's why they can't have the common base class. They could share a trait though, but if not then again, just hard-code the table names directly.
Comment #44
jofitz CreditAttribution: jofitz at ComputerMinds commented1. Removed change.
2. & 3. Replaced migration plugin with deriver.
4. & 5. Hard-coded table names.
Comment #45
jofitz CreditAttribution: jofitz at ComputerMinds commentedSorry, I actually managed to miss @mikeryan's point 1 in the previous commit.
Comment #46
chriscalip CreditAttribution: chriscalip commentedShallow test. last patch unusable.
Comment #47
chriscalip CreditAttribution: chriscalip commentedLooks like last patch has input options now are vocabulary.machine_name based from vocabulary.vid. Updating my test instance to test again last patch input requirements.
Still no joy.
Comment #48
chriscalip CreditAttribution: chriscalip commentedComment #49
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrect erroneous assumption (introduced in #44): Handle Term migration configuration Vocabulary as an array.
@chriscalip, would you mind retesting, please.
Comment #51
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-roll.
Comment #52
mikeryanComment #53
mikeryanThat target just keeps moving...
copypasta
Comment #54
jofitz CreditAttribution: jofitz at ComputerMinds commentedMajor re-roll in progress!
Comment #55
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-roll.
Comment #56
mikeryanLooks good, and I've successfully tested locally from a D7 environment where I added a couple of fields to the tags vocabulary. Good to go, thanks!
Comment #57
catchShould this be kept for bc until 8.4.x?
Comment #58
mikeryanYes, let's keep this so we don't break BC. Unlike other typical cases where the BC layer can wrap the new stuff, since the one plugin is splitting in two, I think we need to keep it as-is.
Per https://www.drupal.org/node/2785201#comment-11754887, could we change the new source option key from 'vocabulary' to 'bundle'? It'll be beneficial in the long run, I think, to have a standard option name here for any entity type which may be filtered by bundle.
Thanks.
Comment #59
jofitz CreditAttribution: jofitz at ComputerMinds commentedChanged 'vocabulary' to 'bundle', as recommended.
Comment #60
mikeryanThanks.
Let's restore (and deprecate) the old version-agnostic plugin.
Comment #61
jofitz CreditAttribution: jofitz at ComputerMinds commentedRestored (and deprecated) the old version-agnostic plugin.
Comment #62
mikeryanOK, think this is good to go!
Comment #64
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-roll.
(let's get this committed super-quickly so i don't have to re-roll yet again :p )
Comment #66
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #67
jofitz CreditAttribution: jofitz at ComputerMinds commentedI think this can safely be returned to RTBC - it was only a minor re-roll.
Comment #70
catchFixed a redundant use statement on commit.
Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!
Comment #73
xjmThis caused HEAD failures on 8.2.x that I was able to reproduce locally. alexpott was not able to reproduce the fail on 8.3.x. I've reverted both commits; we may need to do something different to backport or fix something else in the patch.
Comment #74
xjmOh and here is the failing 8.2.x result: https://www.drupal.org/pift-ci-job/536475
Comment #75
jofitz CreditAttribution: jofitz at ComputerMinds commented8.2.x fails because it cannot handle scalers with an IN condition (while 8.3.x has additional code to handle this).
What is the best way to resolve this? A separate patch for 8.2.x? Or include a line that is redundant in 8.3.x so that it works for both?
Comment #76
jofitz CreditAttribution: jofitz at ComputerMinds commentedHere is the slightly adjusted version that should work for 8.2.x, but has a redundant line for 8.3.x.
I'll let someone else decide how they want to handle these!
Comment #77
mikeryanAh, I was working on debugging this today, you got there first:P.
Slightly simpler would be casting with (array).
Comment #78
jofitz CreditAttribution: jofitz at ComputerMinds commentedI couldn't resist @mikeryan's simpler, more elegant solution.
Comment #79
mikeryanAdding interdiff against the originally-committed patch.
I think we're good to get this back in (pending results of the full battery of tests), thanks Jo!
Comment #80
catchI've sent a couple of environments for re-test.
Comment #83
catchCommitted/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!
Comment #85
jhoodI created another issue at https://www.drupal.org/node/2873742 to add the @see link to the change record for this issue, but was unable to locate one.
Comment #86
mikeryanThe change record needs to be added on this issue.
Comment #87
heddnAdded a change record.
Comment #88
mikeryanCR published.