Undefined index: allowed_html FilterSettings.php:45

May be this could help.

Can we get a test that triggers the notice?

+++ b/core/modules/filter/src/Plugin/migrate/process/FilterSettings.php
@@ -42,7 +42,9 @@ class FilterSettings extends ProcessPluginBase {
+	  if (!empty($value['allowed_html'])) {
+		$value['allowed_html'] = str_replace(array_keys($this->allowedHtmlDefaultAttributes), array_values($this->allowedHtmlDefaultAttributes), $value['allowed_html']);
+	  }

Patch contains tab characters - should be spaces.

Updated patch with no tabs.

I am getting this error with a source D6 that is a fresh install of Ubercart 6, with a store and a couple of products. Made an brief attempt at a test but then, I couldn't generate the error again.

Tracked this to FilterFormat::prepareRow(). When the deita is 0, the $settings array is made from several variables, allowed_html_$format, filter_html_help_$format, and filter_html_nofollow_$format. In the failure case, when $format=1, all those variables do not exist. Then, in the process plugin, trying to write to any index on $settings fails, causing the error. I don't know how those variable got deleted when building an Ubercart test site.

So, making a test would require deleting those variables from the test database before running the test. That seems excessive for a change that we could argue should be made in of itself, that is the process should check that it is safe to write to the array before doing so.

Marking NR, to get feedback on the need for a test.

Just caught this, too. Running commerce_migrate tests caused a failure due to this notice.

So, making a test would require deleting those variables from the test database before running the test. That seems excessive for a change that we could argue should be made in of itself, that is the process should check that it is safe to write to the array before doing so.

Sanity-checking inputs is something that should be done in and of itself, yes, and if we were purely sanity-checking for the sake of it I might agree we don't need tests. But, given that this is something multiple people have seen in real life, I do think we should test for it.

OK, guess I'm just feeling lazy.
Here is a test and the fix.

Looks good, thanks!

For what it's worth, I realize that I am the cause of this bug. I tried to trim up the original commerce_migrate test fixtures to piggy back off of core's, and quietone had re-rolled them for me. So I did create a one of a kind possible scenario.

The root causes of the DB inconsistency are unknown.

I'm all for throwing exceptions and failing early. Perhaps we should throw an exception about an unexpected change from what a default Drupal 6 database would contain? Or, work around it as proposed.

The root causes of the DB inconsistency are unknown.

Not so sure about that. I just did a fresh install of D6. And in the variable table there is no entry for allowed_html. So, the process plugin really should check that the array key exists before trying to write to it.

MariaDB [uc6]> select * from variable where name like '%html%';
+---------------+-------+
| name          | value |
+---------------+-------+
| filter_html_1 | i:1;  |
+---------------+-------+
1 row in set (0.01 sec)

Looks good other than minor code style things, but especially a comment needs updating:

1. +++ b/core/modules/filter/tests/src/Kernel/Plugin/migrate/process/FilterSettingsTest.php
   @@ -0,0 +1,102 @@
   +  /**
   +   * Tests transformation of image field settings.
   

   Hm, not image field tests, right?

2. +++ b/core/modules/filter/tests/src/Kernel/Plugin/migrate/process/FilterSettingsTest.php
   @@ -0,0 +1,102 @@
   +      // Test with an empty source array
   ...
   +      // Test with an empty source array
   

   Minor: Period at end of line/sentence missing.

Both fixed.

+++ b/core/modules/filter/tests/src/Kernel/Plugin/migrate/process/FilterSettingsTest.php
@@ -0,0 +1,102 @@
+ * Unit tests of the filter_id plugin.

It's filter_settings, not filter_id. :) Otherwise, this looks fine to me. RTBC once this is fixed.

Changes made as per comment #23 & also added interdiff.

Yep, think this is good to go.

Looks good now, thanks! Great to see the added test coverage, woot :)