Migrated filters may have invalid/incomplete settings applied

This issue still has the "Needs tests" tag. If the patch does still need tests, then the status should be NW.

I see a small update to one of the tests. Does that make a difference, or does it just replace Mocks with Prophesies?

The affected code is a migrate process plugin. Even though that code lives in the Filter module, perhaps the Component should be changed to "migration system".

The migrate maintainers work from the 'migration system' component.

+++ b/core/modules/filter/src/Plugin/migrate/process/FilterSettings.php
@@ -36,10 +41,59 @@ class FilterSettings extends ProcessPluginBase {
+    $filter_plugin_definition = $this->filterPluginManager->getDefinition($row->getDestinationProperty('id'));

A process plugin should never look at the destination. Another solution needs to be found for this to go in to core. I think a destination plugin. If there are other config migrations that need to 'merge' data with the existing configuration then maybe changes could be made in \Drupal\migrate\Plugin\migrate\destination\Config with maybe a new configuration key. But I can't think of any config migration that needs such a thing.

Migrate is full of unique cases.

The IS and the comments all refer to Drupal 7 yet this is tagged for Drupal 6 source as well. How does this apply to Drupal 6 sources? Does Drupal 6 have the media_filter as well?

A process plugin should never look at the destination.

It's common to pass in a destination value as part of the source though. It might be clumsy to add the processed id to the source in this case, but maybe we could add a new (optional) configuration parameter to filter_settings. The yml could look like this:

      settings:
        plugin: filter_settings
        source: settings
        default_settings_filter_id: '@id'

The entity_generate plugin in migrate_plus can do something like this. It's actually related to setting default values so it's almost exactly like the situation with filters. You can pass in an array of values and there's a nifty little call to $this->row->get($property) to get the value.

    // Gather any additional properties/fields.
    if (isset($this->configuration['values']) && is_array($this->configuration['values'])) {
      foreach ($this->configuration['values'] as $key => $property) {
        $source_value = $this->row->get($property);
        NestedArray::setValue($entity_values, explode(Row::PROPERTY_SEPARATOR, $key), $source_value, TRUE);
      }
    }

So in our case, assuming we add a config key called default_settings_filter_id, we could just call $this->row->get($this->configuration['default_settings_filter_id']) instead of $row->getDestinationProperty('id')

I also found an instance in core where a process plugin has a hardcoded reference to a destination value. It's in d7_field_instance_settings.

$allowed_values = $row->get('@allowed_values');/code>

Here's a show at a fail test for this issue. This changes a few test cases to expect that filter settings get default values added during migration if necessary. Does this look reasonable?

I see a small update to one of the tests. Does that make a difference, or does it just replace Mocks with Prophesies?

From what I can tell, the test was updated in #4 to account for the change to FilterSettings::__construct.

Those failures look good. Setting back to NR to get a review of the updated test cases. Are those good enough to work against?

This issue came up in the migrate meeting today. I took a closer look at the patch and the filter migration and now see why the process plugin is accessing the destination row. I don't tend to write process plugins like that but that is not the point. The point is that I withdraw my objection about accessing the destination row as sated in #9.

I would still like an answer to my question in #9.

I'm going to post a new patch where the fix from #4 runs against the tests from #14 (except with a whitespace removal reverted and another trivial simple test case added). This is going to fail still, but that's ok.

The new tests case is below. I don't think and interdiff adds much here so I'm not adding one.

+      // Test that a filter value is not overwritten by default.
+      [
+        [
+          'filter_url_length' => 123,
+        ],
+        'filter_url',
+        [
+          'filter_url_length' => 123,
+        ],
+      ],

Removing the use.

That one fails because the $filter_plugin_manager isn't sufficiently mocked. I think at this point, I think the right thing to do is convert this to a Kernel test (it's a Unit test in the Kernel directory!?!?) so that the plugin.manager.filter service exists in full.

After making that conversion to a Kernel test, the cases with any_filter start failing because the plugin manager can't find that filter.

Drupal\Tests\filter\Kernel\Plugin\migrate\process\FilterSettingsTest::testTransform with data set #0 (array(), 'any_filter', array())
Drupal\Component\Plugin\Exception\PluginNotFoundException: The "any_filter" plugin does not exist. Valid plugin IDs for Drupal\filter\FilterPluginManager are: filter_align, filter_autop, filter_caption, filter_html, filter_htmlcorrector, filter_html_escape, filter_html_image_secure, filter_null, filter_url

/var/www/html/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php:53
/var/www/html/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryCachedTrait.php:25
/var/www/html/core/modules/filter/src/Plugin/migrate/process/FilterSettings.php:90
/var/www/html/core/modules/filter/tests/src/Kernel/Plugin/migrate/process/FilterSettingsTest.php:42
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

In real migrations this isn't a problem because $row->getDestinationProperty('id') would return filter_null instead of any_filter. I opted to enable the filter_test_plugin module and replace any_filter with filter_sparkles, which is a dummy plugin provided by that module.

Now we are to interesting test failures. The test cases where we pass in a string fail because of the assert(is_array($value)) that is part of the code block that gets and applies default values.

1) Drupal\Tests\filter\Kernel\Plugin\migrate\process\FilterSettingsTest::testTransform with data set #1 ('a string', 'filter_sparkles', 'a string')
assert(is_array($value)) in /var/www/html/core/modules/filter/src/Plugin/migrate/process/FilterSettings.php:93

What's the deal with passing a string anyway? Is that a real-world scenario? I guess we should try to maintain the existing behavior, which means this still needs work.

Here's me going a little nuts.

I've changed how the process plugin handles strings in the cases there's also default values: it goes with the default values. I also changed an isset to a !empty() to make the first failing test case pass.

I changed the second failing test case because the expectation should be that if a source string gets passed in, that string gets discarded and replaced by default settings if there are defaults settings.

I've also added a case or two, rearranged some cases for clarity, and changed some comments that were no longer accurate in the data provider.

This time I actually expect the tests to pass, FWIW.

Per a suggestion from @benjifisher at the most recent migrate meeting, I'm adding a test case for when the id destination property does not exist. That's a new code path my new test cases did not cover.

I plan to review this issue at DrupalCon Portland.

tags update

It seems to me that there are two open questions. First, how does this issue affect Drupal 6. Second, should we convert the unit test to a kernel test?

From #8:

The IS and the comments all refer to Drupal 7 yet this is tagged for Drupal 6 source as well. How does this apply to Drupal 6 sources? Does Drupal 6 have the media_filter as well?

The media_filter filter is provided by the media module, which has only a dev release for Drupal 6. It is migrated by the media_migration module.

I looked at the core filter_id process plugin. The code comments mention Drupal 7 but not Drupal 6.

So it looks to me as though this issue potentially affects Drupal 6, but I am not aware of any examples where it does. An example would consist of a Drupal 6 filter that has no settings and is migrated to a filter that does have setitngs.

From #23:

That one fails because the $filter_plugin_manager isn’t sufficiently mocked. … at this point, I think the right thing to do is convert this to a Kernel test (it’s a Unit test in the Kernel directory!?!?) so that the plugin.manager.filter service exists in full.

It is certainly odd that we had a unit test in the wrong directory.

My first thought is that I would rather keep this a unit test. I know it is a lot of effort to mock the plugin manager, but that makes the test more self-contained. As a kernel test, we either have to look at the plugin definitions or trust the code comments to see what is being tested. Of course, keeping it a unit test will also make the test run a little faster.

If you agree and switch back to a unit test, then you can remove at least one of the the changes (enabling the test module) added to fix the test. In my opinion, you can move it to the correct directory if you want.

From #25:

Now we are to interesting test failures. The test cases where we pass in a string fail because of the assert(is_array($value)) that is part of the code block that gets and applies default values.

That code was in the original patch (Comment #4). The test did not change between Drupal 9.1.x and 9.4.x, so why didn’t it fail in #4?

I have a few suggestions for cleaning up the patch.

1. Thanks for paying attention to the doc block (and other code comments). But I think the proposed text is confusing. I would use something like “Update settings for migrated filters.” Then, instead of listing the filters that get updated in the doc block, I would have a regular comment at the start of each code block.

    +++ b/core/modules/filter/src/Plugin/migrate/process/FilterSettings.php
    @@ -2,12 +2,17 @@
    ...
     /**
    - * Adds the default allowed attributes to filter_html's allowed_html setting.
    + * Adds default filter settings; updates D7 core filters' default settings.
    + *
    + * By adding the default filter settings,
      *
      * E.g. map '<a>' to '<a href hreflang dir>'.

2. Now that we call getDestinationProperty() three times, is it worth creating a variable for it? Like $destination_id = $row->getDestinationProperty('id').

    @@ -36,10 +41,60 @@ class FilterSettings extends ProcessPluginBase {
    ...
       public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
    +    if (!$row->getDestinationProperty('id')) {
    +      return $value;
    +    }

3. If we keep this as a kernel test, then we should use $filter_plugin_manager = \Drupal::service('plugin.manager.filter');. This is a contentious issue: see #2066993: Use magic methods to sync container property to \Drupal::getContainer in functional tests.

    +++ b/core/modules/filter/tests/src/Kernel/Plugin/migrate/process/FilterSettingsTest.php
    @@ -3,10 +3,9 @@
    ...
       public function testTransform($value, $destination_id, $expected_value) {
    -    $migration = $this->createMock(MigrationInterface::class);
    -    $plugin = new FilterSettings([], 'filter_settings', [], $migration);
    +    $filter_plugin_manager = $this->container->get('plugin.manager.filter');
    +    $plugin = new FilterSettings([], 'filter_settings', [], $filter_plugin_manager);

+ * Adds default filter settings; updates D7 core filters' default settings.

Think this comment should include why it doesn't apply to D6 right?

This is a reroll of #34 for Drupal 10.3

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 Module and Migrate Drupal UI are deprecated and they are not in Drupal 12.0.0.

Issues for these modules should now be on the 11.x branch. And the changes are limited to critical and major bug fixes. Other changes are allowed at the discretion of the core Release Managers in consultation with the Migrate subsystem maintainers.