Problem/Motivation

A view that uses a view display extender does not get a config dependency on the module that provides the display extender plugin.

This means that the view may be broken (best case) or cause a WSOD (#2635728: Uninstalling a module providing display extenders causes fatal errors) if the module is uninstalled.

Steps to reproduce

  1. Have a view with a display using a display extender from a module that's not views, e.g. metatag (Code) or views_inject (Code)
  2. Uninstall the providing module (metatag, views_inject, ...)
  3. Notice: The view remains unchanged and can trigger a WSOD

Expected result is that the user is informed of this View requiring the module to be uninstalled and...

  • the user agrees to delete the View as a consequence (minimal transparency, see #11 & #13)
  • or the display extender is removed from all affected views/displays (convenient transparency, see #18)

Proposed resolution

  • Display extenders (and other similarly overlooked plugins) are noted when determining View dependencies
  • DisplayExtenderPluginBase provides an onDependencyRemoval method that removes the extender from all displays, overridable by the plugin itself

Remaining tasks

API changes

TBD

CommentFileSizeAuthor
#13 drupal-views_display_extender_dependencies_and_caching-2426607-13.patch3.43 KBdeepak goyal
#11 drupal-views_display_extender_dependencies_and_caching-2426607-11.patch3.5 KBckaotik
extender_dependencies.patch1.21 KBdevpreview

Issue fork drupal-2426607

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

dawehner’s picture

dawehner
Primary language German
commented
Category: Feature request » Bug report
Issue tags: +VDC, +Needs tests

Note: This is not a feature but rather a missing functionality, so a bug.

Do you mind writing some kind of test? I'm pretty sure we have test coverage for display extenders already.

+++ b/core/modules/views/src/Plugin/views/display_extender/DisplayExtenderPluginBase.php
@@ -82,6 +82,11 @@ public function optionsSummary(&$categories, &$options) { }
+  /**
+   * Calculates dependencies for the configured plugin.
+   */
+  public function calculateDependencies() { }

Let's remove that line, the base class already implements it.Returning NULL is IMHO wrong.

devpreview’s picture

devpreview commented

I have not yet figured out the testing system so that writing tests I can take some time.

Let's remove that line, the base class already implements it.Returning NULL is IMHO wrong.

I acted similarly to other methods from DisplayExtenderPluginBase class. e.g. buildOptionsForm() and optionsSummary().
This methods already defined of views\PluginBase class. But I do not understand why it was done and I do not see the need.

Lendude queued extender_dependencies.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, extender_dependencies.patch, failed testing.

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

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ckaotik’s picture

ckaotik
Primary language German
Location Berlin
commented
Version: 8.6.x-dev » 8.8.x-dev
Status: Needs work » Needs review
Related issues: +#2635728: Uninstalling a module providing display extenders causes fatal errors, +#1811986: Try to minimize the "overhead" of display extenders
StatusFileSize
new drupal-views_display_extender_dependencies_and_caching-2426607-11.patch3.5 KB

I've attached a patch that adds both dependencies and caching information (if present) for a view's display_extender plugins. They weren't yet handled by the generic views plugin integrations for "one of a kind" plugins (e.g. one Query plugin, one Cache plugin etc.).

This will also help with #2635728: Uninstalling a module providing display extenders causes fatal errors as views using a display extender can now be identified by the uninstall validator. It can then warn about them being required as usual.
Currently, the display extender's configuration is added to every view/display (and thus with this patch its dependency as well), regardless of it actually using the functionality. This is mentioned in #1811986: Try to minimize the "overhead" of display extenders, and I tried to prevent this by introducing the DisplayExtenderPluginBase::isEnabled method (using a very naive approach).

We still need tests, though! And possibly an update hook that updates all views with extenders to remove excess data?

joachim’s picture

joachim commented
Status: Needs review » Needs work

Looks good, just a nitpick:

+++ b/core/modules/views/src/Plugin/views/display_extender/DisplayExtenderPluginBase.php
@@ -76,6 +76,26 @@ abstract class DisplayExtenderPluginBase extends PluginBase {
+   * Determine if the plugin is enabled.

Nitpick: first line doc comment should be present tense, so 'Determines ...'.

Also, needs work because this needs tests?

deepak goyal’s picture

deepak goyal commented
StatusFileSize
new drupal-views_display_extender_dependencies_and_caching-2426607-13.patch3.43 KB

Updated patch please review.

deepak goyal’s picture

deepak goyal commented
Status: Needs work » Needs review

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
commented
Issue tags: +Needs issue summary update

What's the actual problem here? What is it trying to solve? How does the solution work? Will it have any effect on #3160900: Entire view is deleted on module uninstallation after depending display is deleted?

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Related issues: +#2867936: Must disable Views EVI display extender before uninstalling
joachim’s picture

joachim commented
Issue summary: View changes
Status: Needs review » Needs work

I've updated the IS.

The patch adds the dependency correctly to my view.

However, uninstalling the module that provides the extender makes the config system delete the view complete. The correct behaviour would be for the display plugin to implement onDependencyRemoval() and remove the extender's settings from the view.

To do that though, we need the View::onDependencyRemoval() to call onDependencyRemoval() on the display plugin, which AFAICT it doesn't. That should probably be fixed as a separate issue first.

ckaotik’s picture

ckaotik
Primary language German
Location Berlin
commented
Issue summary: View changes

To do that though, we need the View::onDependencyRemoval() to call onDependencyRemoval() on the display plugin, which AFAICT it doesn't. That should probably be fixed as a separate issue first.

I agree, giving display extenders and views plugins in general the option to provide a onDependencyRemoval method would help with some of the related issues. I've updated the IS accordingly.

How does the solution work? Will it have any effect on #3160900: Entire view is deleted on module uninstallation after depending display is deleted?

That issues' problem is that dependencies (those that are tracked correctly) are always propagated to the whole view. This a patch for this issue will not help there, quite the opposite because more dependencies (that were previously invisible) will be tracked. That issue may come up with a solution to storing dependencies on a per-display basis, or investigate wha the global dependencies are not updated correctly on changes, i.e. when the "needing" display is removed.

However, #2426607: Calculates and adds dependencies of views display extender (this issue) is about certain dependencies not being tracked at all, which can result in lost functionality and also cause #2635728: Uninstalling a module providing display extenders causes fatal errors during dependency uninstallation. Simply, because Drupal doesn't know the uninstalled module is a dependency.

ckaotik’s picture

ckaotik
Primary language German
Location Berlin
commented
Issue summary: View changes

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mxr576 made their first commit to this issue’s fork.

mxr576 opened merge request !12106

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Issue summary: View changes
Issue tags: -Needs tests, -Needs issue summary update

Added test coverage, incorporated the latest fix from #13.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.