Create field collections field plugin

Let's submit this for tests. And a review is up next.

1. +++ b/paragraphs.module
   @@ -436,3 +436,11 @@ function paragraphs_library_info_alter(&$libraries, $extension) {
   +  $migrations['d7_field_instance']['migration_dependancies']['optional'][] = 'd7_field_collection_type';
   +  $migrations['d7_field_instance']['migration_dependancies']['optional'][] = 'd7_paragraph_type';
   

   This makes sense. It would make even more sense after #2904765: Alter derived migration definition early in life lands. So let's add an TODO to revisit after that is committed.

2. +++ b/src/Plugin/migrate/field/Paragraphs.php
   @@ -0,0 +1,104 @@
   +          'prefix' => 'field_',
   ...
   +        'fc_prefix' => 'field_',
   
   +++ b/src/Plugin/migrate/process/ParagraphsFieldInstanceSettings.php
   @@ -0,0 +1,186 @@
   +    $prefix = $this->configuration['fc_prefix'];
   

   We called this 'fc_prefix' and 'prefix' Let's make it consistent. It is already 'prefix' in #2911241: Migrate field collections types as paragraph types, so unless we want to rename it everywhere and add in a BC shim, let's stick with 'prefix'.

3. +++ b/src/Plugin/migrate/process/ParagraphsFieldInstanceSettings.php
   @@ -0,0 +1,186 @@
   +class ParagraphsFieldInstanceSettings extends ProcessPluginBase implements ContainerFactoryPluginInterface {
   

   Can we create a plugin dedicated for paragraphs and another for field collections? Breaking these out makes more sense to me.

4. +++ b/src/Plugin/migrate/process/ParagraphsFieldInstanceSettings.php
   @@ -0,0 +1,186 @@
   +        // included..
   

   Nit: double periods.

5. +++ b/src/Plugin/migrate/process/ProcessOnValue.php
   @@ -0,0 +1,82 @@
   + *   id = "process_on_value"
   

   The namespace for process plugins is flat. Perhaps we should prefix the name here with the name of the module so as to avoid any possible conflicts? And let's keep this as super simple as needed. So remove any of the not_equals logic, etc.

6. +++ b/src/Plugin/migrate/process/StripPrefix.php
   @@ -0,0 +1,34 @@
   +      id = "strip_prefix"
   

   same applies here. prefix with paragraph_.

And let's make this even easier on ourselves and break the pargraph field stuff into another issue. So this is all about field collections and the other about paragraphs.

1. +++ b/src/Plugin/migrate/process/ParagraphsFieldInstanceSettings.php
   @@ -0,0 +1,186 @@
   +        // None selected therefore allow all in d7, but maybe we have
   +        // additional types from a field collection migration that shouldn't be
   +        // included..
   

   We can only assume so much. And only migrate what is in the source. If the source says allow all bundles, let's copy over that configuration. Folks can always change this around, post migration.

2. +++ b/src/Plugin/migrate/process/ParagraphsFieldInstanceSettings.php
   @@ -0,0 +1,186 @@
   +          // Yes, new bundles.  Don't allow the new ones to be selected
   +          // in this instance.
   

   Let's keep it simple. It might make sense to put some comments that we are keeping things simple. And give some suggestions on how that could easily be changed by someone if they didn't like the default. If there isn't an easy way to change the default, folks can still make config changes post migration. They'll already have to do that for views, etc. So this is handled with good docs. Not some technical mind reading exercise.

+++ b/src/Plugin/migrate/field/Paragraphs.php
@@ -0,0 +1,104 @@
+    // I don't believe we can register a plugin in the process system based on
+    // the entity type of the field instance, which would be a more appropiate
+    // place to put the process below.  By putting it here, there could be an
+    // edge case issue where there is a paragraph bundle on a d7 installation
+    // but no field referencing it, and therefore this plugin wouldn't register
+    // and the field storage for the fields on the bundle would not map to the
+    // paragraph entity properly.  I don't know what the point of that would
+    // be, but there may be a better place to put this.

I think, if I understand the problem correctly that we need to pend this part of the changes until we write the migration deriver for the paragraph or field collection. See D6NodeDeriver as an example. It calls plugin.manager.migrate.field and builds all the fields for the specific migration.

We do however, need to alter the FieldInstance and Field source plugins for field collections to now point to paragraphs. See d7_commerce_field in commerce_migrate as an example. And commerce_migrate_commerce_migration_plugins_alter for how we register the replacement source field plugins. That example would get 10x easier once #2904765: Alter derived migration definition early in life. If we can bump that along to RTBC... that would make things easier here. There's a reason it is marked contrib project blocker.

The review patch in #13 didn't seem to have any tests? But things are looking good here.

1. +++ b/paragraphs.module
   @@ -436,3 +441,101 @@ function paragraphs_library_info_alter(&$libraries, $extension) {
   +  /* @todo refactor/rethink this when https://www.drupal.org/project/drupal/issues/2904765 is resolved */
   

   Nit: Comment style should be two slashes and less than 80 characters per line.

2. +++ b/paragraphs.module
   @@ -436,3 +441,101 @@ function paragraphs_library_info_alter(&$libraries, $extension) {
   +  $bundle_process = $migration['process']['bundle'];
   ...
   +  $entity_type_process = $migration['process'][$destination];
   

   Do we need to do an isset/empty check here?

3. +++ b/src/Plugin/migrate/process/ParagraphsProcessOnValue.php
   @@ -0,0 +1,65 @@
   +      throw new MigrateException("source_value not set for ProcessOnValue plugin");
   ...
   +      throw new MigrateException("expected_value not set for ProcessOnValue plugin");
   ...
   +      throw new MigrateException("process not set or invalid for ProcessOnValue plugin");
   

   Let's throw an \InvalidArgumentException instead. The migration is fine, just the arguments passed to it are a little broken.

4. +++ b/src/Plugin/migrate/process/ParagraphsProcessOnValue.php
   @@ -0,0 +1,65 @@
   +      throw new MigrateSkipRowException('source_value is not valid for ProcessOnValue plugin');
   

   Instead of calling this ProcessOnValue, we typically use the plugin id. "paragraphs_process_on_value"

5. +++ b/src/Plugin/migrate/process/ParagraphsStripPrefix.php
   @@ -0,0 +1,35 @@
   + *   id = "paragraphs_strip_prefix"
   

   I think I suggested namespacing this with the module. However, what if we called it by the name of the previous module? field_collection_strip_prefix? That would make more semantic sense to me.

Wow, a real pleasure to review. Really only some very small nits. This is really close.

1. +++ b/paragraphs.module
   @@ -440,3 +446,107 @@ function paragraphs_library_info_alter(&$libraries, $extension) {
   +      if ((is_a($migration['class'], FieldMigration::class, TRUE))) {
   

   Nit: extra parenthesis isn't needed.

2. +++ b/paragraphs.module
   @@ -440,3 +446,107 @@ function paragraphs_library_info_alter(&$libraries, $extension) {
   +      'start' => 6,
   

   Should we use the class constant so as to explain more clearly what this magic number means? FieldCollectionFieldInstanceSettings::FIELD_COLLECTION_PREFIX_LENGTH

3. +++ b/src/Plugin/migrate/field/FieldCollection.php
   @@ -0,0 +1,81 @@
   +        'prefix' => 'field_',
   

   I don't think this is needed any more.

4. +++ b/src/Plugin/migrate/process/ParagraphsFieldInstanceSettings.php
   @@ -0,0 +1,86 @@
   +          // No bundles are selected and therefore this field was set to allow
   +          // all bundles in d7, but maybe we now have additional bundles
   +          // from a field collection migration or pre-existing that shouldn't be
   +          // included.
   +          if (count($value['allowed_bundles']) == count($bundles)) {
   

   I think I suggested we make this simply map exactly what is in the source. If the source says all, then all it should be. There's a reason for incremental migrations. For folks to modify the resulting site to meet their needs. I don't like guessing. We end up adding complexity and are usually wrong. Unless you feel super strong, let's remove all this logic.

5. +++ b/src/Plugin/migrate/process/ParagraphsProcessOnValue.php
   @@ -0,0 +1,83 @@
   + * Runs a processor on a value if a source value is equal to an expected value.
   

   Can we say "Runs a migration process on a value..." since we then use the name/term it is called in core. Otherwise it leaves me guessing if we are referring to the same thing. Which I think we are referring to a process plugin.

6. +++ b/src/Plugin/migrate/process/ParagraphsProcessOnValue.php
   @@ -0,0 +1,83 @@
   + * source_value (required) string. The source property to check against.
   + * expected_value (required) string. The value to check against.  If the
   ...
   + * process (required) array.  The process array to execute if the source proerty
   

   can we put a colon: after the key to distinguish it more clearly?

7. +++ b/src/Plugin/migrate/process/ProcessPluginBase.php
   @@ -0,0 +1,72 @@
   +    return NestedArray::mergeDeep($this->defaultConfiguration(), $configuration);
   

   I think we want the other way around? The configuration set to the plugin should override the default config.

8. +++ b/tests/src/Kernel/ParagraphsMigrationTest.php
   @@ -44,12 +66,217 @@ class ParagraphsMigrationTest extends MigrateDrupalTestBase {
   +  protected function executeMigrationDependencies(MigrationInterface $migration) {
   

   This would be nice to add the core MigrateDrupalTestBase. Please open an issue :)

   Actually, this is an problem today in a lot of places in core/contrib and having this in core would be a wonderful addition.

9. +++ b/tests/src/Unit/migrate/FieldCollectionsFieldInstanceSettingsTest.php
   @@ -0,0 +1,112 @@
   +  public function getData() {
   +    $data = [
   +      [
   

   Can we give this test data an array key title/description?

10. +++ b/tests/src/Unit/migrate/ParagraphsFieldInstanceSettingsTest.php
    @@ -0,0 +1,103 @@
    +  public function getData() {
    +    $data = [
    +      [
    

    Can we give this scenario an array key title/description?

For #28.4, if we want to create a follow-up and add a TODO, that might let us continue the conversation?

I want to do some more manual testing, but the actual code looks pretty solid at this point. Leaving in review status since there's nothing to fix, just needs more testing.

I think we need to make the test in the plugin alter a little less strict. See #2944597: Make the tests for plugins in plugins alter stricter. That might help with some the errors from my manual testing. Also some other findings after applying the patch and running things manually.

1. +++ b/paragraphs.module
   @@ -8,7 +8,12 @@
   +use Drupal\field\Plugin\migrate\source\d7\FieldInstance;
   
   +++ b/src/Plugin/migrate/process/ParagraphsProcessOnValue.php
   @@ -0,0 +1,84 @@
   +    if ($source_value == $this->configuration['expected_value']) {
   

   This should be === me thinks to account for 0 and ''.

2. +++ b/src/Plugin/migrate/process/ParagraphsProcessOnValue.php
   @@ -0,0 +1,84 @@
   +    if (is_null($source_value)) {
   

   Do we want to check for null, empty and zero?

3. +++ b/src/Plugin/migrate/process/ParagraphsProcessOnValue.php
   @@ -0,0 +1,84 @@
   +      if (array_key_exists('plugin', $process)) {
   +        if (empty($process['source'])) {
   +          $process['source'] = $destination_property;
   +        }
   +      }
   +      else {
   +        if (empty($process[0]['source'])) {
   +          $process[0]['source'] = $destination_property;
   +        }
   

   This logic here could serve some comments. I'm not clear from looking at it what we are achieving.

For my manual testing:

1. I applied the patch to paragraphs.
2. Installed the d7 fixture provided by paragraphs php ./db-tools.php import --database d7_settings.php_db_key ../../modules/paragraphs/tests/fixtures/drupal7.php.
3. Installed D8 with minimal profile drush si -y minimal.
4. Enabled the following modules: drush en -y node paragraphs file image link text telephone block_content comment filter migrate_drupal options path toolbar update node paragraphs file image link text telephone migrate_upgrade migrate_tools taxonomy datetime datetime_range
5. Ran the migrate_upgrade command to generate all migration configurations drush migrate-upgrade --legacy-db-key=d7_settings.php_db_key --legacy-root=http://drupal.localhost --configure-only
6. Exported the config so I could review it. drush cex -y
7. Ran the whole migration drush mim -y --all

The resulting config for field:

uuid: 7dfb51a9-89da-413c-9fe3-9e0f9ccf2b30
langcode: en
status: true
dependencies: {  }
id: upgrade_d7_field
class: Drupal\migrate_drupal\Plugin\migrate\FieldMigration
field_plugin_method: processField
cck_plugin_method: null
migration_tags:
  - 'Drupal 7'
  - Configuration
migration_group: migrate_drupal_7
label: 'Field configuration'
source:
  plugin: d7_field
  constants:
    status: true
    langcode: und
process:
  entity_type:
    -
      plugin: get
      source: entity_type
    -
      plugin: static_map
      map:
        field_collection_item: paragraph
        paragraphs_item: paragraph
      bypass: true
  status: constants/status
  langcode: constants/langcode
  field_name: field_name
  type:
    plugin: process_field
    source: type
    method: getFieldType
  cardinality: cardinality
  settings:
    plugin: d7_field_settings
destination:
  plugin: 'entity:field_storage_config'
migration_dependencies:
  required: {  }
  optional: {  }

And for field instance:

uuid: cc3a0eeb-3b55-4201-b00e-54b665ae7d7b
langcode: en
status: true
dependencies: {  }
id: upgrade_d7_field_instance
class: Drupal\migrate_drupal\Plugin\migrate\FieldMigration
field_plugin_method: processFieldInstance
cck_plugin_method: null
migration_tags:
  - 'Drupal 7'
  - Configuration
migration_group: migrate_drupal_7
label: 'Field instance configuration'
source:
  plugin: d7_field_instance
  constants:
    status: true
process:
  type:
    plugin: process_field
    source: type
    method: getFieldType
  entity_type:
    -
      plugin: get
      source: entity_type
    -
      plugin: static_map
      map:
        field_collection_item: paragraph
        paragraphs_item: paragraph
      bypass: true
  field_name: field_name
  bundle:
    -
      plugin: static_map
      source: bundle
      bypass: true
      map:
        comment_node_forum: comment_forum
    -
      plugin: paragraphs_process_on_value
      source_value: entity_type
      expected_value: field_collection_item
      process:
        plugin: substr
        start: 6
  label: label
  description: description
  required: required
  status: constants/status
  settings:
    plugin: d7_field_instance_settings
    source:
      - settings
      - widget
      - field_definition
  default_value_function: ''
  default_value:
    plugin: d7_field_instance_defaults
    source:
      - default_value
      - widget
  translatable: translatable
destination:
  plugin: 'entity:field_config'
migration_dependencies:
  required:
    - upgrade_d7_field
  optional:
    - upgrade_d7_node_type
    - upgrade_d7_comment_type
    - upgrade_d7_taxonomy_vocabulary
    - upgrade_d7_field_collection_type
    - upgrade_d7_paragraphs_type

And the errors:

$ drush mim --all
Processed 50 items (46 created, 0 updated, 0 failed, 4 ignored) - done with 'upgrade_d7_field'                       [status]
Processed 7 items (7 created, 0 updated, 0 failed, 0 ignored) - done with 'upgrade_d7_node_type'                     [status]
Processed 7 items (7 created, 0 updated, 0 failed, 0 ignored) - done with 'upgrade_d7_comment_type'                  [status]
Processed 4 items (4 created, 0 updated, 0 failed, 0 ignored) - done with 'upgrade_d7_taxonomy_vocabulary'           [status]
Processed 3 items (3 created, 0 updated, 0 failed, 0 ignored) - done with 'upgrade_d7_field_collection_type'         [status]
Processed 2 items (2 created, 0 updated, 0 failed, 0 ignored) - done with 'upgrade_d7_paragraphs_type'               [status]
Missing bundle entity, entity type paragraphs_type, entity id collection_test.                                       [error]
(/home/lucas/websites/drupal/core/lib/Drupal/Core/Entity/EntityType.php:910)
Missing bundle entity, entity type paragraphs_type, entity id collection_test.                                       [error]
(/home/lucas/websites/drupal/core/lib/Drupal/Core/Entity/EntityType.php:910)
The field_any_paragraph entity reference field (entity_type: node, bundle: paragraphs_test) no longer has any valid  [error]
bundle it can reference. The field is not working correctly anymore and has to be adjusted.
Missing bundle entity, entity type paragraphs_type, entity id _fc_outer.                                             [error]
(/home/lucas/websites/drupal/core/lib/Drupal/Core/Entity/EntityType.php:910)
Missing bundle entity, entity type paragraphs_type, entity id _fc_inner.                                             [error]
(/home/lucas/websites/drupal/core/lib/Drupal/Core/Entity/EntityType.php:910)
Processed 74 items (62 created, 0 updated, 4 failed, 8 ignored) - done with 'upgrade_d7_field_instance'              [status]
upgrade_d7_field_instance Migration - 4 failed.                                                                      [error]
Processed 7 items (7 created, 0 updated, 0 failed, 0 ignored) - done with                                            [status]
'upgrade_d7_comment_entity_form_display_subject'
Processed 9 items (9 created, 0 updated, 0 failed, 0 ignored) - done with 'upgrade_d7_view_modes'                    [status]
Missing bundle entity, entity type paragraphs_type, entity id collection_test.                                       [error]
(/home/lucas/websites/drupal/core/lib/Drupal/Core/Entity/EntityType.php:910)
Missing bundle entity, entity type paragraphs_type, entity id collection_test.                                       [error]
(/home/lucas/websites/drupal/core/lib/Drupal/Core/Entity/EntityType.php:910)
Missing bundle entity, entity type paragraphs_type, entity id _fc_outer.                                             [error]
(/home/lucas/websites/drupal/core/lib/Drupal/Core/Entity/EntityType.php:910)
Missing bundle entity, entity type paragraphs_type, entity id _fc_inner.                                             [error]
(/home/lucas/websites/drupal/core/lib/Drupal/Core/Entity/EntityType.php:910)
Processed 86 items (69 created, 0 updated, 4 failed, 13 ignored) - done with 'upgrade_d7_field_formatter_settings'   [status]
upgrade_d7_field_formatter_settings Migration - 4 failed.                                                            [error]
Missing bundle entity, entity type paragraphs_type, entity id collection_test.                                       [error]
(/home/lucas/websites/drupal/core/lib/Drupal/Core/Entity/EntityType.php:910)
Missing bundle entity, entity type paragraphs_type, entity id collection_test.                                       [error]
(/home/lucas/websites/drupal/core/lib/Drupal/Core/Entity/EntityType.php:910)
Missing bundle entity, entity type paragraphs_type, entity id _fc_outer.                                             [error]
(/home/lucas/websites/drupal/core/lib/Drupal/Core/Entity/EntityType.php:910)
Missing bundle entity, entity type paragraphs_type, entity id _fc_inner.                                             [error]
(/home/lucas/websites/drupal/core/lib/Drupal/Core/Entity/EntityType.php:910)
Processed 74 items (62 created, 0 updated, 4 failed, 8 ignored) - done with                                          [status]
'upgrade_d7_field_instance_widget_settings'

+++ b/paragraphs.module
@@ -454,35 +454,40 @@ function paragraphs_library_info_alter(&$libraries, $extension) {
+    if (empty($migration['migration_tags']) || !in_array('paragraphs-processed', $migration['migration_tags'])) {
...
+      $migration['migration_tags'][] = 'paragraphs-processed';

I'm not a fan of this. Tagging with embedded meaning like this is fragile. Can we do this another way?

This has gone through a ton of testing. I think we are ready for prime time now. Onward and upward.