Migrate text fields with conflicting text_processing setting as formatter text fields

Patch review

The patches here implement idea 1 from the issue summary.

+++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceTest.php
@@ -217,35 +217,21 @@ public function testFieldInstances() {
+    // No text field instances are expected to be skipped.
     $migration = $this->getMigration('d7_field_instance');
     $errors = array_map(function ($message) {
       return $message->message;
     }, iterator_to_array($migration->getIdMap()->getMessages()));
...
+    $this->assertCount(0, $errors);

🥳🥳🥳

Committability review

In order to make this more likely to get committed, I think we need to more clearly articulate why we propose this addition/change, which was apparently chosen not to be done in #2842222: D7 Plain text fields incorrectly migrated to D8 as Text (formatted).

We should update the issue summary with information about the rationale in #2842222 and why we now believe a different rationale is appropriate. Looking at the change record at https://www.drupal.org/node/2906468, it seems that the rationale was that either the settings on the D7 site need to be fixed ("option 1") or a custom migration needs to be developed that splits the field into two ("option 2").

We're introducing an option 3 here. I suspect that to avoid BC, we at minimum need to make this behavior opt-in (using the same $settings['migrate_node_migrate_type_classic'] = FALSE;-style approach that #2746541: Migrate D6 and D7 node revision translations to D8 introduced?).

Conclusion

Marking Needs work only for the committability aspects!

Oops, I missed the screenshots in #5. Updating the issue summary.

This actually addresses most of my Committability review concerns in #7.

Furthermore, I realized that there is in fact no backwards compatibility concern because this issue will not change any existing migrated data, it will ensure that more data gets migrated at all!

Looks like I forgot to set the correct issue status…

Came to review but unfortunately this needs a reroll. Tagging for a reroll and since it looks like the changes to the fixture are OK I am also tagging novice for the reroll.

I did skim the patch and nothing jumped out at me that was problematic.

3 merge conflicts:

1) core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7Test.php

Re-applied delta's from patch to these array elements in method getEntityCounts:
* field_config: +8
* field_storage_config: +4
* node: +2

2) core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldTest.php

Conflict with the new method testDatetimeFields, which was added to class MigrateFieldTest after the #6 patch was created.

3) core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceTest.php

No obvious conflict in hunk, so re-applied modification from patch.

See previous comment.

Closing brace indented properly in MigrateFieldTest.

* Modify count of expected errors for d7_field migration in testDatetimeFields() to match source count (from 5 to 1).
* Modify index in error $messages array accordingly.
* Fixed a typo.

The re-roll looks correct. I examined a diff between #6 and #16, and all the changes make sense to me. Thanks, @tvb! I'm removing the Novice tag since the re-roll is complete.

This still Needs Review though since it wasn't fully reviewed prior to the re-roll.

Adding a related issue #3061681: Provide the ability to map text formats between source and destination. The Problem/Motivation sections are almost identical. Maybe that one could even be closed as a duplicate.

Reroll

Been sitting thinking about the BC aspects of this. We are changing the field type plugin, which does mean that existing migrations using it will be returning one result today and a different result after this patch. That said, it is an improvement in the sense that we are migrating a more accurate representation of data than we were in the past. In general, I'm fine with this, but there are a couple use cases I would like to have addressed here.

First, the one BC issue that I see is that if someone runs the migration set as far as the d7_field migration, then updates drupal to include this patch, and then continues their migration set by running the d7_field_instance migration, they are going to end up getting a field type that doesn't exist on the destination system. (Because it wasn't created before the patch, but after the patch, d7_field_instance won't skip that row anymore and it's going to try). There needs to be some way to check that the field was actually created before the instance migration tries to create an instance, because we can't guarantee that anymore.

Second I'd like to see some consideration for those that want to use migrate_upgrade or such to export most of their core migrations, but would like to actually do the work to split the text fields into separate new fields. Without actually testing through this, this would seem to require exporting and running the config migrations, finding the content types that had string fields and deleting the new long field instances, and replacing them with a new string field instance with a new machine name, and then updating the content type migrations to use the new field. This may be okay if we can't do much to make it easier, but that leads me to the next issue:

Documentation. This needs a CR for sure, the migration documentation needs to be updated, and we should document recommended steps for an advanced developer that wants export and run the core configuration migrations, but also adjust to properly separate the text fields in D9/10.

I'm setting this back to NW for some sort of consideration of the BC implications of running the instance migration with this patch if the field migration was ran before the patch, along with a CR and a documentation update plan.

Also, it looks like a test needs an update that I missed in my Re-roll

Re-roll of #20 for latest 10.1.x updates.

CHanging the issue title here for clarity, as failing to migrate something is not data loss. The data remains right where it's supposed to be.

The patch was no longer applying so I created a MR based on it that would apply. I'm not sure if it should go back to Needs Review after #22

The Migrate Drupal Module was approved for removal in #3371229: [Policy] Migrate Drupal and Migrate Drupal UI after Drupal 7 EOL.

This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

The deprecation work is in #3522602: [meta] Tasks to remove Migrate Drupal module and the removal work in #3522602: [meta] Tasks to remove Migrate Drupal module.

Migrate Drupal will not be moved to a contributed project. It will be removed from core after the Drupal 12.x branch is open.

The Migrate Drupal and Migrate Drupal UI modules are deprecated and will be removed from Drupal 12. For these modules, effort is now focused on bug fixes and necessary tasks. Therefore, this feature request is closed as won't fix.

Thanks to all for working on this!