Undefined index: text_processing in Drupal\text\Plugin\migrate\field\d7\TextField for D7 sites previously upgraded from D6

Found the right place for testing the issue, no need for adding new test file 😊.

#5 has the test-only patch twice 😶, but not the complete one.

It turned out that HEAD's \Drupal\Tests\text\Unit\Migrate\d7\TextFieldTest is testing the Drupal 6 field plugin \Drupal\text\Plugin\migrate\field\d6\TextField, and not \Drupal\text\Plugin\migrate\field\d7\TextField.

I think that the unit test with the namespace \Drupal\Tests\text\Unit\Plugin\migrate\field\d7\TextFieldTest is now complete.

Since I don't see why we need two separate tests for the Drupal 7 TextField plugin, I removed the one with the namespace \Drupal\Tests\text\Unit\Migrate\d7\TextFieldTest.

It seems that when text_processing is completely missing from the settings, then it equals to having text_processing = 1.

See #3168646: D7 sites upgraded from previous Drupal versions may not have a "text_processing" setting for their body fields.

Can anyone confirm this? I'm not familiar with Drupal 7 (anymore).

Changing component to where the migrate maintainers will see this.

I'd like to understand why the case of text_processing not being set wasn't already handled. Is this a special case in D7? I don't know D7 very well so don't know.

I've only done a cursory look but the fix looks good. I do wonder if the deletion of core/modules/text/tests/src/Unit/Migrate/d7/TextFieldTest.php should be done in a separate issue.

In D7 why is text_processing not set?

+++ b/core/modules/text/src/Plugin/migrate/field/d6/TextField.php
@@ -109,7 +109,7 @@ public function getFieldType(Row $row) {
+      $field_type = !empty($settings['text_processing']) ? 'text' : 'string';

@@ -117,7 +117,7 @@ public function getFieldType(Row $row) {
+      $field_type = !empty($settings['text_processing']) ? 'text_long' : 'string_long';

Can we change these to empty instead of !empty? Just makes is a little easier to read.

Did the condition change as suggested in #19.

Can we change these to empty instead of !empty? Just makes is a little easier to read.

I see that 21 applied that change.

I do wonder if the deletion of core/modules/text/tests/src/Unit/Migrate/d7/TextFieldTest.php should be done in a separate issue.

It seems like it is truly related, so unless some maintainer feel strongly about this, doing it here sounds OK.
If that is the case that change may also like to remove the d7_filter_format references over Drupal\text\Plugin\migrate\field\d6\TextField and Drupal\Tests\text\Unit\Migrate\d6\TextFieldTest.

I was confused at first, on why Drupal\Tests\text\Unit\Migrate\d7\TextFieldTest was being removed.
My first reaction was to address the problem by overriding the relevant methods to set the right plugin class.
The main reason seems to be that there is already another test dealing with the text migrate field plugin unit tests at Drupal\Tests\text\Unit\Plugin\migrate\field\d7\TextFieldTest, which seems more relevant, and it is where the code has been added.

It also brings up the question of unification of approach, and why Drupal\Tests\text\Unit\Migrate\d6\TextFieldTest lives at a different namespace.
A clear hint about it is that the two related classes are mostly the same, see textfieldunittest-migrate-vs-plugin-namespace.patch.txt, the output from diff -up core/modules/text/tests/src/Unit/Migrate/d6/TextFieldTest.php core/modules/text/tests/src/Unit/Plugin/migrate/field/d6/TextFieldTest.php.

I would suggest to either open a follow-up about the duplication of code problem, or address it here, unifying d6 and d7 unit test likely with a test base class.

I am keeping the issue in NR to get an opinion from a migration subsystem maintainer.

I reviewed the field plugins and the tests. It would help to have some comments in the field plugins explaining why text_processing may be empty.

1. +++ b/core/modules/text/src/Plugin/migrate/field/d6/TextField.php
   @@ -109,7 +109,7 @@ public function getFieldType(Row $row) {
   +      $field_type = empty($settings['text_processing']) ? 'string' : 'text';
   

   Let's add a comment here explaining why text_processing is empty.

2. +++ b/core/modules/text/src/Plugin/migrate/field/d7/TextField.php
   @@ -72,8 +72,8 @@ public function getFieldType(Row $row) {
   +      $text_processing = unserialize($instance['data'])['settings']['text_processing'] ?? '0';
   

   Add a comment here as well.

The tests do test what needs to be tested. Which is great but the changes for d6 are in one namespace and for d7 in another. Let's get them all into the correct namespace, which is Drupal\Tests\text\Unit\Plugin\migrate\field.

I have thought more about the deletion of the D7 test and still think it is out of scope and should be done in a followup that deletes both the D6 and D7 test in \Drupal\Tests\text\Unit\Migrate. And with the changes to the tests to be done in the correct namespace that will be conceptually easier.

The deletion of tests should always be questioned, so here is my reasoning. We know we can delete them because the D7 test in that namespace wrongly covers the D6 field plugin and the D6 test there is a functional duplicate of the D6 test in the correct namespace. Proof of that is given in the diff provided by marvil07 in #25. Thanks marvil07!

In regards to the duplicate code and adding a base class, there is no need to take on that work. The problem exists in many migrate tests and the focus on the work remains on functionality and bugs. The exception would be for tests in the migrate module itself.

Setting to NW.

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.