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.
Comment | File | Size | Author |
---|---|---|---|
#7 | pluginnotfoundexception-2387983-7-tests-only.patch | 2.33 KB | Garrett Albright |
#7 | pluginnotfoundexception-2387983-7.patch | 3.52 KB | Garrett Albright |
Comments
Comment #1
Garrett Albright CreditAttribution: Garrett Albright commentedBumping 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.
Comment #2
larowlanHere's a test
Comment #4
BerdirCan 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...
Comment #5
tim.plunkettThis 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.
Comment #6
catchIf 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.
Comment #7
Garrett Albright CreditAttribution: Garrett Albright commentedNew 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?
Comment #9
catch#2392293: Refactor hook_system_info_alter implementations to use ModuleUninstallValidatorInterface just got opened.
Comment #10
slashrsm CreditAttribution: slashrsm commentedUpdated 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()
.Comment #11
cs_shadow CreditAttribution: cs_shadow commentedComment #12
vitalie CreditAttribution: vitalie commentedpatch in #7, pluginnotfoundexception-2387983-7.patch, works for me. Thank you.
Comment #13
slashrsm CreditAttribution: slashrsm commentedLooks good to me and apparently fixes the issue.
Comment #14
slashrsm CreditAttribution: slashrsm commentedLooks good to me and apparently fixes the issue.
Comment #15
cilefen CreditAttribution: cilefen commentedThis fixed it for me on a manual test.
Comment #16
alexpottThis 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!