Convert Migrate's cckfield plugins to use the new field plugins

Waiting for the cckfield rename to get committed.

Now that #2683435: CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins is set to "Patch (to be ported)" does that mean we can un-postpone this? If so, what is the best way to approach this? I suspect one big patch could become unwieldy so suggest a separate ticket for each field type. Thoughts?

@Jo Fitzgerald, good questions. I have the same ones.
But nothing like jumping in with both feet. Here is a patch that should have all the remaining changes.

Retesting with 8.4.x

I'll start by fixing a couple of minor coding standards errors before addressing the test fails.

Removed assertions in MigrateCckFieldPluginManagerTest about "cckfields" that no longer exist (because they have been converted to "fields") leaving only assertions about fields provided by the migrate_cckfield_plugin_manager_test module.

@Jo Fitzgerald, thanks. And let's put those tests into MigrateFieldPluginManagerTest, as well as add tests for link. Found a usage of a cck plugin in FieldFileTest, changed it so d6_file.

Also, discovered that MigrateFieldPluginManagerTest loads the file module but then doesn't test the file module plugins, it tests file from it's test module. It could use a little tidy up, which is outside the scope of this issue.

This now contains all the remaining conversions. I understand that I started a patch without getting an answer to Jo Fitzgerald's question (#5) about breaking this into smaller pieces. Its just that my curiosity got the better of me and I wanted to see just how big the patch would be.

So, what is the next step? Should this be divided up? If so, how?

+++ b/core/modules/file/src/Plugin/migrate/field/d6/FileField.php
@@ -1,18 +1,18 @@
- * @MigrateCckField(
+ * @MigrateField(
  *   id = "filefield",
  *   core = {6}

+++ b/core/modules/file/src/Plugin/migrate/process/d6/CckFile.php
@@ -2,91 +2,9 @@
  * @MigrateProcessPlugin(
  *   id = "d6_cck_file"
  * )

+++ b/core/modules/file/src/Plugin/migrate/process/d6/FieldFile.php
@@ -0,0 +1,92 @@
+ * @MigrateProcessPlugin(
+ *   id = "d6_field_file"
+ * )

In D6 it actually was called CCK not "Field", right? Even for Filefield.

Yes, D6 used Cck terminolgy and D7 (and onward) uses fields.

OK, so why are the D6 plugins being renamed in this patch? Sorry if I'm missing something.

The MigrateCckField plugin started out just doing the D6 field plugins. But now it is used both D6 Cck and D7 Fields. It is equally odd to use a MigrateCckField plugin for D7 fields. And then there is 'text', which is for used for both D6 and D7. What name makes sense?

 * @MigrateCckField(
 *   id = "text",
 *   type_map = {
 *     "text" = "text",
 *     "text_long" = "text_long",
 *     "text_with_summary" = "text_with_summary"
 *   },
 *   core = {6,7}
 * )

Since Cck is a term from contrib in D6 and fields is in core since D7, making them all Fields was the way to go.

Another way to look at it - for both D6 and D7, the things being migrated are called fields. "CCK" is just the name of the D6 module (and just the human-readable name at that - the actual module machine name is "content").

I think #17 is answered - restoring RTBC.

Modified the title since we're not just touching .yml files here.

Sorry for not following up, totally forgot. Yeah, if this is an explicit decision that the Migrate maintainers are on board with then I don't want to hold this up.

To clarify (from my understanding) - we've renamed field plugin classes (as opposed to making copies), so if anyone has extended their own field plugins from those classes, they'll break. The "won't fix" arguments are:

1. This is Drupal-to-Drupal-specific stuff, so it's still alpha-experimental - we are allowed to break BC.
2. We don't know of anyone actually doing this (extending field plugins) in the wild. Note that the link field plugin was renamed in the original patch four weeks ago, and no one has appeared to notice.

+++ b/core/modules/file/tests/src/Unit/Plugin/migrate/process/d6/FieldFileTest.php
@@ -0,0 +1,51 @@
diff --git a/core/modules/link/src/Plugin/migrate/cckfield/LinkField.php b/core/modules/link/src/Plugin/migrate/cckfield/LinkField.php
new file mode 100644

Why is this adding in a cckfield plugin?

@quietone: A previous issue renamed cckfield/LinkField to field/LinkField—but this breaks BC if anybody extends LinkField. Rather than revert the previous issue, we're trying to fix it here by re-adding a cckfield/LinkField.

@vasi, thanks. Just got myself all confused for a while there.

So, building on the work of heddn, this patch uses the changes made for the text cckfield/field tests as a model for all the other cckfield/field pairs of plugins. There should be a deprecation notice in all the cckfield classes.

There are two known failing tests. /cckfield/d7/LinkCckFieldTest.php and /field/d7/LinkFieldTest.php. They need the constants defined in system.module. What is the proper way to do that?

And lastly, expect copy/paste errors. This got a bit tedious.

I can't figure out why those system.module constants aren't available. I tried changing it to a kernel test, and even explicitly adding "system" to the modules property. The other core tests that use these constants, extend WebTestBase and BrowserTestBase, which is total overkill for this.

I read through the changes and they LOOK good to me. But this is a huge patch. I would trust a machine much more than my own eyes. I know I would have committed after each step, so I could see the diffs for "moved file X" and "modified file X" separately.... @quietone did you do something like that locally? Or is this flat diff basically the only representation we have to work with?

No, sorry. I did this in one go. :-( However, in the interest of getting this done and some learning on my part let's start again.

The first thing is to make a new patch for #27. Why? Because 'diff 2833206-27.patch 2833206-27.patch produces 101 lines of output under "diff -u b/core/modules/file/src/Plugin/migrate/process/d6/FieldFile.php b/core/modules/file/src/Plugin/migrate/process/d6/CckFile.php'. And is just confusing. Making a new patch with '-M' to fix that. Should have the same errors as #27.

Now fix the errors. Just some missing lines in the data provider.

There are cckfield/field plugins in file, link, taxonomy and text. The test for text are already complete, and passing tests. That leaves file, link and taxonomy.

This patch add tests for the cckfield and field plugins in the file module.

Now add tests for cckfield and field plugins in the taxonomy module and fix coding standard errors found in testing.

Sigh, various copy/paste errors I missed because I didn't run some tests.

Next, add tests to the link module.

Right so, these errors are the same as way back in patch #31.

This patch adds deprecation notices on all the cckfield plugin classes and removes some extra spaces on a line d7/LinkCckTestphp.

That is as expected and there are no new errors. The next addition is to move the two tests in the text module. This will put them in the same tree structure as the other tests.

@heddn, thanks. I've added @legacy to the remaining tests. However, still getting errors on link/tests/src/Kernel/Plugin/migrate/cckfield/d7/LinkCckTest.php. Apparently, @legacy has no effect for KTB?

Missed an @group, and add @group legacy to file/tests/src/Kernel/Migrate/process/d6/CckFileTest.php.

Test passing now, setting to NR.

With following git config settings:

diff.renames=copies
diff.renamelimit=1500

and to reduce the patch size just reroll the patch.

Assigning for review

Nice work. This is a big patch. I'll certainly miss some things, but here is what I found so far:

1.

+++ b/core/modules/file/src/Plugin/migrate/cckfield/d6/FileField.php
@@ -2,6 +2,10 @@
+@trigger_error('FileField is deprecated in Drupal 8.4.x and will be
+be removed before Drupal 9.0.x. Use \Drupal\file\Plugin\migrate\field\d6\FileField
+instead.', E_USER_DEPRECATED);

+++ b/core/modules/file/src/Plugin/migrate/process/d6/CckFile.php
@@ -2,91 +2,17 @@
+@trigger_error('CckFile is deprecated in Drupal 8.3.x and will be
+be removed before Drupal 9.0.x. Use \Drupal\file\Plugin\migrate\process\d6\FieldFile
+instead.', E_USER_DEPRECATED);

... and other places.
Sometimes it's 8.3.x and other times it's 8.4.x, I think it should be 8.4.x everywhere.

2.

+++ b/core/modules/file/src/Plugin/migrate/process/d6/CckFile.php
@@ -2,91 +2,17 @@
+@trigger_error('CckFile is deprecated in Drupal 8.3.x and will be
+be removed before Drupal 9.0.x. Use \Drupal\file\Plugin\migrate\process\d6\FieldFile
+instead.', E_USER_DEPRECATED);
+
 
 /**
  * @MigrateProcessPlugin(
  *   id = "d6_cck_file"
  * )

Extra line.

3.

+++ b/core/modules/file/src/Plugin/migrate/cckfield/d6/FileField.php
@@ -2,6 +2,10 @@
+@trigger_error('FileField is deprecated in Drupal 8.4.x and will be
+be removed before Drupal 9.0.x. Use \Drupal\file\Plugin\migrate\field\d6\FileField

+++ b/core/modules/file/src/Plugin/migrate/cckfield/d7/FileField.php
@@ -2,6 +2,10 @@
+@trigger_error('FileField is deprecated in Drupal 8.4.x and will be
+be removed before Drupal 9.0.x. Use \Drupal\file\Plugin\migrate\field\d7\FileField

+++ b/core/modules/file/src/Plugin/migrate/cckfield/d7/ImageField.php
@@ -2,6 +2,10 @@
+@trigger_error('ImageField is deprecated in Drupal 8.4.x and will be
+be removed before Drupal 9.0.x. Use \Drupal\file\Plugin\migrate\field\d7\ImageField

... and other places.
Extra 'be', and this will also fix the 80 characters limit.

4.

+++ b/core/modules/file/src/Plugin/migrate/process/d6/CckFile.php
@@ -2,91 +2,17 @@
-class CckFile extends ProcessPluginBase implements ContainerFactoryPluginInterface {
...
+class CckFile extends FieldFile {}

Why this is the only class that we empty and make extend the new class? All the other ones are just copied.

5.

+++ b/core/modules/file/src/Plugin/migrate/cckfield/d6/FileField.php
@@ -2,6 +2,10 @@
+@trigger_error('FileField is deprecated in Drupal 8.4.x and will be
+be removed before Drupal 9.0.x. Use \Drupal\file\Plugin\migrate\field\d6\FileField
+instead.', E_USER_DEPRECATED);
+

+++ b/core/modules/file/src/Plugin/migrate/process/d6/CckFile.php
@@ -2,91 +2,17 @@
+@trigger_error('CckFile is deprecated in Drupal 8.3.x and will be
+be removed before Drupal 9.0.x. Use \Drupal\file\Plugin\migrate\process\d6\FieldFile
+instead.', E_USER_DEPRECATED);
+
 
 /**
  * @MigrateProcessPlugin(
  *   id = "d6_cck_file"
  * )
+ *
+ *  @deprecated in Drupal 8.3.x, to be removed before Drupal 9.0.x. Use
+ * \Drupal\file\Plugin\migrate\process\d6\FieldFile instead.

Why do we sometimes only add the @trigger_error, and other times we add @trigger_error and @deprecated?

6.

+++ b/core/modules/text/src/Plugin/migrate/field/TextField.php
@@ -1,13 +1,13 @@
-namespace Drupal\text\Plugin\migrate\cckfield;
+namespace Drupal\text\Plugin\migrate\field;
 
 use Drupal\migrate\Plugin\MigrationInterface;
 use Drupal\migrate\Row;
-use Drupal\migrate_drupal\Plugin\migrate\cckfield\CckFieldPluginBase;
+use Drupal\migrate_drupal\Plugin\migrate\field\FieldPluginBase;
 
 /**
- * @MigrateCckField(
+ * @MigrateField(
  *   id = "text",
  *   type_map = {
  *     "text" = "text",
@@ -17,7 +17,7 @@

@@ -17,7 +17,7 @@
  *   core = {6,7}
  * )
  */
-class TextField extends CckFieldPluginBase {
+class TextField extends FieldPluginBase {

I know that in #2842222: D7 Plain text fields incorrectly migrated to D8 as Text (formatted) we will need a dedicated D7 TextField plugin. Should we already put this one in the d6 namespace, thus avoiding us to make another copy and have another deprecated class, or will it be ok to just leave it here and just add the new one in the d7 namespace?

7.
After this patch gets in, there will still be some reference to cck in some other places (NodeDerivers, TaxonomyTermDerivers, FieldType process plugin, etc.). This patch being big as it is, I guess we should have a follow up to see where we could and can clean things up.

1. All deprecation notices should now be using 8.4
2. Fixed
3. Fixed
4. TODO
5. Added missing class deprecation notices to deprecated classes.
6. TODO
7. TODO

Thanks quietone & heddn, I'll review this more extensively tonight, but in the meantime, there's still some extra 'be' left and 80 characters limit left:

+++ b/core/modules/file/src/Plugin/migrate/process/d6/CckFile.php
@@ -2,91 +2,16 @@
+@trigger_error('CckFile is deprecated in Drupal 8.4.x and will be
+be removed before Drupal 9.0.x. Use \Drupal\file\Plugin\migrate\process\d6\FieldFile

+++ b/core/modules/link/src/Plugin/migrate/cckfield/LinkField.php
@@ -0,0 +1,55 @@
+@trigger_error('LinkField is deprecated in Drupal 8.4.x and will be
+be removed before Drupal 9.0.x. Use \Drupal\link\Plugin\migrate\field\d6\LinkField

+++ b/core/modules/link/src/Plugin/migrate/cckfield/d7/LinkField.php
@@ -0,0 +1,55 @@
+@trigger_error('LinkField is deprecated in Drupal 8.4.x and will be
+be removed before Drupal 9.0.x. Use \Drupal\link\Plugin\migrate\field\d7\LinkField

This should fix the text tests. Made a d6 and d7 version. I've added a diff and not an interdiff because interdiff output makes no sense at all.

Conflicting with this RTBC issue currently: #2896507: Update FieldPluginBase with a default processFieldValues() and getFieldFormatterMap()

I'ma fix those broken tests.

This should fix the broken test.

Again, this should fix the broken tests.

Oups, missed one.

Related CR: https://www.drupal.org/node/2896537

Agreed, RTBC +1

Sorry I missed the comment in #80, would have been better to post it on that issue maybe. Sending for re-test.

Re-rolled.

re-RTBC'd. I diffed the diffs to ensure things didn't change in any significant way.

Sorry, I miss-read that diff change I thought it was removed hunks from an existing deleted file, not a missing deleted file. Here's the change to bring it back to a reroll of #79.

Needed the following in my git to get it to look like @maxocub's.

diff.renames=copies
diff.renamelimit=1500

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!