Uninstalling a filter plugin removes text formats

Drupal warns you:

We have an API for this now, see ConfigEntityInterface::onDependencyRemoval() and existing implemetations for it, most notably in EntityDisplayBase.

Fully agreed that this is a major if not critical bug, it should actually never be possible to delete a filter format.

Definitely critical. Nice find.

This is fine except it doesn't work. ;-)

Does not work how, does it still remove the filter?

Filter format uses EntityWithPluginCollectionInterface, so the dependencies are calculated based on getPluginCollections() -> filters(), which has a cache. I'm not sure if we re-load the objects or not.

I also don't know if those methods even care about the status?

This still does not work. removeFilter() is called in this situation but the code to remove the filter is wrong.

I added a test that should not pass because the patch doesn't work. But, it passes

So the KernelTestBase::uninstallModule() does not actually fire module uninstall code - it is a special flower.
Also filter formats are a special flower too since the filter plugin collection will always return all available filter plugins since disabled configuration is preserved - unless it matches the default state - see \Drupal\filter\FilterPluginCollection::getConfiguration().

@alexpott Thank you. Manual testing with #12 works for me. +1 for RTBC

WOW!!!!!!

Definitely critical. Nice find.

+1

1. +++ b/core/modules/filter/src/Tests/FilterAPITest.php
   @@ -386,4 +386,27 @@ public function assertFilterFormatViolation(ConstraintViolationListInterface $vi
   +    $filter_format->setFilterConfig('filter_test_restrict_tags_and_attributes', $filter_config);
   +    $filter_format->save();
   

   Can be chained?

2. +++ b/core/modules/filter/src/Tests/FilterAPITest.php
   @@ -386,4 +386,27 @@ public function assertFilterFormatViolation(ConstraintViolationListInterface $vi
   +    \Drupal::entityManager()->getStorage('filter_format')->resetCache();
   

   I'd think that this would happen automatically, since uninstalling modules should invalidate the extension cache tag. But I'm guessing this is for the static cache, which doesn't use cache tags?

3. +++ b/core/modules/filter/src/Tests/FilterAPITest.php
   @@ -386,4 +386,27 @@ public function assertFilterFormatViolation(ConstraintViolationListInterface $vi
   +  }
    }
   

   Missing trailing newline.

Tnaks for the review @Wim Leers - re #15

1. Fixed
2. Yes it is for the static cache - we're in a KernelTestBase test too
3. Fixed

Adding comments to clarify further.

Sorry, if I'm missing something, but at the very least this was not discussed here, so...

I don't see how this is even remotely viable. The specific configuration and order of filters in a text format is highly security relevant and it is in no case whatsoever safe to just willy-nilly delete some of them without explicit user confirmation.

If you uninstall PHP filter module for example, you will potentially reveal highly sensitive information to the world.

Surely the current solution of deleting the formats isn't good, but at least it hides the text wholesale, because check_markup() can't find the format. This patch makes things dramatically worse.

The correct solution is to disallow uninstalling any module which provides a filter to an existing text format.

The correct solution is to disallow uninstalling any module which provides a filter to an existing text format.

This was my first thought also! :)

But consider this: you just installed a text filter. You gave it a try. You dislike it. You want to remove it. With what you propose, you can't.

Why is this warning not sufficient:

?

That is not sufficient because quite simply we are putting the site in a potentially insecure state. And the user might be aware of that but she or he might not. A warning like that is nothing more than an excuse, when it comes to security. And it's a poor one too, because it is not at all explicit. Maybe I am completely aware of the security implications but I think that Drupal will simply delete all content which uses the given format, so I just go ahead. Who knows what

updated if possible, or deleted

means?!

So I think @tstoeckler is correct. We need to implement hook_system_info_alter in the filter module to not allow a module to be uninstalled if it provides a filter format plugin that is enabled in any filter format configuration entity.

This is not quite it (it makes all filter-providing modules required), but it's a start.

Hang with #23 and #24. I was wrong. #21 now thinking about it - surely this is an existing issue. In Drupal 7 you can uninstall a module that provides the equivalent of a filter format plugin so this is an existing issue. I think the patch in #17 is good to go. We can fix this issue in the old issue created for it #562694: Text formats should throw an error and refuse to process if a plugin goes missing (which I have a patch for).

Uploading the patch in #17 again to make it the latest and bring the issue back to being in scope.

Re #26: Ok, I agree that that issue is the one we need to fix properly. I still agree with this going in in the meantime, though. As stated above, the current behavior of deleting text formats is pretty bad, too, but at least it makes sure that text never gets filtered incorrectly, because check_markup() bails out in that case. I don't see why we should be purposely making things worse and introducing a known Security flaw just because we have another issue to fix that properly.

@tstoeckler This is fixing a regression. Without #27, you have a broken site. #27 is making things better, not worse. Let's focus attention on #562694: Text formats should throw an error and refuse to process if a plugin goes missing after this goes in.

@cilefen: Well it might be broken, but at least it is not insecure. In that tradeoff we should always choose security and I would consider it a terrible decision to commit this without #562694: Text formats should throw an error and refuse to process if a plugin goes missing but @alexpott is right that D7 suffers from similar behavior, so... In any case it's not up to me :-)

This patch does not make anything less secure. Also I think we're confusing filter plugins with filter formats. Filter plugins are provided be modules and can be configured to be part of a filter format. HEAD's current behaviour is totally ridiculous. If a module provides a filter plugin all filter formats will depend it on. When that module is uninstalled ALL filter formats will be deleted. This fixes that.

Separately we should address the issue that if a module provides a filter plugin and it is used in any filter format then we need to prevent that module being uninstalled. That is exactly what #562694: Text formats should throw an error and refuse to process if a plugin goes missing is (I've made this critical). I've made critical and will have a patch the moment this one is committed. There is a separate issue around what occurs when a whole filter format is removed and that WORKS - a missing filter format results in no output.

Given @tstoeckler says:

I still agree with this going in in the meantime, though

I'm re-rtbcing.

Sorry, that was a really weird typo. I meant disagree.

And just want to clarify that this does make things less secure as explained above.

Okey-dokey I'm not going to hold this up anymore especially since I've already done the work. Here is a patch that prevents uninstalling a module that provides a filter plugin that is currently used in any filter format configuration entity.

fwiw I completely agree with tstoeckler here.

One case where we could delete the filter format entirely is when it's not used at all in any content, but that doesn't really seem compatible with the hook_system_info_alter() approach.

I really don't like all these issues being hanged from hook_system_info_alter ; it will make any rebuild quite slow and brittle . We are working on an uninstall in #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference

If we can add a validation step to module uninstall and present the information then that would save doing it in hook_system_info_alter() - is worth a side issue to add that. I don't want to hold any of these issues up on that though - we can convert later using the same information and it'll be an API addition rather than change.

We'll need to make sure at least drush also respects this though (as it does required modules).

ModuleHandler::uninstall() doesn't check if a module is required - if it did and we implement this extra step there then Drush would haven't to do anything too special. +1 to not holding this up.

Patch fixes test for new module_installer service.

Now for the patch

Tagging with the upgrade path as the current HEAD could get a site into an inconsistent state.

Reviewed the patch, tested it manually, stepped through the test coverage, tried to remove parts that I didn't understand at first sight/that didn't seem to be doing anything, hence gaining a full understanding. I couldn't break it. RTBC.

1. +++ b/core/modules/filter/filter.module
   @@ -83,6 +84,38 @@ function filter_theme() {
   +  if ($type == 'module' && !defined('MAINTENANCE_MODE')) {
   

   Theoretically we'd want to exclude the filter module from getting this treatment, because that'd imply a nonsensical recursive dependency. However, it also has real-world — though rare — repercussions: it makes it impossible to uninstall the Filter module as long as any text format (FilterFormat) config entity is
   still using any filter plugin. To uninstall the Filter module, you'd have to disable every filter plugin on every text format.

   I'd almost say that this is a feature: it makes it exceedingly annoying/painful to remove the Filter system, and that's a good thing :)

2. +++ b/core/modules/filter/src/Entity/FilterFormat.php
   @@ -387,4 +388,43 @@ public function getHtmlRestrictions() {
   +      // Only remove disabled filters.
   

   Nitpick: clearer comment would be "Remove disabled filters, so that this FilterFormat config entity can continue to exist."

Awesome, thanks @alexpott for pushing this. This is a great solution to a non-trivial problem!

I think the issue has diverged from the summary a bit, from the patch we seem to be making two changes:

1. Mark modules that provide plugins required if the filter plugin is used in a filter format.

2. Remove filter plugins from formats if the plugin provider is uninstalled, but only if the filter plugin is disabled.

I'm confused why if we do #1 how we'd ever get to #2 though?

Minor nits below:

1. +++ b/core/modules/filter/filter.module
   @@ -83,6 +84,38 @@ function filter_theme() {
   +      foreach(filter_formats() as $filter_format) {
   +        foreach($filter_plugins as $filter_plugin) {
   

   Minor and can fix on commit but missing space after foreach x 2.

2. +++ b/core/modules/filter/src/Tests/FilterAPITest.php
   @@ -386,4 +387,64 @@ public function assertFilterFormatViolation(ConstraintViolationListInterface $vi
   +   */
   

   Not sure this comment describes what the test does enough.

   What about:

   "Ensure that modules providing filter plugins are required when the plugin is in use, and that only disabled plugins are removed from format configuration entities rather than the configuration entities being deleted."

   Although if that's the correct description I'm still not sure if the behaviour is right.

Updated issue summary.

I'm confused why if we do #1 how we'd ever get to #2 though?

This is because disabled filters are still stored in text formats if they have different settings from their defaults.

re #43

1. Indeed filter module will be exceptionally hard to uninstall
2. Fixed

re #45

1. Fixed
2. Fixed

Tentatively setting back to rtbc since the changes are very small.

OK that makes sense. Not storing anything for disabled filters is I think for #2312379: Bring filter format plugin configuration into line with other systems, I'd forgotten we'd not resolved that completely yet.

Commit 8b6c343 has made it so that one gets a PluginNotFoundException when enabling a module which provides a filter. Try the D8 branch of Pathologic, for example. (Despite the exception when the module is enabled, if you later go to the text format configuration form, the filter is available…)

I'm trying to look into it to discover why, but might not succeed before my bedtime.

Slate ~/Sites/d8.test/www> drush en pathologic -y
The following extensions will be enabled: pathologic
Do you really want to continue? (y/n): y
exception 'Drupal\Component\Plugin\Exception\PluginNotFoundException' with message 'The "filter_pathologic" plugin does not exist.' in                                 [error]
/Users/Albright/Sites/d8.test/www/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php:57
Stack trace:
#0 /Users/Albright/Sites/d8.test/www/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryCachedTrait.php(30):
Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition(Array, 'filter_patholog...', true)
#1 /Users/Albright/Sites/d8.test/www/core/modules/filter/src/FilterPluginCollection.php(75):
Drupal\Core\Plugin\DefaultPluginManager->getDefinition('filter_patholog...')
#2 /Users/Albright/Sites/d8.test/www/core/lib/Drupal/Component/Plugin/LazyPluginCollection.php(85):
Drupal\filter\FilterPluginCollection->initializePlugin('filter_patholog...')
#3 /Users/Albright/Sites/d8.test/www/core/modules/filter/src/FilterPluginCollection.php(31): Drupal\Component\Plugin\LazyPluginCollection->get('filter_patholog...')
#4 /Users/Albright/Sites/d8.test/www/core/modules/filter/src/Entity/FilterFormat.php(138): Drupal\filter\FilterPluginCollection->get('filter_patholog...')
#5 /Users/Albright/Sites/d8.test/www/core/modules/filter/filter.module(104): Drupal\filter\Entity\FilterFormat->filters('filter_patholog...')
#6 /Users/Albright/Sites/d8.test/www/core/lib/Drupal/Core/Extension/ModuleHandler.php(496): filter_system_info_alter(Array, Object(Drupal\Core\Extension\Extension),
'module')
#7 /Users/Albright/Sites/d8.test/www/core/modules/system/system.module(913): Drupal\Core\Extension\ModuleHandler->alter('system_info', Array,
Object(Drupal\Core\Extension\Extension), 'module')
#8 /Users/Albright/Sites/d8.test/www/core/modules/system/system.module(970): _system_rebuild_module_data()
#9 /Users/Albright/Sites/d8.test/www/core/modules/user/src/PermissionHandler.php(219): system_rebuild_module_data()
#10 /Users/Albright/Sites/d8.test/www/core/modules/user/src/PermissionHandler.php(205): Drupal\user\PermissionHandler->systemRebuildModuleData()
#11 /Users/Albright/Sites/d8.test/www/core/modules/user/src/PermissionHandler.php(184): Drupal\user\PermissionHandler->getModuleNames()
#12 /Users/Albright/Sites/d8.test/www/core/modules/user/src/PermissionHandler.php(90): Drupal\user\PermissionHandler->sortPermissions(Array)
#13 /Users/Albright/Sites/d8.test/www/core/modules/user/user.module(1374): Drupal\user\PermissionHandler->getPermissions()
#14 [internal function]: user_modules_installed(Array)
#15 /Users/Albright/Sites/d8.test/www/core/lib/Drupal/Core/Extension/ModuleHandler.php(396): call_user_func_array('user_modules_in...', Array)
#16 /Users/Albright/Sites/d8.test/www/core/lib/Drupal/Core/Extension/ModuleInstaller.php(264): Drupal\Core\Extension\ModuleHandler->invokeAll('modules_install...',
Array)
#17 /Users/Albright/.composer/vendor/drush/drush/commands/core/drupal/environment.inc(129): Drupal\Core\Extension\ModuleInstaller->install(Array, true)
#18 /Users/Albright/.composer/vendor/drush/drush/commands/core/drupal/environment.inc(196): drush_module_install(Array)
#19 /Users/Albright/.composer/vendor/drush/drush/commands/pm/pm.drush.inc(1120): drush_module_enable(Array)
#20 [internal function]: drush_pm_enable('pathologic')
#21 /Users/Albright/.composer/vendor/drush/drush/includes/command.inc(359): call_user_func_array('drush_pm_enable', Array)
#22 /Users/Albright/.composer/vendor/drush/drush/includes/command.inc(210): _drush_invoke_hooks(Array, Array)
#23 [internal function]: drush_command('pathologic')
#24 /Users/Albright/.composer/vendor/drush/drush/includes/command.inc(178): call_user_func_array('drush_command', Array)
#25 /Users/Albright/.composer/vendor/drush/drush/lib/Drush/Boot/DrupalBoot.php(46): drush_dispatch(Array)
#26 /Users/Albright/.composer/vendor/drush/drush/drush.php(76): Drush\Boot\DrupalBoot->bootstrap_and_dispatch()
#27 /Users/Albright/.composer/vendor/drush/drush/drush.php(16): drush_main()
#28 {main}
Slate ~/Sites/d8.test/www> 

Looks like filter_system_info_alter() is trying to get info about a plugin which doesn't fully exist yet when a module is enabled.

Garrett: please open a new issue and link to this one. It's no longer accepted in core to reopen already committed issues. It's much easier for everyone to have a new issue. It's fine to link to the new one from the fixed one though!

I seems this introduced another bug: #2397393: Fatal if installing a module that defined a filter plugin