D7 taxonomy term fields are not migrated with allowed vocabularies

Tagging for d7.

+++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldInstanceSettings.php
@@ -17,9 +17,19 @@ class FieldInstanceSettings extends ProcessPluginBase {
+      $vocabulary = $field_settings['allowed_values'][0]['vocabulary'];

Doesn't the D7 taxonomy_term_reference field permit multiple target vocabularies? We should migrate the full list.

I'll be reviewing this.

Looks good, but we should have a test (please include a test-only patch to demonstrate the resulting misconfiguration without the fix).

Added tests (including test only).

Thanks!

@mikeryan is right in #8, while the 7.x UI doesn't allow multiple vocabulary references, the API does.

Handle multiple vocabulary references.

I think what gives me pause here is the possibility that vocabularies' machine names may change during the migration. Honestly, this should probably cause the field settings migration to depend on the d7_taxonomy_vocabulary migration, then look up the vocabulary names there. But that seems like a big rewrite (and far more complex) so...I dunno.

I think what gives me pause here is the possibility that vocabularies' machine names may change during the migration.

Just to be clear, the concern would be if a vocabulary machine name needed to be deduped, right? d7_taxonomy_vocabulary doesn't do deduping (since D7 had proper machine names for vocabularies), so not a concern for the straight upgrade path, but maybe for custom scenarios - if you're renaming vocabularies, then the straight copy of the machine name here breaks you (or forces you to do a custom override of the process plugin).

If we wanted to handle that, we would

cause the field settings migration to depend on the d7_taxonomy_vocabulary migration, then look up the vocabulary names there:

+++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldInstanceSettings.php
@@ -17,9 +17,20 @@ class FieldInstanceSettings extends ProcessPluginBase {
+          $instance_settings['handler_settings']['target_bundles'][$vocabulary] = $vocabulary;

I.e., call the migration process plugin to translate the incoming $vocabulary to any renamed machine name.

I think we may discuss this on the weekly migration call, leaving "Needs review" for now.

In the interest of getting this working as soon as possible for the 90% use case, I'd be OK with putting a TODO comment there explaining the situation, and what needs to be done to make it work for custom migration scenarios.

Tested path #17 and #13, both also not working for me.

Rollback and re-migrated the following:
upgrade_d7_field
upgrade_d7_field_instance
upgrade_d7_field_instance_widget_settings
upgrade_d7_field_formatter_settings

I got sick of seeing this "Needs work" in my issue list so I've had a go at the @todo suggested by @phenaproxima.

@ckng Can you tell us more about the problems you have had, please? It works fine for me.

Re-roll.

Self-assigning for re-review.

1. +++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldInstanceSettings.php
   @@ -20,6 +20,22 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +    if ($row->getSourceProperty('type') == 'taxonomy_term_reference') {
   +      foreach ($field_settings['allowed_values'] as $allowed_value) {
   +        if ($vocabulary = $allowed_value['vocabulary']) {
   +          $instance_settings['handler_settings']['sort'] = [
   +            'field' => '_none',
   +          ];
   +          // @todo Handle duplicate vocabulary machine names ($vocabulary). This
   +          // current code works for migrations from Drupal 7 (because that uses
   +          // proper machine names for vocabularies), but may have problems with
   +          // custom migration scenarios if they include vocabulary names that
   +          // require de-duping.
   +          $instance_settings['handler_settings']['target_bundles'][$vocabulary] = $vocabulary;
   +        }
   +      }
   +    }
   

   Can this be delegated to a cckfield plugin?

   Also, as noted in the comment, this is not accounting for the possibility of the migrated vocabulary having a different machine name due to uniqification or whatever. The plugin should use the migration process plugin to do a lookup. That will get real messy if this is kept in this one FieldInstanceSettings plugin, so that's another argument for moving this to its own cckfield plugin.

   EDIT: I know this contradicts what I said in #20, but that was three months ago. Circumstances have changed, and I think it's worth doing this right the first time.

2. +++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceTest.php
   @@ -110,6 +110,22 @@ protected function assertLinkFields($id, $title_setting) {
   +   * @param array $target_bundles
   

   Should be string[], not array.

3. +++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceTest.php
   @@ -110,6 +110,22 @@ protected function assertLinkFields($id, $title_setting) {
   +  protected function assertEntityReferenceFields($id, $target_bundles) {
   

   $target_bundles should be type hinted.

4. +++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceTest.php
   @@ -110,6 +110,22 @@ protected function assertLinkFields($id, $title_setting) {
   +    $this->assertSame($expected_target_bundles, $handler_settings['target_bundles']);
   

   This will fail if $expected_target_bundles and $target_bundles have the same values, but not in the same order. We should just call assertContains() or assertArrayHasKey() on every element of $target_bundles.

Is there a chance we see it in D8.4?

Patch in #24 no longer applies to doing a quick re-roll before addressing @phenaproxima's comments in #27.

Addressing review in #27:

1. @phenaproxima can you explain a bit more about delegating this to a field plugin, please? I have spent too long fiddling with Drupal\taxonomy\Plugin\migrate\field\TaxonomyTermReference without success (am I even looking at the right plugin!?!)
2. array replaced with string[].
3. Type-hinted $target_bundles with array.
4. Call assertContains() on every element of $target_bundles. Also re-ordered the array in one of the tests so that it would fail with the previous code (but not this).

Field and field instances now have access to the full set of configuration, so I think that changes things around a little bit now. And we can merge in a process plugin that extends from migration_lookup (or maybe is the lookup itself).

I've just taken another look at this and I'm none the wiser. I think I want to add Drupal\taxonomy\Plugin\migrate\field\TaxonomyTermReference::processFieldInstance(), but I don't know how to replicate the functionality of FieldInstanceSettings::transform() in there. In particular how to obtain the allowed_values from unserialize($value[2]['data'])['settings']['allowed_values'](!)

I've probably got the wrong end of the stick, but I could do with some guidance, please.

FWIW, I hit the identical bug with a D6 to D8 migration trying to use core's migrate_drupal, migrate_plus, migrate_tools + drush. The term reference fields in the D8 site were created, but the target_bundles configuration was broken. Here's an example of what it created:

langcode: en
status: true
dependencies:
  config:
    - field.storage.node.field_topic
    - node.type.insert_box
id: node.insert_box.field_topic
field_name: field_topic
entity_type: node
bundle: insert_box
label: Topic
description: ''
required: true
translatable: true
default_value: {  }
default_value_callback: ''
settings:
  handler: 'default:taxonomy_term'
  handler_settings:
    target_bundles:
    - field_topic
    auto_create: true
field_type: entity_reference

Here's what it needs to be to work properly (which is what I exported to config after manually selecting the appropriate checkbox in the "Allowed Vocabularies" setting on the field after migration):

langcode: en
status: true
dependencies:
  config:
    - field.storage.node.field_topic
    - node.type.insert_box
    - taxonomy.vocabulary.topic
id: node.insert_box.field_topic
field_name: field_topic
entity_type: node
bundle: insert_box
label: Topic
description: ''
required: true
translatable: true
default_value: {  }
default_value_callback: ''
settings:
  handler: 'default:taxonomy_term'
  handler_settings:
    target_bundles:
      topic: topic
    sort:
      field: name
      direction: asc
    auto_create: true
    auto_create_bundle: ''
field_type: entity_reference

Looks like the same basic approach from patch #31 would work if it was also applied to core/modules/field/src/Plugin/migrate/process/d6/FieldInstanceSettings.php

I'm so out of the loop on core development these days, I have no idea if I'm hijacking the issue and should create a separate issue for the D6 case. Please advise if so. But it seems reasonable to fix the same bug in both migrations in the same issue. Apologies if that's not the preferred approach.

Thanks,
-Derek

Tentatively tagging this issue for the D6 case, too...

D6 and D7 can probably be done at the same time.

Patch #31 works for me on Drupal 8.5.
Thanks.

@phenaproxima: I'm trying to use migration_lookup to get the new vocabulary names from the mapping table like you suggested in #27.1, but there's a big problem. The allowed_values in the D7 field config use the vocabulary machine names while in the mapping table we have the vocabulary vid. Do you have a idea how we could fix that? I don't.

Aren't the vid and vocabulary machine names the same thing? In d6, its an integer for the vid, but in D7, it was a string.

In D7 we had both, a numeric vid and a string machine_name.

Hi! Patch from #31 fails to apply to Drupal 8.6.x. Does anyone on this issue thread know whether this issue is still relevant? I haven't dug into why the patch won't apply yet. Thanks.

Yes, this issue is still relevant, but the proposed solution from comment #27 does not work, we need to find a new one.

You can still apply the patch by doing git apply <patch_name> --reject. I think the only conflicts might be in the test fixtures.

Thanks for the tip maxocub!

I decided to re-rolled patch to apply properly with 8.6.x and 8.7.x anyway. Sorry this doesn't help actually fix the issue.

Let's see how this works for Drupal 7. So far I haven't figured out how to set up allowed vocabularies in Drupal 6.

Following a successful upgrade that replicated the problem, I installed the patch in #47 and started over with a fresh database, and the upgrade went fine until it hit this error:

Error: Call to undefined method Drupal\migrate\Row::get() in Drupal\field\Plugin\migrate\process\d7\FieldInstanceSettings->transform() (line 27 of /var/www/html/[site]/web/core/modules/field/src/Plugin/migrate/process/d7/FieldInstanceSettings.php)

Since this is one of the lines added by the patch, I'm going to guess it needs work. :(

Never mind, I was testing with Drupal 8.6 above! With 8.8.0-dev, the patch works correctly and my imported tag reference fields have the correct vocabularies selected. Well done!

@BenStallings, Thanks! Great to hear that the patch works. Another step closer!

I'm setting back to NR so this can get a code review as well.

The change I added to the latest patch gets the vid and puts on the row so that a migration lookup can be used in the process to get the correct, migrated, vocabulary.

Code looks fine. Looked at it for a while this morning. Great job here.

Based on the #49and #51 I'm changing this to D7 onley and made and issue for D6 #3042533: D6 taxonomy term fields are not migrated with allowed vocabularies. That way this can keep progressing.

That was supposed to be an RTBC.

@heddn, thanks for checking back in on this and setting to RTBC.

Added a little info to the IS

This is also blocking #2979970: Migrate D7 vocabulary language settings.

+++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldInstanceSettings.php
@@ -23,6 +23,18 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+        foreach ($allowed_value as $vocabulary) {
+          $instance_settings['handler_settings']['sort'] = [
+            'field' => '_none',
+          ];

Why is this assignment in the nested foreach?

No reason, really. I've got no evidence that it matters, so have reworked that code.

Let's take out the reset() that I tried in the last patch.

Feedback addressed.

This needs a reroll. Did that. Merging serialised arrays is fun.

Re-roll passed green and diff stats are identical. Seems like we could pass back to RTBC.

We need a follow-up to fix Drupal 6 as per #33 as we didn't do #37

Committed and pushed 16f38cff6e to 8.8.x and b22572e598 to 8.7.x. Thanks!

Can anyone confirm that this is broken again? Drupal 9.1. This worked fine for many months, and now does not. I spent three days on this and everything is in order, the code of this patch is present in the release now.
First problem, the FieldInstanceSettings code is not run on 'settings' unless I assign it to a funny element like 1: - when it runs as in the core module configuration (no extra transformation in the chain), it does not run the code. Strange. The rest of the field instance is processed.

When I print out the settings in the transform() method, in the data from the FieldInstanceSettings I can see the settings handler_settings target_bundles is set to the correct D9 vocabulary. However, the value is not saved into the field configuration.

This seems more like a problem with core field migrations and the way settings are handled. Seemed to come out of nowhere.

I worked with this some more and assigning a string key to my process in the yml made it work for some reason that is inscrutable. There must be something about how that settings parameter has changed recently.

I'm working through a migration from 7 to 8.9.13 and experienced the same issue regarding the target bundles not being set. I switched the zero to a 1 in my migrate_plus.migration.upgrade_d7_field_instance.yml and it seems to work. Thanks for the tip!