Uninstalling a module providing display extenders causes fatal errors

Are things ok if you disable the display extender thingy (is disabling something they can do?)?

I ask because the module creating the Display Extender or Views could implement the ModuleUninstallValidator which would prevent the module providing the extender from being uninstalled and explain why.

Oh yeah I think \Drupal\views\Plugin\views\display\DisplayPluginBase::calculateDependencies needs to have some logic around display extenders as well.

Ideally the display extender for itself then would figure out whether it changes the behaviour of a particular view and in that case add itself as module dependency.

Should there be an added 'Disabled' mode that disables all plugins that are provided by a module?

Here is an example.

Added tests exposing the problem

+++ b/core/modules/views/src/Tests/Plugin/DisplayExtenderTest.php
@@ -49,6 +49,17 @@ public function testDisplayExtenders() {
+    ], $view->getDependencies());
+    ¶
+    \Drupal::service('module_installer')->uninstall(array('views_test_data'));

There is some unwanted white space here, which can be fixed in the next patch.

I don't think this is the right solution... I think that we need a views_modules_uninstalled hook implementation that checks if the module being uninstalled provides a display extender and remove it from views.settings::display_extenders and also clean up any views which have configuration for that display extender.

Isn't this fundamentally a question of how we want this handled from the user's perspective? It seems rather "harsh" to essentially break a user's configuration (i.e. remove everything that prevents the module from uninstalling cleanly; [IUUC, this is what catch is suggesting]). Shouldn't this be something the user more consciously decides to do? When you disable a module that is a dependency for other modules, that doesn't disable the dependent modules, it simply prevents the module from being disabled. It is up to the user to change their system in such a way that it is possible to cleanly disable/uninstall the module in a way that is well understood and planned for. I think that rational should ideally be extended to other dependency issues (such as Views Display Extender plugins or input filters).

So, what I wrote in #2?

Well, yes, although I think it's important to first agree on the intended outcome (user must take action to be able to cleanly disable a module, vs. automatically trying to do that cleaning).

With #2 it would never be possible to uninstall a module that provided a display extender because afaik display extenders don't actually have to be configured in a view - they might just act using the views.settings::display_extenders setting.

Do we tell people what happens on the uninstall form for this? If so that counts as giving people a choice for me.

Is there also an issue when configuration is imported and the module/plugin isn't enabled?

I've got a question about the comment #2 which claims we have similar problems with input filter plugins. Drupal\filter\FilterUninstallValidator should prevent this from happening. If we can confirm the input filters are an issue I think we should open an new issue about input filters specifically. I've tried to reproduce this by doing this:

1. Add $settings['extension_discovery_scan_tests'] = TRUE; to settings.php
2. Installed the filter_test_plugin module
3. Added the sparkles plugin to the basic html filter format
4. Visited the module uninstalled page and saw:
   
   

With regards to display extenders I think we should implement a ModuleUninstallValidatorInterface that prevents modules that provide display extenders that are in used from being uninstalled.

#2639336: filter.format.php_code already exists in active configuration may be such a filter issue...

Just a general idea: Allow display extenders to define whether they apply to a display. When they do, we could add it to a list of them on the display and leverage that on runtime and for config dependencies. Here is a rough start of some patch to support it.

Updated issue summary

Updated issue summary.
Working on steps to reproduce.

removing novice task tag from issue summary

What module provides a display extender?

The test module: views_test_data has one.

Discused with @effulgentsia, @xjm and @cottser. This was made critical in #5 due to the claims of an issue with filter plugins in #3. I investigated the claims in #19 and since then no one has replied proving the claims of #3. Therefore I'm downgrading to major since view's display extenders are strange plugins and there is an acceptable work around of just implementing hook_uninstall() in the couple of modules that use them.

If someone does find a critical issue relating to filter plugins please file a new issue.

Here is the reroll.

@alexpott: I just noticed in #19 you referred to #2 but what you meant was #3. But you referenced #3 in subsequent comments, so it pays to keep reading...

@dawehner, @tim.plunkett, @alexpott, @cilefen, and I discussed this issue awhile back and agreed that it is a major bug. The bug causes fatal errors in the user interface and data integrity/uninstallability problems for a fully supported API. As explained in #28, it's not critical because display extenders are, in general, a limited/special usecase.

We recommend that any module that provides a display extender implement hook_uninstall() to require removing any dependencies on it in the meanwhile.

Regarding filter plugins - we have a module that added a filter plugin to try to solve a specific problem. We solved that in another way, and would now like to remove the filter from the module without uninstalling the module. We have disabled all instances of the filter in all text formats, but removing the filter plugin from the module results in broken output wherever a text format is used:

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "product_link_ajax_filter" plugin does not exist. in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 52 of core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).

Happy to open a new issue about this, but really I'd just like to know what we should do in this situation to allow us to remove the unused filter plugin from the module.

@John Pitcairn
Please open its own issue for that. Its a bit unrelated to this one, to be honest. Thank you

It would be nice if ViewsPluginManager implemented FallbackPluginManagerInterface as suggested in the proposed resolution too.

A much nicer developer experience being able to rename plugins on the fly without reinstalling modules ;)

can this be re-rolled for Drupal.8.6.13.

I was using socrata_views which added socrata_views_extender.
It uses hook_uninstall

However uninstalling the module does not solve the issue.

I am on 8.6.13 and the patch does not apply:

git apply 2635728-44.patch
warning: core/modules/views/src/Plugin/views/display/DisplayPluginBase.php has type 100755, expected 100644
error: patch failed: core/modules/views/src/Plugin/views/display/DisplayPluginBase.php:565
error: core/modules/views/src/Plugin/views/display/DisplayPluginBase.php: patch does not apply

patch -p1 < 2635728-44.patch
patching file core/modules/views/config/schema/views.data_types.schema.yml
patching file core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
Hunk #1 succeeded at 464 (offset 1 line).
Hunk #2 FAILED at 566.
1 out of 2 hunks FAILED -- saving rejects to file core/modules/views/src/Plugin/views/display/DisplayPluginBase.php.rej
patching file core/modules/views/src/Plugin/views/display_extender/DisplayExtenderPluginBase.php

I disagree:

$ git checkout 8.6.13
HEAD is now at 89e23ccafb Drupal 8.6.13

$ curl https://www.drupal.org/files/issues/2019-04-09/2635728-44.patch | git apply
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  2291  100  2291    0     0  30268      0 --:--:-- --:--:-- --:--:-- 30546
drupal8x ((8.6.13) *)$ git status
HEAD detached at 8.6.13
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   core/modules/views/config/schema/views.data_types.schema.yml
	modified:   core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
	modified:   core/modules/views/src/Plugin/views/display_extender/DisplayExtenderPluginBase.php

no changes added to commit (use "git add" and/or "git commit -a")

We just hit this problem, if you need to fix your site, here's a quick script, run it with drush scr:

/** @var \Drupal\Core\Config\FileStorage $s */
$s = \Drupal::service('config.storage.sync');
foreach ($s->listAll('views.view.') as $name) {
  $c = $s->read($name);
  foreach (array_keys($c['display']) as $display) {
    unset($c['display'][$display]['display_options']['display_extenders']['simple_sitemap_display_extender']);
  }
  $s->write($name, $c);
}

I manually edited views.settings.yml and then ran drush cim. I will the read the issue later and see whether I can help but wanted to put this out there just in case someone needs help.

Running #44 for the 9.4.x to see if any test failures happen. Confirmed the patch does apply cleanly.

Is there a good way to help test this one?

Think this still needs work. After applying the patch and trying to do a drush cim I get

Invalid data type in config views.data_types.schema, found in file core/modules/views/config/schem
a/views.data_types.schema.yml: Duplicate key "display_extenders" detected at line 261.

There are several hundred sites that need to uninstall the views_ajax_get module in 10.1. It doesn't implement the hook_uninstall() workaround. Bumping to critical since this leaves sites hosed.

This bug prevents Gutenberg module from being uninstalled when using webprofiler module. See #3261040

Triaged as part of BSI. This definitely still seems valid but we need:
1. A reroll onto an MR.
2. Tests
3. An update to the IS as it still mentions filter plugins which seem to not be an issue as per #28

I do not understand, whether is missing todo or not. just with the patch views does not add the dependency via \Drupal\views\Plugin\views\display\DisplayPluginBase::calculateDependencies. It does also not call \Drupal\views\Plugin\views\display_extender\DisplayExtenderPluginBase::applies.

There is a plugin_type display_extender in \Drupal\views\Plugin\views\display\DisplayPluginBase::getAllPlugins, but always NULL, when used in \Drupal\views\Plugin\views\display\DisplayPluginBase::calculateDependencies.

I believe we need something like #2426607: Calculates and adds dependencies of views display extender, which use \Drupal\views\Plugin\views\display_extender\DisplayExtenderPluginBase::applies.

Ok, as mention in #2426607: Calculates and adds dependencies of views display extender a dependency is not a option because this would delete the view.

Therefore -> views_modules_uninstalled clean-up the config and we will verify that the plugin does exits before we use it.

The clean up in the view display itself is done by the next save of the display.

I evaluated two solutions for the issue.

1. Modify/cleanup the views in hook_modules_uninstalled hook
2. Implement a new module uninstall validator for the display handler plugins Comment #19

Modify/cleanup the views in hook_modules_uninstalled hook
- I feel this might not work in this case.
Reason is, that we can't fetch the list of all the display handler plugins provided by the module being uninstalled.
In the uninstall() function, this hook is the last one to get called https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

Since plugin ID or anything won't be available here, we can't identify which display handler plugins we want to unset from views config.

I evaluated another option to check the plugin by doing namespace-based lookup, however, that seemed a bit hacky to me.

Considering that, it might not be straightforward to update all the views and unset the display handler plugins.
We can certainly disable the plugin in views > Advanced config settings.

Implement a new module uninstall validator for the display handler plugins Comment #19
This worked fine and prevented module being uninstalled.
I added on validator plugin along with kernel test to validate the exception.
This seems to be working fine.

I feel at this point we should go ahead with the approach where we display a warning rather than silently updating the views config.

Once we have the consensus around the solution, I can update the issue summary with the necessary details.

Left some comments on the MR.

Attributing according to triage list in #28