CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins

This would disrupt #2631736: Cckfield Plugins must distinguish core versions, which is clearly more important, so let's not try to do this until that is committed.

Unblocked.

Renaming stuff makes for a nice mindless Friday-afternoon task... The basic algorithm was to remove "CCK" wherever it appeared next to "field", and otherwise replace it with "field".

Missed one file rename...

I'd like to add a BC layer where possible (deprecated CCK-named plugins/classes wrapped around the new names).

BC layer added, draft change record written. Interdiff was a bit confused by the BC layer, but I'm assuming no one has reviewed too closely so far...

I think at least the plugins need the BC layer - people have been implementing custom Drupal-to-Drupal migration paths and have migrations referencing those plugins, which they will have to edit if we don't have the BC layer.

Will this cause the processing to run twice?

If you're using the d6_cck_link plugin, it's implemented by the CckLink class which simply wraps the FieldLink class and runs the code in the FieldLink class.

1. +++ b/core/modules/migrate_drupal/src/Annotation/MigrateField.php
   @@ -0,0 +1,54 @@
   + * (provided by CCK in Drupal 6 and Field API in Drupal 7) to Drupal 8. They are ¶
   

   There's an extra character of whitespace at the end of this line, and the git gods will smite us if it stays.

2. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldInterface.php
   @@ -2,86 +2,26 @@
   +   * Apply any custom processing to the field bundle migrations.
   +   * ¶
   

   Same here (the blank line).

3. +++ b/core/modules/migrate_drupal/tests/modules/migrate_field_plugin_manager_test/src/Plugin/migrate/field/D6FileField.php
   @@ -0,0 +1,29 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getFieldFormatterMap() {}
   

   Isn't this supposed to return an array?

4. +++ b/core/modules/migrate_drupal/tests/modules/migrate_field_plugin_manager_test/src/Plugin/migrate/field/D6NoCoreVersionSpecified.php
   @@ -0,0 +1,25 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getFieldFormatterMap() {}
   

   Ditto here.

Fixed the trailing whitespace issues.

+++ b/core/modules/migrate_drupal/tests/modules/migrate_field_plugin_manager_test/src/Plugin/migrate/field/D6FileField.php
@@ -0,0 +1,29 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function getFieldFormatterMap() {}

Isn't this supposed to return an array?

Perhaps - but this is not an actual change in this patch, that's just code that moved.

Had to reroll for recent changes - interdiff was unhappy because the old patch no longer applies....

Rerolled and empty array returns added to the tests.

Random update test failure, retesting.

This patch is a lot less intimidating than it appears. It's just renaming things. And to that extent, it looks completely legit and incredibly thorough. Excellent work, @mikeryan!

My one question is this. If this already constitutes a BC break (and the issue is tagged as such), why are we bothering to keep the deprecated CckField cruft around? Can't we just remove it, flat-out, and break BC?

I neglected to remove the tag when I added the BC layer.

Well, if we're going to leave the BC layer in for now, then maybe we can remove the cckfield crap in the next minor version, as suggested by @catch on IRC.

In the meantime, I dig this patch. As far as I'm concerned, RTBC.

Reroll. Only one file didn't apply, core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php.

Missed that the D7 link migration was changed in #2674090: Unable to migrate D7 link fields. Rolled that in and it passes locally.

If this is no bc break, I think we should consider doing it against 8.2.x, then remove the bc in 8.3.x - that makes it more likely to happen before migrate becomes beta (after which I don't think we could remove the bc). It seems most likely people would be using these in specific migrations, which are less likely to persist beyond a minor release anyway.

Seems to me that the field plugin manager should be of class Drupal\migrate_drupal\Plugin\MigrateFieldPluginManager not Drupal\Component\Plugin\PluginManagerInterface. That's changed, plus some other tidy up.

And removed the reroll tag.

It actually should be PluginManagerInterface, since we try to type hint to interfaces rather than actual classes. It loosens the coupling in general and makes it easier to mock test objects. Can you restore the original type hint?

It also makes the IDE complain. But sure. Restored. The only change then is removing two use statements identified by the IDE as not being used. It is easier to see the wee change with interdiff against #29.

Seems completely legit.

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/cckfield/CckFieldPluginBase.php
   @@ -1,83 +1,17 @@
   -namespace Drupal\migrate_drupal\Plugin\migrate\cckfield;
   +namespace Drupal\migrate_drupal\Plugin\migrate\field;
   

   The namespace shouldn't really change here. Apart from breaking BC, it also makes this class impossible to find as the directory hasn't changed.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/cckfield/CckFieldPluginBase.php
   @@ -1,83 +1,17 @@
   +abstract class CckFieldPluginBase extends FieldPluginBase {
    
    }
   

   As per standards, this could all be on one line.

Another thing I realized is that since the migrations now use the newer classes, which use the newer annotations, and so on, the tests really don't check for old classes anymore. I don't really care too much about that though.

I fixed the points in #35 and also reduced the patch size by detecting more renames. Hopefully, that will be easier to review too.

1. +++ b/core/modules/file/src/Plugin/migrate/process/d6/CckFile.php
   @@ -2,103 +2,12 @@
   + * @deprecated in Drupal 8.2.x and will be removed in Drupal 9.0.x. Use the
   

   This will probably be removed before D9, so let's change "in" to "before".

2. +++ b/core/modules/link/src/Plugin/migrate/process/d6/CckLink.php
   @@ -2,85 +2,12 @@
   + * @deprecated in Drupal 8.2.x and will be removed in Drupal 9.0.x. Use the
   

   s/in/before

3. +++ b/core/modules/migrate_drupal/migrate_drupal.services.yml
   @@ -1,4 +1,13 @@
   +  # @deprecated in 8.2.x, to be removed in 9.0.x.
   

   s/in/before

4. +++ b/core/modules/migrate_drupal/src/Annotation/MigrateCckField.php
   @@ -2,53 +2,16 @@
   + * @deprecated in Drupal 8.2.x, to be removed in Drupal 9.0.x. Use
   

   s/in/before

5. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldInterface.php
   @@ -2,86 +2,10 @@
   + * Provides an interface for all field type plugins.
     */
   -interface MigrateCckFieldInterface extends PluginInspectionInterface {
   

   This interface is missing a deprecation notice.

6. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
   @@ -2,54 +2,14 @@
   + * @deprecated in Drupal 8.2.x, to be removed in Drupal 9.0.x. Use
   

   s/in/before

7. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateFieldPluginManager.php
   @@ -0,0 +1,55 @@
   +      foreach ($migration->getPluginDefinition()['migration_tags'] as $tag) {
   +        if ($tag == 'Drupal 7') {
   +          $core = 7;
   +        }
   +      }
   

   Why not use in_array() here?

8. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php
   @@ -2,122 +2,10 @@
   + * @deprecated in Drupal 8.2.x, to be removed in Drupal 9.0.x. Use
   

   s/in/before

9. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/FieldMigration.php
   @@ -0,0 +1,116 @@
   + * Migration plugin class for migrations dealing with field values.
   

   Can we change this to "field configuration and values"?

10. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/cckfield/CckFieldPluginBase.php
    @@ -2,82 +2,14 @@
    +use Drupal\views\Plugin\views\field\FieldPluginBase;
    

    Whoops! This is the wrong FieldPluginBase.

11. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/cckfield/CckFieldPluginBase.php
    @@ -2,82 +2,14 @@
    + * @deprecated in Drupal 8.2.x, to be removed in Drupal 9.0.x. Use
    

    s/in/before

12. +++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateFieldPluginManagerTest.php
    @@ -0,0 +1,58 @@
    +    $plugin_manager = \Drupal::service('plugin.manager.migrate.field');
    

    Let's use $this->container->get() rather than \Drupal::service().

13. +++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateFieldPluginManagerTest.php
    @@ -0,0 +1,58 @@
    +    $this->assertIdentical('Drupal\\file\\Plugin\\migrate\\field\\d6\\FileField', get_class($plugin_manager->createInstance('filefield', ['core' => 6])));
    

    Can we change this to assertInstanceOf() for readability?

14. +++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateFieldPluginManagerTest.php
    @@ -0,0 +1,58 @@
    +    try {
    +      // If this test passes, createInstance will raise a
    +      // PluginNotFoundException and we'll never reach fail().
    +      $plugin_manager->createInstance('filefield', ['core' => 7]);
    +      $this->fail('Expected Drupal\Component\Plugin\Exception\PluginNotFoundException.');
    +    }
    +    catch (PluginNotFoundException $e) {
    +      $this->assertIdentical($e->getMessage(), "Plugin ID 'filefield' was not found.");
    +    }
    

    I'd rather this were its own test method using the @expectedException and @expectedExceptionMessage annotations.

15. +++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateFieldPluginManagerTest.php
    @@ -0,0 +1,58 @@
    +    $this->assertIdentical('Drupal\\file\\Plugin\\migrate\\field\\d7\\ImageField', get_class($plugin_manager->createInstance('image', ['core' => 7])));
    +    $this->assertIdentical('Drupal\\file\\Plugin\\migrate\\field\\d7\\FileField', get_class($plugin_manager->createInstance('file', ['core' => 7])));
    +    $this->assertIdentical('Drupal\\migrate_field_plugin_manager_test\\Plugin\\migrate\\field\\D6FileField', get_class($plugin_manager->createInstance('file', ['core' => 6])));
    +
    +    $this->assertIdentical('Drupal\\text\\Plugin\\migrate\\field\\TextField', get_class($plugin_manager->createInstance('text', ['core' => 6])));
    +    $this->assertIdentical('Drupal\\text\\Plugin\\migrate\\field\\TextField', get_class($plugin_manager->createInstance('text', ['core' => 7])));
    +
    +    // Test fallback when no core version is specified.
    +    $this->assertIdentical('Drupal\\migrate_field_plugin_manager_test\\Plugin\\migrate\\field\\D6NoCoreVersionSpecified', get_class($plugin_manager->createInstance('d6_no_core_version_specified', ['core' => 6])));
    

    Let's change all of these to assertInstanceOf().

16. +++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateFieldPluginManagerTest.php
    @@ -0,0 +1,58 @@
    +    try {
    +      // If this test passes, createInstance will raise a
    +      // PluginNotFoundException and we'll never reach fail().
    +      $plugin_manager->createInstance('d6_no_core_version_specified', ['core' => 7]);
    +      $this->fail('Expected Drupal\Component\Plugin\Exception\PluginNotFoundException.');
    +    }
    +    catch (PluginNotFoundException $e) {
    +      $this->assertIdentical($e->getMessage(), "Plugin ID 'd6_no_core_version_specified' was not found.");
    +    }
    

    Can this be its own method with @expectedException and @expectedExceptionMessage?

17. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
    @@ -250,6 +249,24 @@ protected function getCckData(array $field, Row $node) {
    +   * @deprecated in Drupal 8.2.x, to be removed in Drupal 9.0.x. Use
    

    s/in/before

@phenaproxima: Regarding most of your comments for s/in/before/: I think it is how we are interpreting those words. I see your point that it might mean that it will be present in Drupal 9.0 but it also means that it will be removed in Drupal 9.0, which means before Drupal 9.0 is released.

I checked how this is used in other parts of the core and it is mixed. Some places mention in while others use before. To me, the meaning is clear with both (because it is semver).

For point 9, I changed it to 'field config and values' as it goes beyond 80 characters.

I am fixing point 12 even though it's slightly out of scope because it is a simple change, but I think points 13-16 can be a follow-up. Actually, code in points 13 and 15 gets changed in #2787639: MigrateCckFieldPluginManager mixes up its behaviour for creating and loading definitions and then we don't need assertInstanceOf at all.

Based on chat in IRC with @phenaproxima, I am changing s/in/before/. I think I got them all.

Rerolling first.

The patch got messed up for some reason. I don't know how but I am inclined to blame OS X.

This patch should apply correctly and apart from the reroll, also changes references to MigrateCckFieldPluginManager.

Kind of a stupid mistake. :/

Fixing the failure in plugin manager test.

Looks great!

+++ b/core/modules/field/migration_templates/d6_field.yml
@@ -2,8 +2,8 @@ id: d6_field
-class: Drupal\migrate_drupal\Plugin\migrate\CckMigration
-cck_plugin_method: processField
+class: Drupal\migrate_drupal\Plugin\migrate\FieldMigration
+field_plugin_method: processField

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php
@@ -2,131 +2,10 @@
-          call_user_func([$this->cckPluginCache[$field_type], $this->pluginDefinition['cck_plugin_method']], $this);

I don't see how this is maintaining BC - existing migrations are going to be broken by this change - no?

@alexpott, where are you seeing that? I can't locate the line in question...

I see - if someone has an existing migration containing

class: Drupal\migrate_drupal\Plugin\migrate\CckMigration
cck_plugin_method: processField

CckMigration wraps FieldMigration so the right class is used, but then call_user_func() is looking for field_plugin_method and getting cck_plugin_method instead.

The attached patch will fall back to cck_plugin_method if field_plugin_method is not available.

Yes, git diff 8.2.x works better...

@alexpott explained his comment to me while sprinting yesterday. Nice catch.

I think the BC layer can been improved.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/FieldMigration.php
@@ -0,0 +1,140 @@
+          if (!empty($this->pluginDefinition['field_plugin_method'])) {
+            $method = $this->pluginDefinition['field_plugin_method'];
+          }
+          else {
+            // For backwards compatibility, fallback to the old
+            // cck_plugin_method.
+            $method = $this->pluginDefinition['cck_plugin_method'];
+          }

To do this I would put a const on the class.. on this class the constant would be equal to 'field_plugin_method' and on the old class that has been gutted should override this constant and set it to cck_plugin_method.

This way once we remove the old class we no longer support 'cck_plugin_method' and don't need to remember to change the new class.

I couldn't get to this earlier but I had a slightly different approach to solving this. Since the 'cck_plugin_method' only comes in picture when used with CckMigration, it would be better to isolate the changes there. This is what I had in mind.

class CckMigration extends FieldMigration {

  public function getProcess() {
    $this->pluginDefinition['field_plugin_method'] = $this->pluginDefinition['cck_plugin_method'];
    return parent::getProcess();
  }

}

With the above, the changes to FieldMigration in #55 can be reverted. We could probably add checks to the above but this is just to convey the idea.

The approach in #55 works too but the above code keeps the deprecated parts in the deprecated class and hence easier to remove later.

Ah, cross-post. The const idea by @alexpott seems like lesser work and better than what I said.

Implemented alexpott's suggestion of adding a constant that defines which configuration option has the migration processing function.

No interdiff because I'm getting a weird error from it ("hunk-splitting is required in this case, but is not yet implemented"). But I saw this patch before it was posted, and the changes look OK to me. RTBC.

@phenaproxima that is because the patch in #61 is not made with with

[diff]
        renames = copies

in the git config... @michael_wojcik see https://www.drupal.org/documentation/git/configure

+++ b/sites/default/default.settings.php
@@ -144,6 +144,11 @@
+ *
+ * Per-table prefixes are deprecated as of Drupal 8.2, and will be removed in
+ * Drupal 9.0. After that, only a single prefix for all tables will be
+ * supported.
+ *

This is not part of the patch :)

Here's a new patch without that and coding standards fix. Interdiff back to #55.

Well caught, @alexpott. Thank you! Pre-emptively re-RTBC assuming Drupal CI passes.

This is now soft-blocking #2447727: Add base class for migrating reference fields.

Committed and pushed 24f816a to 8.3.x and dbf8ac6 to 8.2.x. Thanks!

Before migrate goes into beta we should get this done - there is a BC layer so existing migrations should not be affected.

Committing this to 8.2.x after 8.2.1 was released is odd. As a maintainer I must either drop 8.2.1 support, or use the deprecated classes.

s/processCckFieldValues/processFieldValues is a BC break, but not sure it's worth fixing given it's such a minor change. Perhaps a mention in the CR.

I've opened #2816529: Any references to CckFieldPluginBase should now use FieldPluginBase. The tests didn't pass locally but will see what the bot says.

Missed that part. Time to revert this from 8.2.x then?

Ah damn the BC layer doesn't work because the old base class. Untested BC layers suck... let's fix that.

Here's the fix - I should have spotted this. And we should add a test too.

Shouldn't the method name/call be the other way around?

@Sam152 nope. We've renamed processCckFieldValues to processFieldValues so I think #73 was correct.

If you apply #73, in older plugins there is still no implementation of processFieldValues, which is what the new interface requires. #74 fixes this.

+++ b/core/modules/node/src/Plugin/migrate/D6NodeDeriver.php
@@ -130,12 +130,12 @@ public function getDerivativeDefinitions($base_plugin_definition) {
-              $this->cckPluginCache[$field_type]
-                ->processCckFieldValues($migration, $field_name, $info);
+              $this->fieldPluginCache[$field_type]
+                ->processFieldValues($migration, $field_name, $info);

Ah I see @Sam152 is right :) nice one.

So we can go one better and add an abstract method to keep the old interface promise alive and well.

So we need to add a test module that leverages every aspect of the BC layer added here so we don't break things again.

I stared at this with a befuddled expression on my face for the better part of an hour before I figured out how to write a test for it. As far as I can tell, there's not really much to this BC layer that can be actively tested -- really, it's just the processCckFieldValues() method, and that is what I have added a test for in this patch. Everything else in the BC layer would, I'd think, reveal itself very clearly if it wasn't working -- I imagine that the Migrate tests would be blowing up left and right with complaints about non-existent classes and interfaces, or calls to non-existent methods. If there are other aspects of the BC layer that need explicit tests, I'm all ears.

Apologies that there is no interdiff -- for some reason, despite my best efforts, the patch is always an extra 30 KB when I roll it, so interdiff gives me flak about "hunk-splitting", whatever that means, and refuses to generate an interdiff.

Here's a proper patch which looks the right size and an interdiff to #78.

+++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateFieldPluginManagerTest.php
@@ -56,4 +57,22 @@ public function testPluginSelection() {
+    // Ensure that the processCckFieldValues() method exists and is callable
+    // (i.e., public).
+    $callable = [$plugin, 'processCckFieldValues'];
+    $this->assertInternalType('callable', $callable, 'processCckFieldValues method is callable');

We need to call process on it and ensure that processCckFieldValues() is called. Atm this is not really testing the BC layer.

Okay. After getting clarification from @alexpott in IRC, here is a better test that actually verifies the BC layer for processCckFieldValues(). Sorry about the continued lack of an interdiff; my git is still misbehaving for whatever reason.

Actually, there isn't a BC break here. What was I thinking? However, it is blocking #2631698: Fix sub-optimal DX in MigrateFieldInterface, which is a BC break.

Rerolled and fixed the broken test. No interdiff because reroll.

This is almost certainly an API Break. If nothing else there is the new/changed abstract method in CckFieldPluginBase so custom cck migrations would have to change. Whether that's ok is a different discussion.

+++ b/sites/default/default.settings.php
@@ -144,6 +144,11 @@
+ *
+ * Per-table prefixes are deprecated as of Drupal 8.2, and will be removed in
+ * Drupal 9.0. After that, only a single prefix for all tables will be
+ * supported.
+ *

This doesn't look right and wouldn't apply locally.

This is almost certainly an API Break. If nothing else there is the new/changed abstract method in CckFieldPluginBase so custom cck migrations would have to change. Whether that's ok is a different discussion.

Could you elaborate on this? What I see in the current patch is that processCckFieldValues() got moved from the interface into an abstract function on CckFieldPluginBase - the signature is the same, and custom plugins derived from CckFieldPluginBase will need to define it either way, so I don't see how they would break.

This doesn't look right and wouldn't apply locally.

Looks like someone missed a rebase? Setting "needs work" to fix this...

Reroll of the latest patch. Something changed in file/src/Plugin/migrate//process/d6/CckFile.php.

@heddn, can you review and possibly RTBC?

Thanks.

1. +++ b/core/modules/field/src/Plugin/migrate/process/FieldType.php
   @@ -41,14 +41,14 @@ class FieldType extends StaticMap implements ContainerFactoryPluginInterface {
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrateFieldPluginManagerInterface $field_plugin_manager, MigrationInterface $migration = NULL) {
   

   Does this break BC?

2. +++ b/core/modules/file/src/Plugin/migrate/process/d6/CckFile.php
   @@ -2,91 +2,9 @@
   \ No newline at end of file
   

   Nit.

3. +++ b/core/modules/file/src/Plugin/migrate/process/d6/FieldFile.php
   @@ -75,7 +76,18 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +    // throw a MigrateSkipRowException. It shouldn't, but that is being dealt
   +    // with at https://www.drupal.org/node/2487568. Until that lands, return
   +    // an empty item.
   +    catch (MigrateSkipRowException $e) {
   +      return [];
   +    }
   

   Issue 2487568 is closed won't fix. Why is this comment in here? I think this came from a re-roll. This entire try/catch was recently removed from the codebase in another commit.

4. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldInterface.php
   @@ -2,86 +2,11 @@
   + * Provides an interface for all field type plugins.
   

   This is still for cck field types, so no need to change the wording here.

5. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldPluginManager.php
   @@ -2,54 +2,12 @@
   + * Deprecated: Plugin manager for migrate field plugins.
   

   Same here. In the BC layer, this is still for cckfields.

6. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php
   @@ -2,131 +2,17 @@
   + * Migration plugin class for migrations dealing with field values.
   

   BC layer should still call this CCK field values, no?

7. +++ b/sites/default/default.settings.php
   @@ -144,6 +144,11 @@
   + * Per-table prefixes are deprecated as of Drupal 8.2, and will be removed in
   + * Drupal 9.0. After that, only a single prefix for all tables will be
   + * supported.
   

   This is a stray insertion.

We are fairly close to committing this, but to make it easier for reviews, I've opened #2833206: Convert Migrate's cckfield plugins to use the new field plugins to handle the actual conversion work. Let's slim this back to a single conversion and testing the BC layer. Then punt the rest into the follow-up. With the lack of interdiffs and the re-roll issues we've been facing, I think splitting is going to make things a lot easier.

Discussed in the weekly migrate meeting and decided that splitting this into two issues has decent rational. Assigning to quiteone for additional follow-up.

First, fixes for 2-7 in #95.

Remove the migration ymls.

slim this back to a single conversion

Tried to do this but then MigrateNodeTest fails.

Needs a reroll after #2673960: Unable to migrate D7 User cck fields. Tagging as novice for the reroll.

I've re-rolled the patch. There was the expected conflict of the User migrate plugin, whose base class needed to change.

I couldn't seem to get a sensible interdiff, sorry!

re #102 a interdiff isn't necessary for a re-roll. However, it looks like there might be some imports that didn't get fixed-up. Go ahead and give it another try today. It's close.

Ah yeah, sorry, this fixes the obvious issue with the patch in #102.

Apologies for the noise. PHPStorm was even warning me about those.

I came for a simple-ish re-roll and mistakes aside, I think there's a non-trivial issue remaining now.

Trying to make sense of this patch etc, it looks like maybe the term reference field isn't being migrated properly.

This possibly makes sense, because the D7TaxonomyTermDeriver class is using the cckfield BC layer, and calling processCckFieldValues still I can't see how that makes it back to call processFieldValues, but I'm not really sure, sorry!

I think D7TaxonomyTermDeriver made it in after this patch in #99. So, update that file to use the field plugins and not cck plugins.

Right, tests pass now. Still to do is

slim this back to a single conversion

. Anyone up for working on that?

Removing novice and needs reroll tags and setting to NW.

This is critical because it's blocking a critical: #2447727: Add base class for migrating reference fields

slim this back to a single conversion

Any chance you could outline what this involves? From my limited understanding of this issue on of these conversions was performed between the patches in #107 and #112? So we'd be rolling that back? But we already know that won't work. So is it that or is it something else?

@Steven Jones, my comment there is to put in the new plumbing and do only a single conversion. Then punt all the rest to a follow-up. At nearly 100K, this get very complicated to do a valid review. The plumbing by itself is fairly large. So, let's commit it, prove it works by doing a single conversion and do the rest later.

@heddn Okay, so what I'm getting at it is what is a 'single conversion'?

Would an example from the patch in #112 be the conversion of core/modules/file/src/Plugin/migrate/cckfield/d7/ImageField.php to core/modules/file/src/Plugin/migrate/field/d7/ImageField.php ?

So we undo all but one of those conversions and if the patch still passes then we should be good to go, and we pop those individual conversions back in other patches.

Or

Is it that the changes to D7TaxonomyTermDeriver need to be rolled back to get this in?

It seems like we have a BC layer for the field plugins, but not for the migrations themselves, is that understanding correct?
I.e. people will need to upgrade their migrations that are using cckfield plugins, but people implementing cckfield plugins will get the BC layer.

re #118: the first option. Patch in #112 and a single conversion; the example of ImageField is fine. The rest in a follow-up.

I applied the patch in #112 and then went through and un-did what I think were all the changes for the 'conversions' leaving the plumbing and hopefully valid tests in place. This is the -no-conversions patch.

I then re-added the changes for the Imagefield, i.e. the single conversion discussed in #118 and #119. This is the -imagefield-conversion.patch

I don't have a local testing environment set up at the moment, so these patches may fail I suppose. If they do I'll get an environment set up and work on them until they don't.

I've had a go at re-writing the no-conversions patch and will take on the imagefield conversion patch next.

...and here's an interdiff.

Here's another attempt at the no-conversions patch from which to base the (single) imagefield conversion patch.

Yet another improved version of the no-conversions patch.

The single conversion patch will be based from (the final version of) this - with the filefield updated (rather than imagefield which only has a D7 version).

Please shout if you think this patch is taking things in the wrong direction - I don't want to be wasting my time!

Rerolled the patch.

I understand the intention was to include filefield but I has some errors and thought I would try with the link field. That seems to work, so lets see what testbot finds.

No, that is wrong. I left in core/modules/link/src/Plugin/migrate/cckfield and didn't update the associated CckLinkTest.

Ignore patch in #131.

Another attempt at add just the link migration.

After too much blank staring at field types, field formatters and field widgets and caffeine I finally spotted something worth changing. It is easy to see by comparing d6_field_instance_widget_settings.yml and d7_field_instance_widget_settings.yml. The d7 version converts 'link_field' to 'link_default' but the d6 version has no similar conversion for 'link'. Simply adding 'link: link_default' in the d6 migration template is an improvement, d6/MigrateNodeTest fails without that change. Although MigrateUpgrade6Test fails on the link migration.

But I've yet to explain why in the full cck environment this works without that change. What am I missing?

Let's see what else the testbot finds.

The map table error was "Failed to lookup field type array ( 0 => 'link',...". Sorry about the abbreviated error, I didn't save it. In response to that,, this patch adds link to the static map looking in d6_field_formatter_settings.

No, that is wrong too. Rubber duck (actually, mine is a gnu soft toy) programming isn't working for me on this.

Big thank you to joelpittet for his assistance via a hangout. Full credit to him for finding the problem (and showing me another way to think about the problem), I just made the patch.

There are 7 files that use the new MigrateFieldPluginManager. Six of those try to get the plugin from the MigrateCckFieldPluginManager first and if that fails it then uses the Migrate FieldPluginManager. Just needed to make the 7th file, FieldMigration, do that and things are much better.

This is working locally with the link conversion. But before testing that, let's make sure all the tests pass with these changes to the no-conversions patch.

OK more errors. I'm not sure the 'proper' way to fix FieldMigration and CckMigration. Can someone provide some guidance on that?

I think all that was missing was a little tweak to Drupal\user\Plugin\migrate\User (which extends FieldMigration which was edited in #138) - locally it now passes the tests that failed on #138, let's see what the testbot has to say...

Now that we (hopefully) have a stable "no conversions" version, here is a version with a single conversion: linkfield.

(Assuming the testbot passes this patch) we now meet @heddn's requirements from way back in #96. RTBC perhaps?

@Jo Fitzgerald, thanks for looking at this again. I noticed that since User.php extends from FieldMigration it doesn't need it own constructor, so I removed all that. And I moved the cckPluginCache property into FieldMigration.

Sorry, rename the previous interdiff wrong.

Now apply the changes for the link conversion.

With that small change we are ready for review.

From #96

Discussed in the weekly migrate meeting and decided that splitting this into two issues has decent rational. Assigning to quiteone for additional follow-up.

After that the goal was to make two patches, one with just the renaming and another with one conversion added. That has now been achieved.

The patch in #143 does the renaming only. The patch in #145 does the renaming plus one conversion. That will prove it works with both cck plugins and field plugins in play and it makes the patch smaller and easier to review. The conversion done is to rename the d6 and d7 link plugins from @MigrateCckField to @MigrateField.

Assigning to myself to review.

Re-rolled.

Let's get this one moving again and push it over the line.

The field plugin, LinkField.php, was in the directory cckfield instead of field.

Patch cleanly applied.

@dbjpanda, thank you for your review. You say that patch applied, but what other tests or review did you do? What comes to mind is checking that it meets the coding standards, running the relevant tests locally or even running a real migration using this patch. There is more information about Reviewing Patches in the handbook.

For now, setting back to NR.

+++ b/core/modules/field/src/Plugin/migrate/process/FieldType.php
@@ -76,7 +88,13 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+      try {
+        $plugin_id = $this->fieldPluginManager->getPluginIdFromFieldType($field_type, [], $this->migration);
+        return $this->fieldPluginManager->createInstance($plugin_id, [], $this->migration)->getFieldType($row);
+      }
+      catch (PluginNotFoundException $e) {
+        return parent::transform($value, $migrate_executable, $row, $destination_property);
+      }
     }

I think it would make more sense to flip this logic and do try first on the new fieldManager, followed by cck.

And we should re-roll things for #2575081: [policy, no patch] Use E_USER_DEPRECATED in Drupal 8 minor releases and https://www.drupal.org/node/2575081. Almost there.

Flipped the logic on all the occurrances of getting a cck or field plugin so the field plugin is done first. Added the new deprecations and changed the notice to use 8.3.x instead of 8.2.x. Note that there isn't an example for classes, https://www.drupal.org/node/2856615#how-class, so this could be very wrong.

Looks good to me. Not sure about the standards either. But let's see what a committer things. And... drumroll....
This is ready for RTBC again.

Back to the drawing board?

I didn't check every error most there are a significant number of yaml parse errors. I had followed,https://www.drupal.org/node/2856615#how-service, as is but the deprecated can't span lines. So fixed that.
And there was a trailing whitespace error.

+++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateFieldPluginManagerTest.php
@@ -0,0 +1,52 @@
+  /**
+   * {@inheritdoc}
+   */
+  public static $modules = array('system', 'user', 'field', 'migrate_drupal', 'options', 'file', 'text', 'migrate_field_plugin_manager_test');

@inheritoc should be near to the method testPluginSelection() and use short array syntax.

Applying the patch , suggest me comments please,if i am wrong .

Let's sort out those pesky coding standards.

OK @heddn, bring on the RTBC! :)

And back to rtbc

Attempt to update IS with outstanding tasks.

1. +++ b/core/modules/migrate_drupal/src/Annotation/MigrateCckField.php
   @@ -2,53 +2,20 @@
   +removed before Drupal 9.0.x. Use  \Drupal\migrate_drupal\Annotation\MigrateField
   

   two spaces in front of \Drupal - this could have been fixed on commit but there are a couple of other things...

2. +++ b/core/modules/link/src/Plugin/migrate/process/d6/CckLink.php
   @@ -2,85 +2,9 @@
   +class CckLink extends FieldLink { }
   

   Is this not deprecated too?

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php
   @@ -2,131 +2,17 @@
   + * @deprecated in Drupal 8.2.x, to be removed before Drupal 9.0.x. Use
   

   These need to be updated to 8.3.x

On it...

Made all the changes from #167.

I think we want to use trigger_error(), no? See #154 & https://www.drupal.org/node/2575081

I assume you are referring to \Drupal\link\Plugin\migrate\process\d6\CckLink. Should the same apply to \Drupal\migrate_drupal\Plugin\migrate\CckMigration too?

• Added trigger_error() to the two deprecated files that were missing it.
• Corrected a reference to 8.2.x that should be 8.3.x.
• Removed a few redundant spaces.

And back to RTBC.

Queuing up tests, just to be extra careful. I don't want to see this get reverted.

Renamed to make it less of a consideration.

Random failure due to #2860663: UserTimeZoneTest fails on PHP 7.0.x-dev and 7.1.x-dev. But otherwise, tests came back green.

Retests passed, back to RTBC.

This appears to have been set back to NW for no reason whatsoever. I blame Drupal CI.