Problem/Motivation

Consider this code in CckMigration.php (lines 107-118):

      foreach ($source_plugin as $row) {
        $field_type = $row->getSourceProperty('type');
        if (!isset($this->processedFieldTypes[$field_type]) && $this->cckPluginManager->hasDefinition($field_type)) {
          $this->processedFieldTypes[$field_type] = TRUE;
          // Allow the cckfield plugin to alter the migration as necessary so
          // that it knows how to handle fields of this type.
          if (!isset($this->cckPluginCache[$field_type])) {
            $this->cckPluginCache[$field_type] = $this->cckPluginManager->createInstance($field_type, [], $this);
          }
          call_user_func([$this->cckPluginCache[$field_type], $this->pluginDefinition['cck_plugin_method']], $this);
        }
      }

In the line with createInstance, the field type is passed and checked correctly. However, the type hint for the property cckPluginManager is misleading. It is actually the base class which has different semantics for the createInstance method. Navigation in IDE is confusing and it takes effort to figure out the mix-up between field_type and plugin_id, which may be different.

Proposed resolution

Type hint the property correctly. The CckMigration is created with the service plugin.manager.migrate.cckfield which is/should be \Drupal\migrate_drupal\Plugin\MigrateCckFieldPluginManager or its subclasses.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#19 cckmigration_does_not-2787619-19.patch18.75 KBhussainweb
#19 interdiff-18-19.txt3.2 KBhussainweb
#18 cckmigration_does_not-2787619-18.patch17.94 KBhussainweb
#14 cckmigration_does_not-2787619-14.patch17.89 KBhussainweb
#14 interdiff-11-14.txt2.72 KBhussainweb
#11 cckmigration_does_not-2787619-11.patch16.99 KBhussainweb
#11 interdiff-8-11.txt10.46 KBhussainweb
#8 cckmigration_does_not-2787619-8.patch15.28 KBhussainweb
#8 interdiff-3-8.txt8.9 KBhussainweb
#3 cckmigration_does_not-2787619-3.patch8.33 KBhussainweb
#3 interdiff-2-3.txt5.71 KBhussainweb
#2 cckmigration_does_not-2787619-2.patch2.62 KBhussainweb

Comments

hussainweb created an issue. See original summary.

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new cckmigration_does_not-2787619-2.patch2.62 KB

Attaching a patch with the change.

hussainweb’s picture

hussainweb
Primary language English
commented
StatusFileSize
new interdiff-2-3.txt5.71 KB
new cckmigration_does_not-2787619-3.patch8.33 KB

I found some more places where this plugin manager is used and not hinted correctly.

benjy’s picture

benjy commented

Fixes make sense, just a question regarding dependencies.

  1. diff --git a/core/modules/field/src/Plugin/migrate/process/FieldType.php b/core/modules/field/src/Plugin/migrate/process/FieldType.php
...
+use Drupal\migrate_drupal\Plugin\MigrateCckFieldPluginManager;

    Can someone remind me how we know the FieldType process plugin is dependant on migrate_drupal? I know we've had an issue for this in the past but I thought that was only for the migrations themselves, not the process plugins.

hussainweb’s picture

hussainweb
Primary language English
commented

I know we've had an issue for this in the past but I thought that was only for the migrations themselves, not the process plugins.

Sure, I thought about this issue too and I did not delve too deep into that issue (I'm guessing this is the one you mean: #2560795: Source plugins have a hidden dependency on migrate_drupal). However, I concluded that doesn't matter in the scope of this issue because all these files depend on a service provided by migrate_drupal anyway. The below snippet is from FieldType.php and there are equivalent calls in other classes as well.

  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration = NULL) {
    return new static(
      $configuration,
      $plugin_id,
      $plugin_definition,
      $container->get('plugin.manager.migrate.cckfield'),
      $migration
    );
  }
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

<yoda>Sense, this patch makes. Commit it we should.</yoda>

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

I think we should add an interface for migrate's plugin manager's because they all do this... see \Drupal\migrate\Plugin\MigrateDestinationPluginManager and then they should implement this interface... this interface should extend PluginManagerInterface.

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Needs work » Needs review
Related issues: +#2787639: MigrateCckFieldPluginManager mixes up its behaviour for creating and loading definitions
StatusFileSize
new interdiff-3-8.txt8.9 KB
new cckmigration_does_not-2787619-8.patch15.28 KB

Here is an interface for MigratePluginManager and this documents as well as provides type safety. The original problem still remains however, unless we add another interface for MigrateCckFieldPluginManager for the changed parameter.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work

How is 

public function createInstance($field_type, array $configuration = array(), MigrationInterface $migration = NULL) {

different from

public function createInstance($plugin_id, array $configuration = array(), MigrationInterface $migration = NULL) {

Ah I see you want to change it for field type - fine another interface if we think that is worth it.

Still way too much typehinting on the concretes in the patch.

hussainweb’s picture

hussainweb
Primary language English
commented

@alexpott: I think another interface is worth it, especially considering #2787639: MigrateCckFieldPluginManager mixes up its behaviour for creating and loading definitions, which is a bigger issue.

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-8-11.txt10.46 KB
new cckmigration_does_not-2787619-11.patch16.99 KB

Adding an interface for MigrateCckFieldPluginManager and changing all references.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

If all complaints are addressed, and it seems to me like they are, I think this is good to go.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/migrate/src/Plugin/MigratePluginManagerInterface.php
@@ -0,0 +1,19 @@
+  /**
+   * {@inheritdoc}
+   *
+   * @param \Drupal\migrate\Plugin\MigrationInterface $migration
+   *   The migration context in which the plugin will run.
+   *
+   * A specific createInstance method is necessary to pass the migration on.
+   */

+++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManagerInterface.php
@@ -0,0 +1,21 @@
+  /**
+   * {@inheritdoc}
+   *
+   * @param string $field_type
+   *   The cck field type which is being migrated.
+   *
+   * This createInstance method uses a field type instead of a plugin ID to
+   * create an instance of the plugin.
+   */

We can't mix inheritdoc with other doc... we need to repleat the full documentation here.

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-11-14.txt2.72 KB
new cckmigration_does_not-2787619-14.patch17.89 KB

Fixing for #13. I think at least the first style is being used elsewhere in core, IIRC but I am anxious to get this committed soon as I am waiting to reroll other patches based on this.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

Pre-emptively re-RTBCed in the quite likely event that this passes all the tests.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

#2787639: MigrateCckFieldPluginManager mixes up its behaviour for creating and loading definitions seems to make parts of this patch invalid.

hussainweb’s picture

hussainweb
Primary language English
commented

I always thought that this would go in first, considering it was simple. I was always going to reroll the other, so I'll do that now. :) Thanks!

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new cckmigration_does_not-2787619-18.patch17.94 KB

I am starting with a reroll. The new interfaces needs the method introduced in #2787639: MigrateCckFieldPluginManager mixes up its behaviour for creating and loading definitions

hussainweb’s picture

hussainweb
Primary language English
commented
StatusFileSize
new interdiff-18-19.txt3.2 KB
new cckmigration_does_not-2787619-19.patch18.75 KB

And now, changing the interface. We don't need to override definition of createInstance anymore as that is now exactly the same as the parent.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

I think that looks okay.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 0d74926 to 8.3.x and 6a38601 to 8.2.x and df8e15a to 8.1.x. Thanks!

  • alexpott committed 0d74926 on 8.3.x 
    Issue #2787619 by hussainweb, phenaproxima, alexpott: CckMigration does...

  • alexpott committed 6a38601 on 8.2.x 
    Issue #2787619 by hussainweb, phenaproxima, alexpott: CckMigration does...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.