Updated: Comment #N

Problem/Motivation

In order to satisfy #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall, we need to gather all view dependencies for a view. We can currently do this easily for handler plugins (fields, filters, sorts) as the provider is already stored in configuration for each. This was done when we made changes to make it easier to tell missing plugins, but also optional plugins.

So, we basically need to do the same for other Views plugins (display, row, cache, access, etc...) so we can get the plugin provider from configuration.

Proposed resolution

Make sure we add the provider to plugins when they are added to a view. Off the top of my head, we need to add this when a display is added, when the display options form is submitted for plugins, and during the wizard.

Remaining tasks

- find all the places this needs to be added
- add to test coverage
- convert existing configuration files to reflect these changes

User interface changes

None

API changes

None. Maybe having to add provider if you have a default view for a module (although if this view has been saved again or something, we could update this...maybe)?

CommentFileSizeAuthor
#5 interdiff-2198423-5.txt5.66 KBdamiankloip
#5 2198423-5.patch13.36 KBdamiankloip
#3 interdiff-2198423-3.txt694 bytesdamiankloip
#3 2198423-3.patch7.71 KBdamiankloip
#1 2198423.patch7.03 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Active » Needs review
FileSize
2198423.patch7.03 KB

To get the ball rolling; we need something like this.

Status: Needs review » Needs work

The last submitted patch, 1: 2198423.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
2198423-3.patch7.71 KB
interdiff-2198423-3.txt694 bytes

That test wasn't broken at all.... (Returning the plugin object instead of the definition).

Status: Needs review » Needs work

The last submitted patch, 3: 2198423-3.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
2198423-5.patch13.36 KB
interdiff-2198423-5.txt5.66 KB

Fixed those failures, and added some test coverage.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Awesome!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented 
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/wizard/TaxonomyTerm.php
@@ -42,6 +42,7 @@ protected function defaultDisplayOptions() {
+    $display_options['access']['provider'] = 'user';

This is added in several places, we can't figure out provider -> user from type -> perm automatically?

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Hmm, possibly, then we would need to load the plugin instance in the wizard default options method. Plus we are adding $display_options['access']['type'] = 'perm'; anyway.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

OK that's not perfect but looks like a bigger refactoring that shouldn't block the dependency issue. Committed/pushed to 8.x, thanks!

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Yeh. Unfortunately there is a lot of views code that is not perfect :)

Status: Fixed » Closed (fixed)

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