Migrating reference fields: target_bundles may never be empty array

This happens for two content types on training.acquia.com, both of which do not specify referenceable_types and hence get the [] fallback, which is wrong as explained in the issue summary.

Maybe it should be an array of all the referenceable_types available on the source site.

Let's say on the source site the reference field allowed bundle "A" to be referenced.
However, on the destination, bundle "A" is not available at the moment.

All of this, plus the fact that bundles may be renamed on the destination, leads me to one obvious suggestion: allow all bundles on the destination site. That's the intent of the setting on the source, and it's the equivalent correct configuration on the destination.

IOW: I proposed NULL because it's the simplest possible solution, and the migration process plugin already does that for entityreference source field type migrations. But you're right that it could (should) be even better. That just requires more work.

FYI @huzooka has had to tackle similar problems in https://www.drupal.org/project/media_migration — I think the relevant code is at \Drupal\media_migration\Plugin\migrate\process\MediaImageFieldInstanceSettings::transform(), but I'll let him confirm and elaborate 🤓

Amazing progress here! 🥳

#13: good point. In fact, it will need a required dependency on every migration that targets a config entity type that represents the bundles for a particular destination entity type. Not just node_type. So also d7_taxonomy_vocabulary in core, but more still in contrib.

Ideally, we can automatically compute this. Otherwise many contrib migrations (for contrib entity types) are likely to get this wrong.

+++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldInstanceSettings.php
@@ -68,12 +120,25 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+      $target_bundles = empty($target_bundles) ? NULL : $target_bundles;

NULL should be replaced with an array listing all node type IDs, otherwise it's still invalid.

To follow up on #15: I wanted to be 100% certain about this.

So I went ahead and created a new entity_reference fields with node as the target entity type. Upon doing so, I end, the following field.field.node.course.field_test.yml was generated:

uuid: da867862-23c0-4ad6-8abf-ce11ef3b9bb4
langcode: en
status: true
dependencies:
  config:
    - field.storage.node.field_test
    - node.type.course
id: node.course.field_test
field_name: field_test
entity_type: node
bundle: course
label: test
description: ''
required: false
translatable: false
default_value: {  }
default_value_callback: ''
settings:
  handler: 'default:node'
  handler_settings: {  }
field_type: entity_reference

… which does match the current migration system behavior.

However, if you try to save this at /admin/structure/types/manage/course/fields/node.course.field_test, then you'll get this:

The form validation logic does the right thing: it matches the field_field_config_presave() logic (which was introduced in #2925913: EntityReferenceItem logs critical errors in onDependencyRemoval()).

The problem is that first field.storage.node.field_test.yml gets generated, and a placeholder field.field.node.course.field_test.yml gets generated. You're supposed to click Save settings on the above screenshot. But nothing is forcing you to. But that is a bug/weakness in Field UI (for having such a brittle UX) and in Field API (for not warning about this, anywhere, not even on the Status Report, where it happily reports Entity/field definitions: Up to date 😔).

Overall: a pretty messy situation. But at least for the migration system, the solution is pretty simple (will work on that now). For Field UI/Field API the solution is less clear. And definitely out of scope.

This fixes #15 + #16.

Interdiff for #17.

Similar (but different) problem for user_reference fields:

The problems here are that the roles that are being set are labels instead of the config ID — we need to use the processed role ID (i.e. it must undergo the same processing as d7_user_role.yml specifies).

Furthermore, in D7 it's possible to configure it to only allow the authenticated user. But … that is expressly forbidden in D8: you have to either pick a specific user role. Besides, the equivalent of "only authenticated" is "not anonymous". And for that, D8|9 gained an explicit setting: include_anonymous (since #2434697: Remove UserAutocompleteController).

So, incorporating all newfound processing knowledge that in this interdiff.

Before

uuid: fd86347a-949d-40f9-a0ce-f16d35f612da
langcode: en
status: true
dependencies:
  config:
    - field.storage.node.field_candidate
    - node.type.payment_exam_info
id: node.payment_exam_info.field_candidate
field_name: field_candidate
entity_type: node
bundle: payment_exam_info
label: Candidate
description: ''
required: false
translatable: false
default_value: {  }
default_value_callback: ''
settings:
  handler: 'default:user'
  handler_settings:
    include_anonymous: true
    filter:
      type: role
      role:
        'authenticated user': 'authenticated user'
    sort:
      field: _none
      direction: ASC
    auto_create: false
field_type: entity_reference

After

uuid: a7a1d425-09d0-47ff-9c66-0d89b2eacf28
langcode: en
status: true
dependencies:
  config:
    - field.storage.node.field_candidate
    - node.type.payment_exam_info
id: node.payment_exam_info.field_candidate
field_name: field_candidate
entity_type: node
bundle: payment_exam_info
label: Candidate
description: ''
required: false
translatable: false
default_value: {  }
default_value_callback: ''
settings:
  handler: 'default:user'
  handler_settings:
    include_anonymous: false
    filter:
      type: _none
    sort:
      field: _none
      direction: ASC
    auto_create: false
field_type: entity_reference

1. +++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldInstanceSettings.php
   @@ -68,12 +122,28 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +        // @see field_field_config_presave()
   +        $target_bundles = array_keys(NodeType::loadMultiple());
   

   To get a [article ⇒ article, blog ⇒ blog]-style result here, we should use array_combine().

2. +++ b/core/modules/field/tests/src/Unit/Plugin/migrate/process/d7/FieldInstanceSettingsTest.php
   @@ -18,23 +19,103 @@ class FieldInstanceSettingsTest extends MigrateTestCase {
   -  public function testTransformImageSettings() {
   +  public function testTransformImageSettings($type, $instance_settings, $widget_settings, $field_definition, $lookup_result, $expected_results) {
   

   Running these tests fails locally for me 😬

   1) Drupal\Tests\field\Unit\Plugin\migrate\process\d7\FieldInstanceSettingsTest::testTransformImageSettings with data set "node_reference no content types" ('node_reference', array(), array('node_reference'), array('a:1:{s:8:"settings";a:1:{s:19...:0;}}}'), array(), array('default:node', array(array('_none', 'ASC'), null)))
   Drupal\Core\DependencyInjection\ContainerNotInitializedException: \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container.
   

   Between this and #3197324: Exception trace cannot be serialized because of closure, running core migration tests is … mildly maddening 😅 Pointers welcome!

3. The additional failures in #19 indicate that we have working test coverage for user_reference, but the fact that include_anonymous remains TRUE suggests that we really need to fix #13 too.

Thanks, @srishti.bankar, for addressing those first 2 failures in #19, and oddly enough now there's a different set of failures! 😳

Will re-investigate

You omitted the changes to MigrateDrupal7TestBase.php and the fixture, @narendraR. Why?

#3319582: Fix calls to methods with too many parameters passed in conflicted with this, but also allows slightly simplifying this 👍