Image field migration plugin in wrong module

I think this should deal with the requested changes in the issue. I have not been able to find anywhere in the code base where this file is used.

There would be a slight BC break if anybody extends that class, but not sure if our policy covers that.

Ah, that's a good BC approach @heddn, good call.

Drupal 6 user pictures are disabled by default and sites actively need to enable it, so should not be a fixed requirement for user migrations.

  $picture_support = variable_get('user_pictures', 0);
  $form['pictures']['user_pictures'] = array(
    '#type' => 'radios',
    '#title' => t('Picture support'),
    '#default_value' => $picture_support,
    '#options' => array(t('Disabled'), t('Enabled')),
    '#prefix' => '<div class="user-admin-picture-radios">',
    '#suffix' => '</div>',
  );

Updated the tests.

With this imagefield will display as missing.

I was working on 8.5.x, let's try that again.

I'm unable to replicate the test failures locally, but I have identified a minor change that is required.

I am also running the patch in #10 against 8.5.x on the assumption it will suffer very similar failures.

+++ b/core/modules/file/src/Plugin/migrate/cckfield/d7/ImageField.php
@@ -2,42 +2,16 @@
-@trigger_error('ImageField is deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.x. Use \Drupal\file\Plugin\migrate\field\d7\ImageField instead.', E_USER_DEPRECATED);
+@trigger_error('ImageField is deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.x. Use \Drupal\image\Plugin\migrate\field\d7\ImageField instead.', E_USER_DEPRECATED);
...
  * @deprecated in Drupal 8.3.x, to be removed before Drupal 9.0.x. Use

In other places, "8.3.x" was changed to "8.5.x". Shouldn't these be changed too?

Correct the destination modules in MigrationProvidersExistTest and renamed a directory from cck to cckfield.

Still need some tests to address the concerns in #7.

There are two items in #7

1. tests
2. add a module dependency to d6_user_picture_file.yml on the image module

The second was answered in #8, a module dependency can't be added. That leaves the tests.

I'm a bit unclear on the test, it seems to be a test to ensure that this field plugin, image, is in the correct module, that is the module where the image field type is defined. Is that correct? If that is correct, than the test should cover all field plugins and not be limited to any particular one. And then should that be done here or in a followup? Or, do I have it all wrong?

Asked on IRC and learned I got that all wrong

heddn> quietone: one thing to consider is that if you have a d6 fixture that has a profile photo. and on d8 the image module is disabled, then we 1) should error the user migration 2) we should show that image module isn't enabled on d8.
<heddn> quietone: and that means that imagefield or whatever they call it should show up in the missing upgrade paths. that currently doesn't happen. our current d6 test enables image so we don't surface this problem.
<phenaproxima> quietone: I think the way to test this is to simply use those deprecated classes and instantiate them, and ensure that they do, indeed, throw deprecation notices. No?

2) we should show that image module isn't enabled on d8.

I take this to mean a test of review page when running from the UI, /upgrade. If so, we could use the new test base class, MigrateUpgradeReviewPageTestBase, from #2914974: Migrate UI - handle sources that do not need an upgrade.

This adds a test of image field migration for d6 and a fail test.

36.1 Fixed
36.2 Without menu_ui there are schema errors.

1) Drupal\Tests\image\Kernel\Migrate\d6\MigrateImageTest::testNode
Schema errors for node.type.article with the following errors: node.type.article:third_party_settings.menu_ui.available_menus missing schema, node.type.article:third_party_settings.menu_ui.parent missing schema (/opt/sites/d8/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:95)

I think it is worth noting that a MigrateImageTest for d7 is not added because the d7 MIgrateNodeTest already includes assertions on a image field.

Yes, lets solve the user profile problem in another issue and keep this about moving the image field plugins.

The followup has been created, #2944966: User profile migration should send a warning if image module is disabled

There is an existing change record, Migrate Image CCK and Field plugins moved to image module, which looks good to me, so we are all done here.

Looks good -- I mean, it's incredibly confusing, as these "move X to another module" issues always are -- but it seems legit to me. The only strange element is MigrateImageTest.

1. +++ b/core/modules/image/tests/src/Kernel/Migrate/d6/MigrateImageTest.php
   @@ -0,0 +1,48 @@
   +/**
   + * Image migration test.
   + *
   + * @group migrate_drupal_6
   + */
   +class MigrateImageTest extends MigrateNodeTestBase {
   

   Can this doc comment be expanded? It doesn't explain what the purpose of the test is, or why it's extending MigrateNodeTestBase, or...you get the picture. I think we need to explain the context a bit more.

2. +++ b/core/modules/image/tests/src/Kernel/Migrate/d6/MigrateImageTest.php
   @@ -0,0 +1,48 @@
   +    $node = Node::load(9);
   +    // Test the file field meta.
   

   I find this comment a bit odd.

@heddn, thanks for making the patch. It looks good, but I don't think I can RTBC since I added the test.

+1 RTBC

My comments are addressed. Looks legit.

1. +++ b/core/modules/file/src/Plugin/migrate/cckfield/d7/ImageField.php
   @@ -2,42 +2,16 @@
   -@trigger_error('ImageField is deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.x. Use \Drupal\file\Plugin\migrate\field\d7\ImageField instead.', E_USER_DEPRECATED);
   +@trigger_error('ImageField is deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.x. Use \Drupal\image\Plugin\migrate\field\d7\ImageField instead.', E_USER_DEPRECATED);
   

   Let's add a link to the CR here too... i know this is an existing issue.

2. +++ b/core/modules/file/src/Plugin/migrate/field/d6/ImageField.php
   @@ -2,12 +2,16 @@
   +@trigger_error('ImageField is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.x. Use \Drupal\image\Plugin\migrate\field\d6\ImageField instead.', E_USER_DEPRECATED);
   
   +++ b/core/modules/file/src/Plugin/migrate/field/d7/ImageField.php
   @@ -2,35 +2,16 @@
   +@trigger_error('ImageField is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.x. Use \Drupal\image\Plugin\migrate\field\d7\ImageField instead.', E_USER_DEPRECATED);
   

   This should also link to the CR as per policy.

3. +++ b/core/modules/file/tests/src/Unit/Plugin/migrate/field/d7/ImageFieldTest.php
   @@ -10,6 +10,7 @@
   + * @group legacy
   

   There should be a corresponding * @expectedDeprecation ImageField is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.x. Use \Drupal\image\Plugin\migrate\field\d7\ImageField instead. on the test - this message will need the CR added too.

   Also I think we should have a corresponding test for Legacy test for D6 just to show it works

• Add links to the CR in "cckfield/d7/ImageField.php", "field/d6/ImageField.php" and "field/d7/ImageField.php".
• Add @expectedDeprecation to field/d7/ImageFieldTest.php.
• Create field/d6/ImageFieldTest.php.

+++ b/core/modules/file/src/Plugin/migrate/cckfield/d7/ImageField.php
@@ -2,42 +2,16 @@
+use Drupal\image\Plugin\migrate\cckfield\d7\ImageField as LegacyImageField;
...
+class ImageField extends LegacyImageField {}

+++ b/core/modules/file/src/Plugin/migrate/field/d6/ImageField.php
@@ -2,12 +2,16 @@
+use Drupal\image\Plugin\migrate\field\d6\ImageField as NonLegacyImageField;
...
+class ImageField extends NonLegacyImageField{}

+++ b/core/modules/file/src/Plugin/migrate/field/d7/ImageField.php
@@ -2,35 +2,16 @@
+use Drupal\image\Plugin\migrate\field\d7\ImageField as NonLegacyImageField;
...
+class ImageField extends NonLegacyImageField{}

We can't do this. We should leave the legacy code in place and not extend from the non-legacy. This is because image module might not be installed so we can't extend from its classes.

Adding credit. I confirmed with @heddn in slack that existing migrate plus migrations will pick up the new plugins.

Committed and pushed 544740df31 to 8.6.x and 50f4ceae89 to 8.5.x. Thanks!

Discussed with @catch and we agreed as a migrate_drupal patch which is beta this should be backported to 8.5.x so it's in 8.5.0.

diff --git a/core/modules/file/src/Plugin/migrate/field/d6/ImageField.php b/core/modules/file/src/Plugin/migrate/field/d6/ImageField.php
index 07b0d3fd20..c05d8c3665 100644
--- a/core/modules/file/src/Plugin/migrate/field/d6/ImageField.php
+++ b/core/modules/file/src/Plugin/migrate/field/d6/ImageField.php
@@ -14,4 +14,4 @@
  *
  * @see https://www.drupal.org/node/2936061
  */
-class ImageField extends NonLegacyImageField{}
+class ImageField extends NonLegacyImageField {}
diff --git a/core/modules/file/src/Plugin/migrate/field/d7/ImageField.php b/core/modules/file/src/Plugin/migrate/field/d7/ImageField.php
index d411a9708b..29da4d2b67 100644
--- a/core/modules/file/src/Plugin/migrate/field/d7/ImageField.php
+++ b/core/modules/file/src/Plugin/migrate/field/d7/ImageField.php
@@ -14,4 +14,4 @@
  *
  * @see https://www.drupal.org/node/2936061
  */
-class ImageField extends NonLegacyImageField{}
+class ImageField extends NonLegacyImageField {}

Fixed coding standards on commit.