Cckfield Plugins must distinguish core versions

Can you elaborate on the purpose of the first step of the proposed solution?
Isn't the "type_map" property designed for this?

Attached is a first attempt at a patch for this.
It adds the "core" property to the annotation, and defaults to "6" if that isn't supplied.
It also adds the "core" property to existing MigrateCckField plugins. Not sure of those default values are correctly set, so this needs some extra review.

Attached is a new attempt at this.
CckFieldPlugins get 2 new properties in their annotation: "core" and "field_type".
The CckBuilder now also loads a CckFieldPlugin based on those values, rather than just loading through the plugin_id.

This would make it possible to load different plugins per core version, even if the field type is the same.

Different approach that moves retrieving the right plugin based on field type and core version to a subclass of MigratePluginManager, created for this purpose.
This allows to get the appropriate CckField plugin when running the FieldType process plugin, which doesn't have the CckBuilder context at that point.
This new patch also uses a field type plugin for Drupal 7 fields, so D8 contrib modules can also extend this with their contrib field types - see #2640920: D7 field migration field_type should be extendable, which can probably be closed in favor of this issue.

Additional thought:
Can't we use the type_map values in the CckFieldPlugin annotations, rather than the new field_type?
That property already exists, and will have the expected values, if i'm not mistaking?

Either way, I'm halting work on this for a few days. Would love to get some feedback on my approach, or for someone to continue fixing this in the meantime.

If contrib field types will be able to extend it then it might make sense for some of the core modules (text, link, etc.) to extend it rather than having everything defined in the field module. It makes more sense IMO for a field migration to be defined in the same module the field is, and they would make good examples for contrib developers.

Here is an attmept at a reroll. The plugin in the file module was split into a d7 and d6 version. I guessed the field_type should be "file" instead of "filefield" in both cases as that is what the d7_field.yml and d6_field.yml have it mapping to.

Though the link module was contrib in d7, the d8 core module is considered the next version of it by its developers. Perhaps the d8 core module should support migrating from it. If it doesn't, then where would migration support for it come from?

Ignore previous, I had left out the new files.

I understand now the field_type should indicate the source field, not the destination field. I've made the correction in this version.

There does seem to be some overlap between field_type and type_map. The plugin ID was being used to indicate the source field type, and that is changing to field_type, so it stands to reason that each MigrateCckField plugin can support one single source field type. If that's the case, why would they each need their own type_map?

I might be missing something since it doesn't look like type_map exists in any currently existing annotations. The closest thing seems to be the values in d7_fields.yml . type_map only seems to be used in one place, which is the default implementation CckFieldPluginBase->getFieldType(), but my understanding is that function should only be passed field types that match the plugin id / field_type. Otherwise the implementation of it in \Drupal\file\Plugin\migrate\cckfield\d6\FileField wouldn't make sense as it always returns either 'image' or 'file'.

For these reasons my impression is that type_map should be removed, not field_type. I'd try to better understand how the migrations for the core fields not defined in a module work before I'd say so for sure.

Discovered that CckMigration was passing an empty configuration array to cckPluginManger. Fixed that and the nit in #14.

But D7 builds the migrations differently than D6. What happens if we only use the new createInstance for D6? This is just a test patch, made quickly.

Oh well. At least four of the failing tests, d7\MigrateFieldFormatterSettingsTest, d7\MigrateFieldInstanceTest, d7\MigrateFieldInstanceWidgetSettingsTest and d7\MigrateFieldTest pass locally. I was hoping for more informative results.

I'd say ignore patch #19 and use #16.

Did some testing with MIgrateFieldInstanceTest. There are 3 cases where the process plugin d7_field_type gets the type from looking it up in the plugin instead of the static map. Those are for 'taxonomy_term_reference', 'file' and 'text'. The lookup for the type for 'file' returns 'file' which is correct. But for 'taxonomy_term_reference', taxonomy_term_reference' is returned instead of 'entity_reference' and for 'text' a null is returned.

Took a bit too get the flow of this.

This version has the 'type_map' annotation added to all the D7 cckfield process plugins, except FileField, and the relevant mappings have been removed from the migration_template, d7_field.yml. The type_map is now used by MigrateCckFiledPluginManager to determine if a plugin instance will be created. If it is created, the d7 fieldType plugin then uses getFieldType() to get the field type from that plugin. It is not created it uses the map as defined in the template. However, the getFieldType method exists in the TextField plugin and it was returning NULL (because the widget_type doesn't exist). For now, I choose not to investigate that and added a call to the parent getFieldType method.

Adding type_map to FileField resulted in errors I've seen before, generated because an image doesn't have a height or width, failing in ImageItem.php. I recall seeing a patch about that, but I may not have time to look into that now.

So, still needs work.

+++ b/core/modules/field/src/Plugin/migrate/process/d6/FieldType.php
@@ -64,11 +64,11 @@ public static function create(ContainerInterface $container, array $configuratio
+    $cckplugin = $this->cckPluginManager->createInstance($field_type, ['core' => 7]);

I guess this is wrong (core=7 in a d6 file).

Yes. Heres the fix to d6/FieldType.

I am able to see the errors when I run the tests on my local copy. Be sure to apply the patch to the latest dev and run the tests for the node module (not just migrate).

For unknown reasons interdiff is no longer working for me on my dev container or host machine so a diff is provided.

thx benjy,

1) Contructor removed.
2) alias removed and replaced with 'array_key_exists'.
3) Returning false is needed, It is used in d6 and d7 Node.php to determine what data to insert into the migration process. Here is the use in d7/Node.php.

           Process through a CckFieldPlugin if available.
          if ($this->cckPluginManager->hasDefinition($data['type'])) {
            if ($cckplugin = $this->getCckPlugin($data['type'], 7)) {
              $cckplugin->processCckFieldValues($migration, $field, $data);
            }
          }
          else {
            $migration->setProcessOfProperty($field, $field);
          }

BTW, I found out there is a bug with interdiff and git patches in Debian Jessie. Only ran into this now because I changed my dev env from Wheezy to Jessie.

Sorry for the noise, I left in 2 commented out use statements, they are removed now.

thx, benjy.

This is an attempt to conform to the interface documentation. Not sure about it, but at least it passes tests locally.
Also made corrections to the d7_field.yml and the link process plugin.

Because it made sense at the time?

Right, so ignore patch #35. I started again, from the patch in #33, here is another attempt. Is this better?

+++ b/core/modules/migrate_drupal/src/Annotation/MigrateCckField.php
@@ -24,6 +25,17 @@
+    // Provide default value for core property, in case it's missing.
+    if (empty($this->definition['core'])) {
+      $this->definition['core'] = [6];
+    }

This patch looks good, at a cursory glance, but this bit seems like a slightly dangerous "gotcha!" assumption. Can we safely change it so that, if the core is undefined, it will default to [6, 7]?

Accurate, and will stop me from thinking this needs a migrate-d6-d8 or migrate-d7-d8 tag every time I see it.

Like this?

Oh, What was I doing?

Is it worth renaming the plugins? It's a bit confusing seeing CCK and 7.x.

Field plugins sounds good.

For 8.1.x the API break should be fine at least during beta I think since migrate is experimental - but might get second opinion.

Better not for 8.0.x though.

We could also open another issue for renaming - if the only difference is the rename don't want to hold this up on fixing the bug but wanted to ask since it does look odd in the patch.

There doesn't seem to be any explicit test coverage - so I'm not sure what we are actually fixing without reading the issue summary.

The

way for the builders to decide which plugins to use

needs testing and also the fallback to Drupal 6 needs testing too.

I made some progress on tests for this. The commented out one is currently throwing an exception. The test module just defines a field migration for a "file" field_type for drupal 6 without any implementation. Hypothetically it should be found by the commented out test.

I might be missing something, but it doesn't look like the field_type annotation is being used for anything in this version of the patch. There was some talk about removing either field_type or type_map from the annotation, and currently only the type_map and the plugin_id seem to be getting checked for when it decides what plugin to use.

If support for field_type will be added, be careful about image fields because they have the same field_type as file fields.

It looks like svendecabooter started this with the approach of adding a new field_type annotation but was considering removing it in favour of type_map which already existed when he got pulled away. I think it would make sense to remove the field_type annotation from the patch.

Tests added and field_type removed.

@jmuzz, thx. Yes, removing field_type makes sense. Just 2 questions.

There are tests for the file, filefield, image and test. Should tests for the taxonomy and link plugins be added?

+++ b/core/modules/migrate_drupal/src/Tests/MigrateCckFieldPluginManagerTest.php
@@ -0,0 +1,63 @@
+    $this->assertIdentical('Drupal\\migrate_cckfield_plugin_manager_test\\Plugin\\migrate\\cckfield\\D6FileField', get_class($plugin_manager->createInstance('file', ['core' => 6])));

It's not clear to me what is being tested here and why the test module is needed. What am I not seeing?

I wanted to test the following cases with the module:

- There are different plugins that handle a field type in Drupal 6 and a field type with the same name in Drupal 7.

- A plugin doesn't specify a core version.

I'm sure it couldn't hurt to have tests for all the core modules. I was only trying to test the plugin manager.

thx, I see now. Maybe a comment on that test for the next person who wonders why it is there?

1. +++ b/core/modules/field/migration_templates/d6_field.yml
   @@ -17,7 +17,7 @@ process:
   -      plugin: field_type
   
   +++ b/core/modules/migrate_drupal/src/Tests/MigrateCckFieldPluginManagerTest.php
   @@ -0,0 +1,63 @@
   +    $this->assertIdentical('Drupal\\migrate_cckfield_plugin_manager_test\\Plugin\\migrate\\cckfield\\D6FileField', get_class($plugin_manager->createInstance('file', ['core' => 6])));
   

   Maybe add a comment, 'Tests correct plugin is returned when a field type with the same name exists in different versions'

2. +++ b/core/modules/migrate_drupal/src/Tests/MigrateCckFieldPluginManagerTest.php
   @@ -0,0 +1,63 @@
   +    // Test fallback when no core version is specified.
   

   I think this should be 'Tests'

Makes sense.

thx again, jmuzz. This looks good, but I don't think I can RTBC because I've also submitted patches.

+1 RTBC

This is a new pattern, extending StaticMap and then calling parent::transform().

I think it makes sense to focus on bringing the existing D6 field migration support to D7 in this ticket. I don't know the history about extending StaticMap or the reasons for why it's being done that way, but somebody who can explain it is more likely to help if a separate ticket for it is made.

This patch is going to be really great for any module developer that provides a custom field type in D7. They'll be able to use the field_type plugin in core which only the core modules currently can access because they are the only entries in the static_map.

Renamed d6_field_type back to field_type, moved it up from the d6 directory, removed d7_field_type, and now field_type is used for all MigrateCckField plugins.

Reroll for 8.1.x . The tests are still failing since the two field plugins were merged. I haven't been able to get meaningful error messages on my installation though so hopefully the testbot will say more.

Now running them on the command line passes but running them through the UI gives 'Failed asserting false is true.'

I wonder what testbot will say.

I haven't solved this but I do notice that when the d7 and d6 field type plugins were separate and the tests were passing their transform functions treated the $value argument differently.

This should do it.

This fix would be helpful for making an upgrade path for the field collections module which provides a custom field type that is in use on many Drupal 7 sites.

Noticed a mismatch in the number of parameters in the createInstance call and the method.

Are you sure? This is what I see:

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

@jmuzz, oh, sorry about that. I did get it wrong. Seems that I have to take more care when the IDE identifies a problem.

At the risk of making another mistake and taking up someone's time I made a new patch. This time to make sure that instances of the cck plugin manager use the MigrateCckFieldPluginManager class, as defined in the service file, and not Drupal\Component\Plugin\PluginManagerInterface.

Can you explain why you think referencing a specific class would be better than using the class's interface? Generally in OOP using an interface is better. Are there other examples in core of specific classes being referenced when an interface is available?

No offense but this change looks a bit random to me and contrary to best practices.

Unless some explanation can be provided I recommend that #72 be reviewed instead.

@jmuzz, no I can't, I'm exploring issues flagged by the IDE and learning OOP as I go. In the meantime, I've had a chat with heddn about this and he has give me some things for further study.

I found that in the D6->D8 version several field related migration plugins use the MigrateCckField / CckFieldPluginBase objects, so it's not just for field_type's. To handle this I have moved the "core" key in the migration templates up to the root of the yml's and added object/function references to them to mirror the ones in the D6 yml's.

Also $row->getSourceProperty('core') was always returning NULL so I changed it to get the value of core from the yml.

Moving the core key to the root of the yml's left the field_type plugin without a way to figure out its core version. This is now being handled by using the migration object that it gets passed when it is made.

If we're moving the core property to the top level of the migration configuration (and I'm not crazy about having a Drupal-source-specific property there), why not just use the existing migration_tags "Drupal 6" and "Drupal 7" instead of a custom property?

It's using the existing migration tags now.

I see what you mean about putting Drupal-source-specific properties there. I wonder if that would apply to the cck_plugin_method property now too. It refers to a method that will be called on a MigrateCckField plugin. That plugin may not have been Drupal-source-specific by design even though it hasn't been used in a non Drupal-source context yet. It will definitely be Drupal-source-specific after the core annotation from this patch is added to it.

I've been working on the field formatters too. It doesn't seem directly related to core versions but I've been using this patch as a start so it may be a good follow up issue.

Did a code review. No actual, live testing so not marking as RTBC.

+++ b/core/modules/migrate_drupal/src/Annotation/MigrateCckField.php
@@ -19,6 +20,17 @@
+      $this->definition['core'] = [6];

I think this is safe, but we should remember to mention in the release notes that the default definition is D6 if it isn't set. Better yet, maybe we should just hard-fail.

I'm ok with decision from #89 to stick with a of D6. I would, however, really like it if we could get some real, live migrations run with the patch.

Since this seems like a fairly big change mid-flight for 8.1, I'd feel more comfortable putting this into 8.2 instead. For that reason, I've moved it to the next version.

Leaving in needs review for now to get additional reviews, but this shouldn't move to RTBC until manual testing has occurred.

Well the automated tests seem to be using some pretty elaborate datasets. They're snapshots of fully functional drupal installations if I'm not mistaken.

The problem with wanting this to be tested in the "real world" is that before the patch is applied custom field modules can't even integrate with the system, so it would require not only the patch to be applied but also the migration objects for the custom fields need to be developed, which nobody will have done yet because it wasn't possible.

For what it's worth I have been doing many manual migrations while making this patch, and other than my field collections (which are going to need more than just this required patch) all the data has been getting migrated from the drupal 7 site which I set up from scratch using the installation wizard and the administration interface to set up all the structure.

I mean, honestly, is there anything I can do right now to satisfy your requirements here?

If anyone wants to test the patch against a real example, I have created #2631580: Write migration from Drupal 7. Sadly wasted a few hours on this before realising the functionality wasn't supported.

I agree with #91, this is important to get in. What is actually required to move this issue forward?

Here is a test for video_embed_field 8.x-1.x that passes only when this issue is applied. Can this stand as the test case and real world example of this working?

Based on witnessing this working and being a major contrib blocker, this is an RTBC from me.

1. +++ b/core/modules/field/src/Plugin/migrate/process/FieldType.php
   @@ -0,0 +1,89 @@
   +/**
   + * @file
   + * Contains \Drupal\field\Plugin\migrate\process\FieldType.
   + */
   

   No need for @file anymore.

2. +++ b/core/modules/field/src/Plugin/migrate/process/FieldType.php
   @@ -0,0 +1,89 @@
   +   * @param \Drupal\Component\Plugin\PluginManagerInterface $cck_plugin_manager
   +   *   The cckfield plugin manager.
   +   */
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, PluginManagerInterface $cck_plugin_manager, $migration = NULL) {
   

   Missing $migration param docs and should it be typehinted to MigrationInterface?

3. +++ b/core/modules/field/src/Plugin/migrate/process/FieldType.php
   @@ -0,0 +1,89 @@
   +    $migration = func_get_args()[4];
   +    if (!($migration instanceof MigrationInterface)) {
   +      $migration = NULL;
   +    }
   

   Why can't we add $migration to the create definition? TBH this code looks very odd. Why can we just use the same definition for create as \Drupal\migrate\Plugin\migrate\process\MenuLinkParent does?

Patch only contained new files. Attached is a complete one with an interdiff for #97. Re: point 1, this is used elsewhere from what I can see. Re: 3, I can't speak to that but it looks like it's used to return the right plugin based on the annotation before creating an instance:

        if (array_key_exists($field_type, $definition['type_map']) || $field_type === $plugin_id) {
          return parent::createInstance($plugin_id, $configuration, $migration);
        }

In CckFieldPluginBase::getFieldType, the type_map annotation is used as a basis for the field type. So they are one in the same?

As discussed, invalid comment removed.

Committed 79e3f36 and pushed to 8.1.x. Thanks!

diff --git a/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
index fecf26a..44151cc 100644
--- a/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
+++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
@@ -34,7 +34,7 @@ public function createInstance($field_type, array $configuration = array(), Migr
     if (!empty($configuration['core'])) {
       $core = $configuration['core'];
     }
-    else if (!empty($migration->getPluginDefinition()['migration_tags'])) {
+    elseif (!empty($migration->getPluginDefinition()['migration_tags'])) {
       foreach ($migration->getPluginDefinition()['migration_tags'] as $tag) {
         if ($tag == 'Drupal 7') {
           $core = 7;

Coding standard fixed on commit.

We should have a change record for this, no? At least to let people writing D7 migrations know that their source plugins need to add the core annotation...