Map all Datetime module's field formatters from D6/D7 to D8/D9

@n4r3n, thanks for the patch.

Since these are king of boilerplate additions I am not so sure this should be done by module. Do you have any idea of how many more modules have formatters that need to be added?

Do you have any idea of how many more modules have formatters that need to be added?

Currently we have no knowledge of more core modules where this needs to happen beyond #3212539: Map all Link module's fieldformatters from D7 to D8/D9 and this one. These were the two that surfaced recently.

--- a/core/modules/migrate_drupal/tests/src/Kernel/d7/FieldDiscoveryTest.php
+++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/FieldDiscoveryTest.php
@@ -266,6 +266,8 @@ public function addAllFieldProcessesAltersData() {
                 ],
                 'datetime' => [
                   'date_default' => 'datetime_default',
+                  'format_interval' => 'datetime_time_ago',
+                  'date_plain' => 'datetime_plain',

This updated the D7 test coverage, but the D6 test coverage still needs to be updated:

…\d6\FieldDiscoveryTest::testAddAllFieldProcessesAlters()

is failing.

@Wim Leers, is there a way to check that? I am not opposed to doing some research.

hi,
Patch #7 applied cleanly on 9.3.x-dev.
thankyou.

@Akhildev.cs, Welcome to Drupal! As a new contributor to Drupal I suggest reading the contribute to Drupal core guide.

+++ b/core/modules/datetime/src/Plugin/migrate/field/DateField.php
@@ -30,6 +30,8 @@ class DateField extends FieldPluginBase {
+      'format_interval' => 'datetime_time_ago',
+      'date_plain' => 'datetime_plain',

I can look in Drupal 7 Date, date.field.inc, and see these formatters. For Drupal 6 I only see default and format_interval, there isn't a date_plain and this plugin is annotated for Drupal 6 and Drupal 7. Since this is a mapping it functionally may not matter but then this patch is also modifying the Drupal 6 tests as if that formatter was available id Drupal 6.

We may need to split the field plugin into a D6 and D7 version.

#6: The only way to do that is to audit both Drupal 7 core and all Drupal 7 contrib modules moved into Drupal 8 core and audit all net-new field formatters & widgets in Drupal 8.

So … that means a lot of manual auditing. Which is also exactly why this is only being discovered so many years later!

#9: Good point!

https://git.drupalcode.org/project/date/-/blob/7.x-2.x/date.field.inc

date_default, format_interval, date_plain

https://git.drupalcode.org/project/date/-/blob/6.x-2.x/date/date.module

default, format_interval

… which means \Drupal\datetime\Plugin\migrate\field\DateField::getFieldWidgetMap() is already wrong in HEAD!

We may need to split the field plugin into a D6 and D7 version.

+1

This apparently has been broken since the day this was added to Drupal core, in #2893072: Modify the DateField Field plugin so it's used for D6 & D7.

@quietone How do you propose we continue here? What you say (which makes sense!) basically means bringing back the D6-specific class that #2893072: Modify the DateField Field plugin so it's used for D6 & D7 deprecated — oh irony! 😅

@Wim Leers, thanks for the history.

Yes, it is a lot of manual auditing. But it is an unavoidable part of a migration, something I see as a responsibility that should be done as part of any migration. Of course, we are human and mistakes will be made.

I think the next step is to do the research on the d6/d7 date field migrations and see where they are the same and where they differ. We need to determine if the methods in \Drupal\migrate_drupal\Plugin\migrate\field\FieldPluginBase work for both. If it just the mappings (getField*Map) then it may be OK to keep one field plugin. If not, they will have to be split again. Make sense?

Irony, indeed!

Yes, it is a lot of manual auditing. But it is an unavoidable part of a migration, something I see as a responsibility that should be done as part of any migration. Of course, we are human and mistakes will be made.

Of course!

But that's why I'm saying I think it's better to do these on a per-discovery basis rather than requiring one or more humans to go scouring the depths of many codebases just to make this issue/patch "as complete as possible" (i.e. cover ALL D9-core-relevant D7 field formatters). If we require that, this is unlikely to ever finish. I'd much prefer more smaller issues that land sooner and help more migrations sooner.

I think the next step is to do the research on the d6/d7 date field migrations and see where they are the same and where they differ. We need to determine if the methods in \Drupal\migrate_drupal\Plugin\migrate\field\FieldPluginBase work for both.

AFAICT that work done as part of #2893072: Modify the DateField Field plugin so it's used for D6 & D7?

If it just the mappings (getField*Map) […]

This is what I strongly suspect. That's also why people are not reporting the "D6 date field to D8|9" migration to be broken. All that we're reporting and fixing in this issue is the formatter mapping.

[…] then it may be OK to keep one field plugin. If not, they will have to be split again. Make sense?

Yes! 😊

IMHO, given that you do think it's okay to keep a single field plugin if the difference is only in getField*Map(), I think we should go ahead with this issue & patch as-is. If there is a pre-existing problem with the migration of date field data in Drupal 6 (as opposed to date field formatters), we'd have heard about it.

Making this issue about that too is a significant expansion in scope.

(Not to mention that the number of remaining Drupal 6 sites has dwindled significantly, so I'd prefer being pragmatic to favor helping the ~600K D7 sites that still need to migrate without slowing it downfor the ~20K D6 sites that remain.)

@Wim Leers, My comments in #13 about research were not meant to hinder the progress here. At the risk of repeating myself I was trying to say that the research into the formatters, widgets etc it best done at the time a new FieldPlugin is added. Of course, that has to be balanced with getting something working for the majority of cases and having something working sooner rather than later.

So, where are we now? The fix here is fine, adding a formatter to a map will not break a D6 migration. I wast just about to RTBC it when I got conservative and would like a comment added. Something along the lines below:

+++ b/core/modules/datetime/src/Plugin/migrate/field/DateField.php
@@ -30,6 +30,8 @@ class DateField extends FieldPluginBase {
     return [

Let's add a comment, before the return line, so we don't forget why this is here. I don't know, something like
// The date_plain formatter exists in Drupal 7 but not Drupal 6. It is added here because this plugin is declared for Drupal 6 and Drupal 7.

Setting to NW for the comment and a reroll.

WFM! :)

Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!