Whever I try the upgrade, it hangs at 38%. Checking the error log, I see multiple entries with the message "Missing filter plugin: filter_null."

I know that the PHP plugin was enabled on the old site, so not sure if it's related to that. I did try enabling the contrib version of the D8 PHP filter, but php still showed up as a missing dependency, and there was no change in the import.

CommentFileSizeAuthor
#42 interdiff-2618332-40-42.txt2.73 KBphenaproxima
#42 2618332-42.patch16.69 KBphenaproxima
#40 interdiff-2618332-36-40.txt11.07 KBphenaproxima
#40 2618332-40.patch17.13 KBphenaproxima
#36 interdiff-2618332-35-36.txt4.16 KBphenaproxima
#36 2618332-36.patch17.38 KBphenaproxima
#35 interdiff-2618332-31-35.txt2.16 KBphenaproxima
#35 2618332-35.patch13.04 KBphenaproxima
#31 interdiff-2618332-27-31.txt4.67 KBphenaproxima
#31 2618332-31.patch12.91 KBphenaproxima
#27 interdiff-2618332-26-27.txt1.71 KBphenaproxima
#27 2618332-27.patch8.93 KBphenaproxima
#26 2618332-26.patch9.09 KBphenaproxima
#25 interdiff-2618332-21-25.txt4.45 KBphenaproxima
#25 2618332-25.patch9.12 KBphenaproxima
#23 filter_null-render-informative-message-2618332-23.patch1.15 KBjonhattan
#21 interdiff-2618332-19-21.txt3.19 KBphenaproxima
#21 2618332-21.patch6 KBphenaproxima
#19 interdiff-2618332-11-19.txt2.36 KBphenaproxima
#19 2618332-19.patch3.32 KBphenaproxima
#11 2618332-11.patch1.05 KBrakesh.gectcr
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

mandclu created an issue. See original summary.

mandclu’s picture

I forgot to mention, the import did show a number of dependency warnings, so this issue may be related to #2532336: migrate-upgrade fails with dependency errors but the error seemed different, which is why I created a new ticket.

mikeryan’s picture

Title: migrate-upgrade hangs on missing filter » Better handle replacement of PHP filter with filter_null
Project: Migrate Upgrade » Drupal core
Version: 8.x-1.x-dev » 8.1.x-dev
Component: Code » migration system

Since Drupal 8 core does not support the PHP filter, migration replaces it with filter_null so that PHP content is filtered out on the front-end. The filter_null plugin, unfortunately, unconditionally generates that message, which isn't particularly helpful. But, apart from the message, things are operating as they should - it's unlikely this would have anything to do with anything hanging.

Moving to the core queue - we should find a way to provide a more helpful message. Since filter_null is designed as a fallback for missing filters, maybe it would be as simple as replacing 'filter_null' with something like 'php_filter_not_supported'...

jonhattan’s picture

Title: Better handle replacement of PHP filter with filter_null » Better handle replacement of missing filters with filter_null
Related issues: +#2630578: Formats duplicated in D6 upgrade

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

Priority: Normal » Major
Issue tags: +Migrate critical
alexpott’s picture

Discussed @xjm, @mikeryan, @penyaskito, and @mlhess. Rather than falling back to filter_null in the migration we should just migrate with the missing plugin ID so the filter will fallback naturally to the filter_null plugin. This will then tell the user what plugin is missing when they edit the filter format. Also we need to make it very clear to the user that does the migration that they need to fix the migrated filter format.

jonhattan’s picture

jonhattan’s picture

jonhattan’s picture

rakesh.gectcr’s picture

Status: Active » Needs review
FileSize
1.05 KB

Applying the #7, finger crossed :D

Status: Needs review » Needs work

The last submitted patch, 11: 2618332-11.patch, failed testing.

heddn’s picture

Seems like valid failures.

+++ b/core/modules/filter/migration_templates/d6_filter_format.yml
@@ -23,7 +23,6 @@ process:
-        default_value: filter_null

D6 filter seems to be missing a flag so the static_map doesn't throw an exception. It might be the cause of the failures.
bypass: true

Expecting (for D6) that the error messaging is going to be any more helpful is probably going to let someone down. The source is a numeric filter ID, which is not very useful. But we can expect to see more useful language for the D7 migration.

mikeryan’s picture

Actually, if we did

    default_value: name

would that give us useful messaging?

mikeryan’s picture

Ignore that, of course that would make the default value the exact string 'name'...

vasi’s picture

Rather than falling back to filter_null in the migration we should just migrate with the missing plugin ID

What does this mean in the context of D6 filters? They don't have a "plugin ID", they're just (module, delta) pairs. I guess we'd just have to come up with a name that sounds reasonable to us?

chipway’s picture

For Drupal 6,
1) Should we try to get the UI label of the Drupal6 filter format from filter_formats table (ex: format id = 3 and name = PHP code)?
This could help the site migrator to find out what was the filter used for in Drupal 6.

2) Could be enhanced by adding the module that provided the filter from filters D6 table (ex: format 3 was orivide by module php).

What do you recommand to do?

chipway’s picture

Patch https://www.drupal.org/files/issues/2618332-11.patch successfully applied, but I have not a right D6 website copy to test it today. I will try later back home.

I am wondering if we should add php module to the map, which could lead to some thing like this in core/modules/filter/migration_templates/d6_filter_format.yml?
map:
filter:
- filter_html
- filter_autop
- filter_url
- filter_htmlcorrector
- filter_html_escape
php:
- filter_php

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
3.32 KB
2.36 KB

Convinced the D6 MigrateFilterFormatTest to pass. Let's see if that has a trickle-down effect on the other failing tests...

Status: Needs review » Needs work

The last submitted patch, 19: 2618332-19.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
6 KB
3.19 KB

Hey...progress! This should address the D7 migration failure. Hopefully it will also fix the other broken tests.

mikeryan’s picture

Status: Needs review » Needs work

The tests pass, now to address the issue - how to provide useful messaging for missing filters?

jonhattan’s picture

@mikeryan: I originally submitted a patch in #2630578-18: Formats duplicated in D6 upgrade. It was resubmitted in other two issues that were closed in favour of this one. At the risk to being repetitive, here's the patch again:

Here's a proposal: return an informative message for admins, instead of the empty string.

We don't have access to the format neither the entity from within the filter class, so the message can't be more friendly --although probably can have a better wording.

jonhattan’s picture

Status: Needs work » Needs review
phenaproxima’s picture

I'm not sure I agree with the approach in #23 -- I fear that we will potentially get mired in core politics if we try to change the filter system itself, and it's not necessary for us to do it anyway. IMHO, it's Migrate that needs to accommodate the filter system, not the other way around.

This patch takes another whack at it. It will almost certainly break the tests, but it adds a new process plugin that takes the time to validate the filter ID after it has been run through the static map. It validates it by actually trying to create an instance of the filter. If that fails, it logs the PluginNotFoundException to the migration, which should at least be a little more informative than the current message -- but it lets invalid values through anyway, since missing filters will automatically fall back to filter_null (at which point the user should deal with the problem).

phenaproxima’s picture

FileSize
9.09 KB

Ugh. I hate how one little typo can eff your entire patch. Same interdiff as in #25.

phenaproxima’s picture

Okay, here's an even better version, based on a correct understanding of how fallback-aware plugin managers work. I don't think this one will pass the tests, but it should behave as we intend.

The last submitted patch, 25: 2618332-25.patch, failed testing.

The last submitted patch, 26: 2618332-26.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 27: 2618332-27.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
12.91 KB
4.67 KB

This should fix the failing Drupal 6 tests.

Status: Needs review » Needs work

The last submitted patch, 31: 2618332-31.patch, failed testing.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

Version: 8.3.x-dev » 8.2.x-dev
phenaproxima’s picture

Status: Needs work » Needs review
FileSize
13.04 KB
2.16 KB

I think this will fix the D7 tests, and also give us the result we want.

phenaproxima’s picture

Behold, a unit test!

mikeryan’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/filter/migration_templates/d6_filter_format.yml
    @@ -16,8 +16,9 @@ process:
    +        # Because the bypass flag is not set, the row will be skipped
    +        # entirely if the filter ID cannot be determined.
    

    That's not true, is it? filter_id should apply the fallback filter in that case? And I don't think we want to throw out the whole format for want of a filter...

  2. +++ b/core/modules/filter/src/Plugin/migrate/process/FilterID.php
    @@ -0,0 +1,81 @@
    +      $message = $this->t('Filter @id could not be mapped to an existing filter plugin; defaulting to @fallback.', [
    

    I don't see any test validating the message.

neclimdul’s picture

So I ran this on a big migration. The migration log was pretty and everything looked like it was going to be great but wait, what is this:

Running d6_filter_format                                                    [ok]
Processed 3 items (0 created, 0 updated, 0 failed, 3 ignored) - done    [status]
  1. +++ b/core/modules/filter/migration_templates/d6_filter_format.yml
    @@ -16,8 +16,9 @@ process:
    -        plugin: static_map
    -        default_value: filter_null
    +        # Because the bypass flag is not set, the row will be skipped
    +        # entirely if the filter ID cannot be determined.
    +        plugin: filter_id
    

    So... this skipped the entire row as documented. I've got a filter that will never exist in d8 applied to all my filter_formats though so now I've got no filters on any of my content and everything is messed up and the entire filter mapping is lost. All I needed was for it to be passed through so I could remove the dead filter manually.

  2. +++ b/core/modules/filter/src/Plugin/migrate/process/FilterID.php
    @@ -0,0 +1,81 @@
    +      $message = $this->t('Filter @id could not be mapped to an existing filter plugin; defaulting to @fallback.', [
    +        '@id' => $value,
    +        '@fallback' => $fallback,
    +      ]);
    +      $migrate_executable->saveMessage($message, MigrationInterface::MESSAGE_WARNING);
    

    Just a notice deep in the message logs? Ouch...

That's pretty rough, I'm not sure I'd say its better.

phenaproxima’s picture

Just a notice deep in the message logs? Ouch...

Yeah, it's not optimal...I'm open to other ideas about how to output the message. I am loath to use drupal_set_message(), that's clunky and intended only for the UI. If only Migrate was using PSR-3...

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
17.13 KB
11.07 KB

Okay, this should address everything except #38.2. Not sure what to do about that.

Status: Needs review » Needs work

The last submitted patch, 40: 2618332-40.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
16.69 KB
2.73 KB

How 'bout now?

mikeryan’s picture

I'm happy with what I see here - @neclimdul, can you run your migration against the latest patch here? If that appears to work fine, it's RTBC as far as I'm concerned.

Thanks!

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community
Running d6_filter_format                                                    [ok]
Missing filter plugin: filter_null.                                      [error]
Missing filter plugin: filter_null.                                      [error]
Missing filter plugin: filter_null.                                      [error]
Processed 3 items (3 created, 0 updated, 0 failed, 0 ignored) - done    [status]

Feedback is still a bit weak but as discussed that's a bigger issue.

Migration ran smooth, after that. I've got my filters and content seems to be correctly using them. The ones that have missing filters are initially broken and render nothing but when you open the filter settings you get "The filter_null filter is missing, and will be removed once this format is saved." Saving it has them working again.

This is a definite improvement so this seems good to go.

jonhattan’s picture

The ones that have missing filters are initially broken and render nothing

#23 address this.

phenaproxima’s picture

@jonhattan: The trouble with #23, at least in the context of this issue, is that it changes a filter plugin, which drags the issue out of Migrate's scope. It also doesn't address the problem of Migrate trying to save formats with non-existent filters -- which, as I discovered during a debugging session the other day, is impossible due to a quirk of the way FilterPluginCollection works.

Don't get me wrong -- I think #23 is definitely worth pursuing. But in a separate, non-Migrate issue.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/filter/src/Plugin/migrate/process/FilterID.php
@@ -0,0 +1,90 @@
+      return $fallback;

Should we return the untransformed value and rely on the filter system here?

This is what happens in \Drupal\filter\Element\ProcessedText::preRenderText()

    // If the requested text format doesn't exist or its disabled, the text
    // cannot be filtered.
    if (!$format || !$format->status()) {
      $message = !$format ? 'Missing text format: %format.' : 'Disabled text format: %format.';
      static::logger('filter')->alert($message, array('%format' => $format_id));
      $element['#markup'] = '';
      return $element;
    }

So the system handles missing formats...

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

We cannot return the unprocessed value, because filter formats cannot be saved with invalid plugin IDs under any circumstances. A quirk of the plugin system, and the way FilterPluginCollection uses it, will not allow it.

Let's say we return the unprocessed value, and it happens to be an invalid plugin ID. Once the row reaches the destination plugin, it will try to save a FilterFormat entity. Here, then, is what happens:

  1. FilterFormat::preSave() will load the FilterPluginCollection in an attempt to sort the filters. (\Drupal\filter\Entity\FilterFormat, line 198)
  2. FilterPluginCollection::sort() calls $this->getAll().
  3. getAll() calls $this->initializePlugin().
  4. initializePlugin() calls getDefinition() on the filter plugin manager -- without setting the $exception_on_invalid flag to FALSE.
  5. getDefinition() throws a PluginNotFoundException which is not caught by initializePlugin() -- although it freaking should be, since the filter plugin manager implements FallbackPluginManagerInterface, but it isn't. The exception bubbles up and halts execution, and the filter format is not saved.

So in summary...because of flaws in the filter system, Migrate has to play defense. That's what's going on here. Back to RTBC!

catch’s picture

Yeah, it's not optimal...I'm open to other ideas about how to output the message. I am loath to use drupal_set_message(), that's clunky and intended only for the UI.

trigger_error() with E_USER_WARNING would work. You get dsm() via the error handler if you run by the UI, drush will report it via cli in red.

phenaproxima’s picture

@catch: Nice idea. My only problem with it is an issue of scope; process plugins should have no business determining how errors are to be displayed. They should just pass things along to a PSR-3 logger class. But since Migrate's error reporting in general is pretty crappy, I figured that the next-best thing was to save a message in the MigrateExecutable.

mikeryan’s picture

The idmap saveMessage() is the API for recording messages during migration - what else is needed? Note that drush migrate-import, when run with -d, will also display them at the console.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed ae25829 to 8.3.x and 0a3ce8b to 8.2.x and 806d956 to 8.1.x. Thanks!

I think we should log in the place the where all migrate info is logged which this patch does.

  • alexpott committed ae25829 on 8.3.x
    Issue #2618332 by phenaproxima, jonhattan, rakesh.gectcr, mikeryan,...

  • alexpott committed 0a3ce8b on 8.2.x
    Issue #2618332 by phenaproxima, jonhattan, rakesh.gectcr, mikeryan,...

  • alexpott committed 806d956 on 8.1.x
    Issue #2618332 by phenaproxima, jonhattan, rakesh.gectcr, mikeryan,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Rodeo.be’s picture

I ran into this issue today .. what is the best approach to resolve it?

G42’s picture

I also have this issue on 8.2.6 with the following error messages:

Missing filter plugin: filter_null.

and

Source ID full_html: Filter media_filter could not be mapped to an existing filter plugin; defaulting to filter_null.

I verified that 8.2.6 has the patches applied by alexpott.

aiphes’s picture

Hi,
Upgrading from a D6, I get this error on filter_null.
The D6 website use module's filter like embed media, core's filter like PHP.
So do I need to do something before upgrading according filters ?
Actually, changing the default format profile doesn't change anything, data are in DB but the front or back office show empty fields for body field...

Thanks