Problem/Motivation

Currently we have cckfield plugins that were originally development for the D6 to D8 upgrade path but we need them for D7 as well. Although poorly named (for D7) we can easily re-use the exact same plugins, we just need a way for the builders to decide which plugins to use.

Proposed resolution

  1. Stop using the plugin id as the field machine name, and add a new property called field_name to the definition.
  2. Add a new property to the definition called "core" which can be 6/7/8 etc.
  3. Add support to the cckfield plugin manager to filter on the new core property

Remaining tasks

Write the patch

User interface changes

n/a

API changes

Yes, existing plugins may have to to add the "core" key. We could default to 6.x which should pretty much mean all existing contrib will still work.

Data model changes

n/a

CommentFileSizeAuthor
#101 migrate-cck_core_versions-2631736-101.patch21.21 KBSam152
#101 interdiff.txt711 bytesSam152
#99 migrate-cck_core_versions-2631736-99.patch21.29 KBSam152
#99 interdiff.txt1.45 KBSam152
#96 migrate-cck_core_versions-2631736-96.patch10.37 KBSam152
#96 interdiff.txt5.07 KBSam152
#93 2631736-passing-contrib-example-test-93.txt1.24 MBSam152
#86 interdiff-83-86.txt5.12 KBjmuzz
#86 migrate-cck_core_versions-2631736-86.patch23.26 KBjmuzz
#84 interdiff-81-83.txt5.23 KBjmuzz
#83 interdiff-81-83.txt0 bytesjmuzz
#83 migrate-cck_core_versions-2631736-83.patch25.25 KBjmuzz
#81 interdiff-72-81.txt5.27 KBjmuzz
#81 migrate-cck_core_versions-2631736-81.patch26.01 KBjmuzz
#78 interdiff-2631736-72-78.txt7.01 KBquietone
#78 2631736-78.patch27.69 KBquietone
#76 migrate-cck_core_versions-2631736-72.patch22.31 KBjmuzz
#74 interdiff-72-74.patch1.64 KBquietone
#74 migrate-cck_core_versions-2631736-74.patch22.28 KBquietone
#72 migrate-cck_core_versions-2631736-72.patch22.31 KBjmuzz
#69 migrate-cck_core_versions-2631736-69.patch22.3 KBjmuzz
#67 migrate-cck_core_versions-2631736-67.patch22.59 KBjmuzz
#65 migrate-cck_core_versions-2631736-65.patch19.92 KBjmuzz
#61 interdiff-57-61.txt8.27 KBjmuzz
#61 migrate-cck_core_versions-2631736-61.patch23.34 KBjmuzz
#57 interdiff-53-57.txt1.65 KBjmuzz
#57 migrate-cck_core_versions-2631736-57.patch21.94 KBjmuzz
#53 interdiff-44-53.txt9.37 KBjmuzz
#53 migrate-cck_core_versions-2631736-53.patch21.82 KBjmuzz
#51 MigrateCckFieldPluginManagerTest.txt2.06 KBjmuzz
#51 migrate_cckfield_plugin_manager_test.tar_.gz719 bytesjmuzz
#44 interdiff-2631736-41-44.txt593 bytesquietone
#44 2631736-44.patch15.96 KBquietone
#42 interdiff-2631736-37-41.txt1.03 KBquietone
#42 2631736-41.patch15.91 KBquietone
#37 interdiff-2631736-33-37.txt5.49 KBquietone
#37 2631736-37.patch15.91 KBquietone
#35 interdiff-2631736-33-35.txt6.71 KBquietone
#35 2631736-35.patch14.07 KBquietone
#33 2631736-33.patch16.14 KBquietone
#33 interdiff-2631736-32-33.txt575 bytesquietone
#32 interdiff-2631736-30-32.txt2.38 KBquietone
#32 2631736-32.patch16.24 KBquietone
#30 diff-26-30.txt543 bytesquietone
#30 2631736-30.patch17.19 KBquietone
#26 interdiff-2631736-23-26.txt714 bytesquietone
#26 2631736-26.patch16.7 KBquietone
#23 interdiff-2631736-16-23.txt5.62 KBquietone
#23 2631736-23.patch16.7 KBquietone
#19 2631736-19.patch15.7 KBquietone
#19 interdiff-2631736-16-19.txt995 bytesquietone
#16 interdiff-2631736-11-16.txt1.89 KBquietone
#16 2631736-16.patch15.64 KBquietone
#12 2631736-12.patch14.85 KBjmuzz
#11 2631736-11.patch14.84 KBjmuzz
#10 2631736-10.patch9.94 KBjmuzz
#5 2631736-5.patch14.38 KBsvendecabooter
#4 2631736-4.patch7.16 KBsvendecabooter
#2 2631736-2.patch5.51 KBsvendecabooter
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

benjy created an issue. See original summary.

svendecabooter’s picture

Status: Active » Needs review
FileSize
5.51 KB

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.

Status: Needs review » Needs work

The last submitted patch, 2: 2631736-2.patch, failed testing.

svendecabooter’s picture

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.

svendecabooter’s picture

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.

svendecabooter’s picture

Status: Needs work » Needs review

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.

The last submitted patch, 4: 2631736-4.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: 2631736-5.patch, failed testing.

jmuzz’s picture

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.

jmuzz’s picture

FileSize
9.94 KB

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?

jmuzz’s picture

FileSize
14.84 KB

Ignore previous, I had left out the new files.

jmuzz’s picture

FileSize
14.85 KB

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.

chx’s picture

Issue tags: +SprintWeekend2016
benjy’s picture

Status: Needs work » Needs review

This looks great to me, setting to needs review for the bot.

+++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
@@ -0,0 +1,62 @@
+        if ($field_type == $definition['field_type'] || $field_type == $plugin_id) {

Nit: lets use === here

Status: Needs review » Needs work

The last submitted patch, 12: 2631736-12.patch, failed testing.

quietone’s picture

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

quietone’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 16: 2631736-16.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
995 bytes
15.7 KB

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.

Status: Needs review » Needs work

The last submitted patch, 19: 2631736-19.patch, failed testing.

quietone’s picture

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.

quietone’s picture

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.

quietone’s picture

Status: Needs work » Needs review
FileSize
16.7 KB
5.62 KB

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.

Status: Needs review » Needs work

The last submitted patch, 23: 2631736-23.patch, failed testing.

jonhattan’s picture

+++ 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).

quietone’s picture

Yes. Heres the fix to d6/FieldType.

quietone’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 26: 2631736-26.patch, failed testing.

jmuzz’s picture

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).

quietone’s picture

Status: Needs work » Needs review
FileSize
17.19 KB
543 bytes

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

benjy’s picture

Status: Needs review » Needs work

I think this is nearly ready. Just a few points to fix.

  1. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
    @@ -0,0 +1,63 @@
    +  /**
    +   * Constructs a MigratePluginManager object.
    +   *
    +   * @param string $type
    +   *   The type of the plugin: row, source, process, destination, entity_field,
    +   * id_map.
    +   * @param \Traversable $namespaces
    +   *   An object that implements \Traversable which contains the root paths
    +   *   keyed by the corresponding namespace to look for plugin implementations.
    +   * @param \Drupal\Core\Cache\CacheBackendInterface $cache_backend
    +   *   Cache backend instance to use.
    +   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
    +   *   The module handler to invoke the alter hook with.
    +   * @param string $annotation
    +   *   The annotation class name.
    +   */
    +  public function __construct($type, \Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler, $annotation = 'Drupal\Component\Annotation\PluginID') {
    +    parent::__construct($type, $namespaces, $cache_backend, $module_handler, $annotation);
    +  }
    

    This constructor isn't doing anything? Can just be removed.

  2. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
    @@ -0,0 +1,63 @@
    +      if (in_array($configuration['core'], $definition['core'])) {
    +        if (key_exists($field_type, $definition['type_map']) || $field_type === $plugin_id) {
    +          return parent::createInstance($plugin_id, $configuration, $migration);
    +        }
    +      }
    

    key_exists() never seen that alias used before :)

    Also, the docs on FactoryInterface state:

       * @throws \Drupal\Component\Plugin\Exception\PluginException
       *   If the instance cannot be created, such as if the ID is invalid.

    so we should do that instead of returning FALSE here.

quietone’s picture

Status: Needs work » Needs review
FileSize
16.24 KB
2.38 KB

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.

quietone’s picture

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

benjy’s picture

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.

Or we could just add a try-catch instead? I'll someone else for some input on going against the interface docs.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/CckBuilder.php
@@ -66,15 +66,17 @@ public static function create(ContainerInterface $container, array $configuratio
+  protected function getCckPlugin($field_type, $core = 6, MigrationInterface $migration = NULL) {

+++ b/core/modules/node/src/Plugin/migrate/builder/d7/Node.php
@@ -38,9 +38,11 @@ public function buildMigrations(array $template) {
           if ($this->cckPluginManager->hasDefinition($data['type'])) {
...
+            if ($cckplugin = $this->getCckPlugin($data['type'], 7)) {
+              $cckplugin->processCckFieldValues($migration, $field, $data);

Can we do the hasDefinition call inside of the getCckPlugin as well?

quietone’s picture

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.

benjy’s picture

Do we need extra test coverage for those link changes? Maybe your patch will fail if there is coverage already.

+++ /dev/null
@@ -1,41 +0,0 @@
-class MigrateCckFieldPluginManager extends MigratePluginManager {

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/CckBuilder.php
@@ -76,9 +76,15 @@
+      foreach ($this->cckPluginManager->getDefinitions() as $plugin_id => $definition) {
+        if (in_array($core, $definition['core'])) {
+          if (array_key_exists($field_type, $definition['type_map']) || $field_type === $plugin_id) {
+            return ($this->cckPluginCache[$field_type] = $this->cckPluginManager->createinstance($field_type, [], $migration));

Why move this logic? I think that made sense in the plugin manager, it just needed to throw an exception.

quietone’s picture

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?

phenaproxima’s picture

+++ 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]?

benjy’s picture

Can we safely change it so that, if the core is undefined, it will default to [6, 7]?

If we did that we'd be changing the current behaviour because anyone with an existing plugin now, it would only work for D6. If I was going to change this, i'd be tempted to throw an exception here and say that was a required key?

quietone’s picture

Title: Cckfield Plugins must distinguish between D6 and D7 » Cckfield Plugins must distinguish core versions

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

benjy’s picture

I think this is about ready, @phenaproxima, any thoughts on my response in #39?

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/CckBuilder.php
@@ -75,10 +75,12 @@
   protected function getCckPlugin($field_type, $core = 6, MigrationInterface $migration = NULL) {
...
+    if ($this->cckPluginManager->hasDefinition($field_type)) {
...
+      return $this->cckPluginCache[$field_type];

Not that performance is a big issue here, but if we did a !empty check on the pluginCache first we could avoid the hasDefinition call entirely when we've already got it cached.

quietone’s picture

Status: Needs review » Needs work

The last submitted patch, 42: 2631736-41.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
15.96 KB
593 bytes

Oh, What was I doing?

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Great, thanks. I think this is ready.

catch’s picture

Status: Reviewed & tested by the community » Needs review

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

benjy’s picture

Suggestions for a better name, "field" plugins? Also, that would obviously be an API break, not sure if we want to do that?

catch’s picture

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.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

I think a follow-up would be better so we can open the discussion on a better name. Opened #2683435: CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

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.

jmuzz’s picture

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.

jmuzz’s picture

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.

jmuzz’s picture

Tests added and field_type removed.

quietone’s picture

@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?

jmuzz’s picture

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.

quietone’s picture

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'

jmuzz’s picture

quietone’s picture

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

+1 RTBC

benjy’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/field/migration_templates/d6_field.yml
    @@ -17,7 +17,7 @@ process:
    -      plugin: field_type
    +      plugin: d6_field_type
    

    The D6/D7 field plugins are now identical apart from the core version they specify for the plugin manager. Can we pass that value through as an option. E.g.

      plugin: field_type:
      core: 6
    
  2. +++ b/core/modules/field/migration_templates/d7_field.yml
    @@ -13,7 +13,7 @@ process:
       type:
    -    plugin: static_map
    +    plugin: d7_field_type
         source: type
         map:
           date: datetime
    

    This is a new pattern, extending StaticMap and then calling parent::transform(). Why wouldn't we just have two processes here:

      type:
        -
          plugin: static_map
          source: type
          bypass: true
          map:
            number_decimal: decimal
        -
          plugin: field_type
    
jmuzz’s picture

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.

jmuzz’s picture

Status: Needs work » Needs review
FileSize
23.34 KB
8.27 KB

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.

Status: Needs review » Needs work

The last submitted patch, 61: migrate-cck_core_versions-2631736-61.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jmuzz’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

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.

jmuzz’s picture

Status: Needs review » Needs work

The last submitted patch, 65: migrate-cck_core_versions-2631736-65.patch, failed testing.

jmuzz’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 67: migrate-cck_core_versions-2631736-67.patch, failed testing.

jmuzz’s picture

Status: Needs work » Needs review
FileSize
22.3 KB

Status: Needs review » Needs work

The last submitted patch, 69: migrate-cck_core_versions-2631736-69.patch, failed testing.

jmuzz’s picture

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.

jmuzz’s picture

Status: Needs work » Needs review
FileSize
22.31 KB

This should do it.

jmuzz’s picture

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.

quietone’s picture

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

Status: Needs review » Needs work

The last submitted patch, 74: interdiff-72-74.patch, failed testing.

jmuzz’s picture

Are you sure? This is what I see:

public function createInstance($field_type, array $configuration = array(), MigrationInterface $migration = NULL) {
mikeryan’s picture

quietone’s picture

@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.

jmuzz’s picture

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.

quietone’s picture

@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.

jmuzz’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 81: migrate-cck_core_versions-2631736-81.patch, failed testing.

jmuzz’s picture

Status: Needs work » Needs review
FileSize
25.25 KB
0 bytes

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.

jmuzz’s picture

FileSize
5.23 KB
mikeryan’s picture

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?

jmuzz’s picture

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.

jmuzz’s picture

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.

heddn’s picture

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.

benjy’s picture

We did discuss that earlier: https://www.drupal.org/node/2631736#comment-10914589

I think the reason for keeping it is so that existing code continues to work.

heddn’s picture

Version: 8.1.x-dev » 8.2.x-dev
Issue tags: +Needs manual testing

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.

jmuzz’s picture

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?

Sam152’s picture

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?

Sam152’s picture

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?

Sam152’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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?

Sam152’s picture

Status: Needs work » Needs review
FileSize
5.07 KB
10.37 KB
benjy’s picture

I think the patch in #96 is broken, it only has additions and is less than half the size of the last patch.

Couple of nitpicks but in general I'm +1 to getting this in.

  1. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
    @@ -0,0 +1,47 @@
    +  /**
    +   * {@inheritdoc}
    +   *
    +   * A specific createInstance method is necessary to pass the migration on.
    +   */
    

    Not sure this is valid docs.

  2. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
    @@ -0,0 +1,47 @@
    +    $core = 6;
    

    Can we document this as the default with the reasoning around BC for the current behaviour and maybe even move it to a class property?

  3. +++ b/core/modules/migrate_drupal/tests/modules/migrate_cckfield_plugin_manager_test/src/Plugin/migrate/cckfield/D6FileField.php
    @@ -0,0 +1,29 @@
    + *   type_map = {
    + *     "file" = "file"
    + *   }
    

    Can someone document the differences between type_map and the getFieldType() method, I can't quite follow why we wouldn't just let plugins define their own getFieldType() method rather than the annotation. Is it just meant to make life a bit easier?

Status: Needs review » Needs work

The last submitted patch, 96: migrate-cck_core_versions-2631736-96.patch, failed testing.

Sam152’s picture

Status: Needs work » Needs review
FileSize
1.45 KB
21.29 KB

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);
        }
Sam152’s picture

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

Sam152’s picture

As discussed, invalid comment removed.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

benjy’s picture

Issue tags: -Needs manual testing
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed 262ff52 on 8.2.x
    Issue #2631736 by quietone, jmuzz, Sam152, svendecabooter, benjy:...
mikeryan’s picture

Issue tags: +Needs change record

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...

Status: Fixed » Closed (fixed)

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