Convert remaining widget and formatter type migrations to MigrateField plugins

Just did one more review, left a suggestion that should make it easier for next reviewers.

That being said, I 100% agree with

Now imho this is ready for being reviewed by a maintainer.

(from almost a month ago)

Great work @huzooka - just wanted to say THANK YOU for your migrate patches work done. Just ran into several of the issues you created patches on. Also of course thank you for your reviews @Wim! Confirming #11. Let's now wait for SSM review :)

I started to review this today but have abandoned due to my frustration with Gitlab ignoring my clicks and not able to select multiple lines by dragging.

Overall, the patch looks good and nothing leaps out as a problem.

The title is very accurate and yet I'm changing it. Starting with 'convert' since that is what this is doing and then removing corresponding because to me that implies that the MigrateField plugin already exists, but in many cases they are being added.

I'll try to review again at the end of the week.

@huzooka, thanks for the correction. How about 'remaining'

Did you have more thoughts here, @quietone? :)

No, this fell off my list. But now, the MR is out of date, I can't apply it locally as I like to do.

git fetch
git rebase origin/9.3.x
git push -f drupal-3204212

… and apparently this causes "293 pushed commits" above, https://git.drupalcode.org/project/drupal/-/merge_requests/440/commits to say there's 141 commits when there's really still the same 5 and testbot to immediately conclude it's not mergeable.

No information about this at https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa... sadly 😔

OMG!

Per https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/738 this is supported.

But on d.o, it looks like only the original creator of the merge request can edit the target branch 😬 For example, for https://git.drupalcode.org/project/ckeditor5/-/merge_requests/17 I have the "Edit" button, but here, for https://git.drupalcode.org/project/drupal/-/merge_requests/440, I do not.

@huzooka, can you please go to https://git.drupalcode.org/project/drupal/-/merge_requests/440/edit and change the target branch? 🙏

IMHO it makes no sense that drupal.org automatically changed the target version from 9.2.x-dev to 9.3.x-dev in #12 but did not change the target branch of the MR. 😔

But hey, okay, sticking to 9.2.x then… rebasing again onto the latest 9.2.x instead! 🤞

Here's a reroll of #10, now for 9.2.0-rc1.

Came back to review. I have started to write a response several times and deleted it several times. I just don't know where to start. Trying again.

It is very frustrating for all of us that the rebase to 9.3.x failed. This will need to be on 9.3.x, not 9.2.0-rc1.

Not wanting to hold this up I applied that patch locally and then jumped to Gitlab to look at the unresolved issues. Looking at those and the applied patch I see the my comments, such as this, https://git.drupalcode.org/project/drupal/-/merge_requests/440#note_23224, haven't had a response yet.

I then read the patch.

1. +++ b/core/modules/datetime/src/Plugin/migrate/field/DateField.php
   @@ -30,6 +31,10 @@ class DateField extends FieldPluginBase {
   +      // Drupal 6.
   +      // @see ::getFieldFormatterType
   

   This is kinda cryptic. I don't remember, are both 'default' and 'format_interval' Drupal 6 only? I don't know, maybe, 'The following are for Drupal 6 only.'?

2. +++ b/core/modules/datetime/src/Plugin/migrate/field/DateField.php
   @@ -96,4 +104,19 @@ public function defineValueProcessPipeline(MigrationInterface $migration, $field
   +      // The Drupal 6 date formatter ID might be the machine name of the date
   +      // format, e.g. 'short', 'medium', 'long', or any other custom format.
   

   I prefer the comment before the 'if' but there doesn't seem to be a coding standard for this. I will take it as a lesson in being flexible.

3. +++ b/core/modules/field/migrations/d6_field_formatter_settings.yml
   @@ -65,114 +65,7 @@ process:
   +        map: { }
   
   +++ b/core/modules/field/migrations/d6_field_instance_widget_settings.yml
   @@ -46,21 +45,7 @@ process:
   +      map: { }
   
   +++ b/core/modules/field/migrations/d7_field_instance_widget_settings.yml
   @@ -55,18 +55,7 @@ process:
   +      map: { }
   

   Part of me likes the removal of all this to code and another part likes seeing it in the yml. Overall, consistency wins and having D6 and D7 migrations and code the same, as much as possible, is preferred.

4. +++ b/core/modules/field/src/Plugin/migrate/field/Email.php
   @@ -42,6 +42,11 @@ public function getFieldFormatterMap() {
   +      // Drupal 6.
   

   Let's expand this one too. 'The following formatters are Drupal 6 only.' or some such.

5. +++ b/core/modules/image/image.module
   @@ -480,3 +481,17 @@ function image_field_config_delete(FieldConfigInterface $field) {
   + * Implements hook_migrate_field_info_alter().
   
   +++ b/core/modules/taxonomy/src/Plugin/migrate/field/TaxonomyTermReference.php
   @@ -45,4 +45,13 @@ public function defineValueProcessPipeline(MigrationInterface $migration, $field
   diff --git a/core/modules/telephone/src/Plugin/migrate/field/d6/PhoneField.php b/core/modules/telephone/src/Plugin/migrate/field/d6/PhoneField.php
   

   This is new to me. I have never seen a migrate_field_info_alter hook, nor can I find anything about it. Where to look?

That is all I can manage now.

I made a rebase mistake:

PHPUnit\Framework\Exception: PHP Fatal error:  Cannot redeclare Drupal\file\Plugin\migrate\field\d7\FileField::getFieldWidgetMap() in /Users/wim.leers/Work/d8/core/modules/file/src/Plugin/migrate/field/d7/FileField.php on line 47

#3051252: Upgrade path for Multiupload Filefield Widget and Multiupload Imagefield Widget already added such a method :)

… and that also happens to explain the 124 failures. Probably all of them. Hopefully. 🤞

#25.5 This is undocumented by Drupal core. See:

• \Drupal\migrate_drupal\Plugin\MigrateFieldPluginManager(), which is class MigrateFieldPluginManager extends MigratePluginManager implements MigrateFieldPluginManagerInterface {
• And it inherits the constructor of its parent class:
  

    public function __construct($type, \Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler, $annotation = 'Drupal\Component\Annotation\PluginID') {
      parent::__construct("Plugin/migrate/$type", $namespaces, $module_handler, NULL, $annotation);
      $this->alterInfo('migrate_' . $type . '_info');
      $this->setCacheBackend($cache_backend, 'migrate_plugins_' . $type);
    }
  

• So the key thing here is $type, which the constructor receives from the service definition in core/modules/migrate_drupal/migrate_drupal.services.yml:
  

    plugin.manager.migrate.field:
      class: Drupal\migrate_drupal\Plugin\MigrateFieldPluginManager
      arguments:
        - field
        - '@container.namespaces'
        - '@cache.discovery'
        - '@module_handler'
        - '\Drupal\migrate_drupal\Annotation\MigrateField'
  

#2683435: CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins introduced it.

Aaand #3213616: Map all Datetime module's field formatters from D6/D7 to D8/D9 just landed which forces the need for another rebase here. 7850074a is finally actually running tests 🥳

@quietone On the bright side: this can be easily rebased between 9.2.x and 9.3.x — the only real problem here is the fact that Drupal.org's GitLab merge request refuses to let me test the same merge request against multiple branches. That is a huge regression compared to the patch-based workflow IMHO 😬

Since we can't change the target branch … I propose to be pragmatic: let the target branch continue to be 9.2.x, keep pushing commits there, and just upload a test explicitly for testing against 9.3.x 😅 Did that here. This matches exactly what is in the merge request :)

@Wim Leers, thank you for making a 9.3.x patch.

Everything in #25 has been address. And I have been given a lesson in a plugin hook, \Drupal\Core\Plugin\DefaultPluginManager::alterInfo. Thanks again Wim Leers.

+++ b/core/modules/file/src/Plugin/migrate/field/d6/FileField.php
@@ -24,6 +24,7 @@ class FileField extends FieldPluginBase {
+      'imagefield_widget' => 'file_generic',

Not sure about this. According to https://git.drupalcode.org/project/drupal/-/blob/9.3.x/core/modules/imag... the default widget is image_image, and a mapping for this widget is already provided in \Drupal\image\Plugin\migrate\field\d6\FileField.

#28: Very sharp observation! @huzooka, why did you add that?

Despite the late hour I decided to read #30. I did and I do need to sleep on it. Even so, this feels like it is pushing the scope from a 'conversion' to 'conversion and improvements to the d6 field plugins'. If the latter is true then can we keep this just to conversion and move the improvements to a followup?

Cheers

#32 is waiting for a response from @quietone, making that clearer 🤞

OK. I think I have time and energy to look at this again.

1. +++ b/core/modules/datetime/src/Plugin/migrate/field/DateField.php
   @@ -34,6 +35,10 @@ public function getFieldFormatterMap() {
   + // The date formatter exists in Drupal 6 but not Drupal 7. See
   

   Shouldn't this be 'The default formatter' instead of 'The date
   formatter'?

2. +++ b/core/modules/datetime/src/Plugin/migrate/field/DateField.php
   @@ -34,6 +35,10 @@ public function getFieldFormatterMap() {
   + 'default' => 'datetime_default',
   @@ -100,4 +108,19 @@ public function
   defineValueProcessPipeline(MigrationInterface $migration, $field
   + public function getFieldFormatterType(Row $row) {
   

   These appear to be adding new functionality. I still think that it
   should be in a separate issue. This method is also not tested or even
   executed in the existing tests, d6\MigrateFieldWidgetSettingsTest.php
   and d6\MigrateFieldFormatterSettingsTest.php or Upgrade6Test.

3. +++b/core/modules/field/migrations/d6_field_instance_widget_settings.yml
   @@ -46,21 +45,7 @@ process:
   + map: { }
   

   I saw a comment from Wim Leers above suggesting comments be added to explain
   these empty maps and I agree.

4. +++ b/core/modules/file/src/Plugin/migrate/field/d6/FileField.php
   @@ -24,6 +24,7 @@ class FileField extends FieldPluginBase {
   + 'imagefield_widget' => 'file_generic',
   +++
   b/core/modules/migrate_drupal/src/Plugin/migrate/field/FieldPluginBase.php
   @@ -81,12 +85,19 @@ public function getFieldWidgetMap() {
   + $source_field_types = !empty($plugin_definition['type_map'])
   + ? array_keys($plugin_definition['type_map'])
   + : [$plugin_id];
   + foreach ($source_field_types as $source_field_type) {
   

   I am able to run existing tests without this change. Why is it needed?

I still want to see the conversion of existing functionality separated out from the additional functionality and a separate one for the file migration (I think). If that were done I think it would be easier to find regressions, not harder. It would allow us to identify what needs a new test.

There is also a personal aspect, I want to see these changes committed and if this was split up it I would be encouraged to review the issues promptly.

Forgot I got assigned to this.

#27 no more applies against 9.3.x and needs a reroll though, I guess.

@kostyashupenko, can you provide an interdiff? There are instructions for creating an interdiff . If you think a diff is not needed, just add a comment stating why.

Rerolled fix only patch for 9.3.x

which path should I use for 9.3.3 ?
#39 oder #41

Rerolled fix only patch for 9.4.x

@quietone, I tried interdiffing #27 with #44 and #27 with #38 using patchutils like the documentation suggested here. But can't seem to create the interdiff. Is it even possible to create interdiffs between 2 patches that use different Versions of Drupal?

EDIT: Also tried it using interdiff -iwbB to ignore basically everything, still the hunks do not match...

Just tried to create the interdiff between #27 and #38:
interdiff 3204212-field-migration-widget-formatter-mapping-27.patch 3204212-38.patch > interdiff-27-38.txt

1 out of 6 hunks FAILED -- saving rejects to file /tmp/interdiff-1.fSOElD.rej
interdiff: Error applying patch1 to reconstructed file

The result can be seen below, but as @kostyashupenko never replied what #38 is about and if it's just a reroll against 9.3.x (I think so!) or solving the comments from #35 (and I don't think it does!).

So I'd suggest to proceed with the MR (against 9.2.x see #27) using the review from #35.

@huzooka are you planning to work on this again here?

The fix-only patches can be rerolled against current Drupal Core versions by anyone who needs the fix NOW. :)

Addressed #35.1 and #35.3. Sadly for the rest I'm not experienced enough here. :(

This no longer applies to 9.5.x. Will work on that unless somebody beats me to it 🤓

Thank you @Wim Leers that would be a true christmas present to have this fixed. I'm honestly missing experience in the #35.1 and #35.3 parts as written above.

Straight reroll of #44 — just needed a small conflict resolution caused by #3112452: Fix indentation consistency in core's yaml files..

Fix #44's cspell failures.

Reroll for 10.0.0. Necessary due to #3290810: Remove updates added prior to 9.4.0 (9.4.4 for ckeditor) and add 9.4.0 database dumps.

@Wim Leers: Do you have permission to change the target branch of the MR to 10.1.x? I sadly don't.
That would make the (interdiff) changes a lot more transparent.

As of #50 who might have the knowledge to fix #35.1 and #35.3 finally?
We're heavily relying on this patch plus #3202462: [PP-1] Provide option for contrib modules to map their D6 / D7 field formatter and widget plugin IDs to the equivalent D9 plugin ID. The situation is really messy :(

Drupal migrations are such a pain due to these issues. I'm sure we're losing a lot of people in this process. (No criticism, we're a community. And this is open source. Just saying.)

Patch #52 doesn't apply on 9.5.10. 9.5.7 works just fine.

re-roll Patch #52

It is too late for this issue to be fixed for D9, so I am re-rolling the patch from #53 for 11.x. The conflicts come from #3000717: Missing mapping for "nodereference_url" widget.

I have not done anything to fix the tests, but I am setting the status to NR in order to trigger a new test.

In #54, @Anybody pointed out that there are outstanding points from the review in #35. I am adding that to the issue summary (remaining tasks).

This is still blocking #3202462: [PP-1] Provide option for contrib modules to map their D6 / D7 field formatter and widget plugin IDs to the equivalent D9 plugin ID and #3108302: [PP-2] Field formatter & widget settings: fall back to default if missing plugin.

Without the infrastructure added here + those 2 issues, it is IMPOSSIBLE for contrib modules providing field formatters and widgets to provide a D7 → D9/10/11 migration 😬

I guess most people just manually migrate those settings and hence this has never been a priority for migration system contributors?

I know that #35 requested this to be split up more. But it's already been split up in 3 parts.

https://www.drupal.org/project/acquia_migrate has been running with these patches for years now. And we've contributed many patches to contrib modules that build on top of these. Clearly many others are interested in it too 😊 I'm hoping somebody else can push the remaining pieces forward 🤞

#54: I don't either 😭 Only core committers can do that!

@quietone can you please change the target branch to 11.x? 🙏

#35:

1. I think you're right! 😄
2. This is not new infrastructure. This API was added in #2906203: Widgets/formatters: D7 Plain text fields incorrectly migrated to D8 as Text (formatted) to FieldPluginBase, in 2017. This is just fixing the existing migration in core.
3. 👍
4. Probably because it's necessary for #3202462: [PP-1] Provide option for contrib modules to map their D6 / D7 field formatter and widget plugin IDs to the equivalent D9 plugin ID or #3108302: [PP-2] Field formatter & widget settings: fall back to default if missing plugin or both. If we can get by here without these changes, then let's drop them! Confusingly, the comment is being reworded but is effectively still saying the same thing. Which seems to indicate you're right! 😄

@quietone can you please change the target branch to 11.x? 🙏

(#61)
Ping :)

Patch from #58 doesn't apply anymore to current 11.x. Here is the new reroll.

Not working much with patches... I can't seem to retest the latest patch with 11.x? Latest option is 10.1.x.