Problem/Motivation

#2348925: Uninstalling a filter plugin removes text formats introduced an implementation of hook_system_info_alter() that prevents un-installation of modules that provide filter plugins that are being used.

When trying to install a module, that defines a filter, on an already installed site we get an error:

exception 'Drupal\Component\Plugin\Exception\PluginNotFoundException' with message 'The "entity_embed" plugin does not exist.' in [error]
/var/www/drupal8/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php:57

To reproduce:
1. Download entity_embed module (http://drupal.org/project/entity_embed)
2. Install Drupal
3. Install entity_embed module
4. Fatal is thrown

Proposed resolution

filter_formats() statically caches information it provides. It seems that that information is outdated at the time of error. Clearing static cache of filter_formats() seems to fix the problem.

Remaining tasks

- agree on solution
- prepare patch
- review patch

Original report by @Garrett Albright

Commit 86bc343, from #2348925: Uninstalling a filter plugin removes text formats, caused an inadvertent regression where enabling a module that provides a filter causes an exception, even though the module seems to be fully enabled before that happens and the filter can be added to formats later. It appears this is due to the patch wanting to look for information about the filter plug-in before it fully "exists" yet.

CommentFileSizeAuthor
#7 pluginnotfoundexception-2387983-7-tests-only.patch2.33 KBGarrett Albright
#7 pluginnotfoundexception-2387983-7.patch3.52 KBGarrett Albright
#2 filter-format-2387983.pass_.patch2.92 KBlarowlan
#2 filter-format-2387983.fail_.patch2.15 KBlarowlan
cant-enable-filter-module.patch788 bytesGarrett Albright
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Garrett Albright’s picture

Garrett Albright CreditAttribution: Garrett Albright commented
Priority: Major » Critical

Bumping and marking as critical, to be a pest.

Not resolving this issue means that people cannot upgrade to the Drupal 8 versions of Pathologic (49K installs), Linkit (40K installs, depends on Pathologic), WYSIWYG Filter (31K), Markdown (11K), GeSHi (3K), BBCode (3K), and others without crashing their site. We can't ship with that.

It also is currently blocking progress on #2378437: How to put field elements in fieldsets in filter config forms, which would also be good to get into D8 before release, as it's a regression that won't allow some filters (including Pathologic) to have interfaces consistent with their D7 releases.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
FileSize
filter-format-2387983.fail_.patch2.15 KB
filter-format-2387983.pass_.patch2.92 KB

Here's a test

The last submitted patch, 2: filter-format-2387983.fail_.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Assigned: Unassigned » tim.plunkett

Can we have some sort of assertion or at least an explanation of why we are doing that in the test? Otherwise someone might accidentally remove this again.

Also, @timplunkett is probably the most qualified person to review/RTBC this...

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: tim.plunkett » Unassigned
Issue summary: View changes
Priority: Critical » Major
Status: Needs review » Needs work
Issue tags: +Needs issue summary update 
+++ b/core/modules/filter/filter.module
@@ -101,7 +101,8 @@ function filter_system_info_alter(&$info, Extension $file, $type) {
         foreach ($filter_plugins as $filter_plugin) {
-          if ($filter_format->filters($filter_plugin['id'])->status) {
+          $filters = $filter_format->filters();
+          if ($filters->has($filter_plugin['id']) && $filters->get($filter_plugin['id'])->status) {

This change makes sense. Let's move the $filters = $filter_format->filters(); outside this foreach loop.

Also, I agree the change to testFilterForm() needs better docs.

No way that this is critical.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

If we do #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference with the uninstall validate method, then we can convert all these hook_system_info_alter() to use that instead.

As a quick fix this looks fine though - as long as we remove the checks when converting to uninstall validation.

Garrett Albright’s picture

Garrett Albright CreditAttribution: Garrett Albright commented
Status: Needs work » Needs review
FileSize
pluginnotfoundexception-2387983-7.patch3.52 KB
pluginnotfoundexception-2387983-7-tests-only.patch2.33 KB

New patches which take above suggestions into account.

catch, should we add this issue as a related issue to #2278017 with a note to update this one once that one makes it in?

Status: Needs review » Needs work

The last submitted patch, 7: pluginnotfoundexception-2387983-7-tests-only.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

#2392293: Refactor hook_system_info_alter implementations to use ModuleUninstallValidatorInterface just got opened.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
Issue summary: View changes

Updated issue summary with what I figured out as part of #2397393: Fatal if installing a module that defined a filter plugin. Another solution to this seems to be the clear of static cache in filter_formats().

cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
Status: Needs work » Needs review
vitalie’s picture

vitalie CreditAttribution: vitalie commented

patch in #7, pluginnotfoundexception-2387983-7.patch, works for me. Thank you.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
Status: Needs review » Reviewed & tested by the community

Looks good to me and apparently fixes the issue.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented

Looks good to me and apparently fixes the issue.

cilefen’s picture

cilefen CreditAttribution: cilefen commented

This fixed it for me on a manual test.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs issue summary update

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed f562cc9 and pushed to 8.0.x. Thanks!

  • alexpott committed f562cc9 on 8.0.x 
    Issue #2387983 by Garrett Albright, larowlan: PluginNotFoundException...

Status: Fixed » Closed (fixed)

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