Problem/Motivation

Views optional plugins allow default configuration to be installed with an optional soft-dependency on another module.

Here's an example from core/modules/node/config/views.view.content.yml

 translation_link:
          id: translation_link
          table: node
          field: translation_link
          label: ''
          exclude: true
          alter:
            alter_text: false
          element_class: ''
          element_default_classes: true
          hide_alter_empty: true
          hide_empty: false
          empty_zero: false
          empty: ''
          text: Translate
          optional: true
          plugin_id: content_translation_link
          provider: content_translation

The situation I'm concerned about is the following:

1. Node module is installed, this view is created.

2. content_translation renames the plugin ID for translation_link to translation_uri (or whatever). It includes an update that updates all active views to the new plugin name.

3. content_translation is enabled

4. Because the update in #2 never ran, the configuration now has the correct provider, but the wrong plugin ID.

This issue is critical because it's not that rare of a usecase, and sites can be placed in an unrecoverable state simply by enabling a module that uses this ostensibly supported functionality.

Proposed resolution

  • Remove the optional handler functionality.
  • Provide a separate, disabled default view with the content translation module, with the same path (admin/content) and support for translation.
  • Decide how to respond when c_t is enabled. We probably don't want to magically replace the existing view as the user might have customized it (or we could do so only if it matches the originally installed view). A simpler solution would be for c_t to simply notify the user that the other view is available with a dsm(), an altered message on admin/content, or an admin/reports message.
Files: 
CommentFileSizeAuthor
#15 interdiff.txt3.08 KBdawehner
#15 optional-2212081-15.patch12.75 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,071 pass(es). View
#12 interdiff.txt4.43 KBdawehner
#12 2212081-12.patch9.7 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,928 pass(es), 88 fail(s), and 16 exception(s). View
#9 2212081.patch5.27 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,737 pass(es), 126 fail(s), and 32 exception(s). View

Comments

catch’s picture

xjm’s picture

Issue summary: View changes
Issue tags: +Configuration system
dawehner’s picture

Issue summary: View changes

To be honest I can't really imagine that renaming plugin IDs is a good idea in any place, the same is true for changing default values and so on and so forth.

catch’s picture

Issue summary: View changes

It might not be a good idea but do you really think we can prevent that? Take for example there's a contrib module that provides some plugins (say field + formatters), then a contrib_bonus project provides some extra formatters, then they merge back into the main project later. Both the provider and potentially name of the plugin would need to change.

vijaycs85’s picture

Issue summary: View changes

Another option is, allowing modules to provide it's own plugins for a view. For above case, we can have content_tranlsation.plugin.view.content.yml which provides the plugins (i.e. fields/filters/sort) to content view. So when content_tranlsation module get enabled, the plugins will be automatically added.

The file format is [module name].plugin.view.[view name].yml. So after getting the configuration from [module mame].view.[view name].yml, we can call all the plugins (may need to figure out the order) and update the configuration on both module install & uninstall.

Owning other module's component is always risk especially after live cases.

xjm’s picture

Title: How do views optional plugins in default config get updated for API changes? » Remove views optional handler handling
Issue summary: View changes
Issue tags: +beta target

We discussed this issue with @catch, @alexpott, @effulgentsia, @tim.plunkett, @mtift, @moshe weitzman, and myself at DrupalCon Austin (for reference: issue discussion notes and action items). We added the optional handlers to support a very specific usecase (progressive enhancement of the content admin view when someone enables the content translation module). However, this feature adds a lot of complexity that we aren't really prepared to support. So, in the interest of resolving the issue in the simplest way given where we are at in the release cycle, we agreed to:

  • Remove the optional handler functionality.
  • Provide a separate, disabled default view with the content translation module, with the same path (admin/content) and support for translation.
  • Decide how to respond when c_t is enabled. We probably don't want to magically replace the existing view as the user might have customized it (or we could do so only if it matches the originally installed view). A simpler solution would be for c_t to simply notify the user that the other view is available with a dsm(), an altered message on admin/content, or an admin/reports message.

The issue is still critical, because the consequences of not resolving it before release are that a site can be broken simply by enabling a module that does something we onstensibly support.

xjm’s picture

Issue summary: View changes
Status: Postponed » Active

Oh and I think this can be active now as we're no longer blocked on anything.

Gábor Hojtsy’s picture

Issue summary: View changes

#2301045: Standard profile has views which include elements dependent on uninstalled modules, not valid in config now landed, so there is no actual optional handler elements anymore in the standard profile at least that we know. The feature itself should be removed as well.

dawehner’s picture

Status: Active » Needs review
FileSize
5.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,737 pass(es), 126 fail(s), and 32 exception(s). View

Let's try.

Gábor Hojtsy’s picture

This looks good. I remember seeing a message for those. This patch does not remove them. Are those still needed for the broken handler display?

Status: Needs review » Needs work

The last submitted patch, 9: 2212081.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,928 pass(es), 88 fail(s), and 16 exception(s). View
4.43 KB

There we go.

Status: Needs review » Needs work

The last submitted patch, 12: 2212081-12.patch, failed testing.

Gábor Hojtsy’s picture

+++ b/core/modules/views/src/Plugin/views/BrokenHandlerTrait.php
--- a/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_view_optional.yml
+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_view_optional.yml

What is the remaining point of this file. By its name, it sounds like it would be for testing optional views elements? Which are not to be tested anymore?

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,071 pass(es). View
3.08 KB

There is indeed none. This file was still loaded on the UI test, so removed that in a hunk as well.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Yay, that looks great to me.

  • catch committed d526f0d on 8.x
    Issue #2212081 by dawehner: Remove views optional handler handling.
    
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

If anyone is sad to see this issue go in because they like the optional handler handling, feel free to open a new feature request to add it back in - if there turns out to be a way to make it work then no reason not to add it back, but no-one's come up with one yet.

Gábor Hojtsy’s picture

Yay, thanks!

damiankloip’s picture

We do all realise that this doesn't really fix anything except an optional message in the UI, and supressing a debug() message when a plugin is missing/broken?

Gábor Hojtsy’s picture

@damiankloip: well the pre-patch API said "hey, come on in, we support optional elements" while the new one says "error, unknown". Quite a difference, no?

damiankloip’s picture

I guess anything that helps. Are we ok with not showing broken handlers anywhere apart from if you are on the edit form for a particular view?

+++ b/core/modules/views/src/Entity/View.php
@@ -268,7 +268,7 @@ public function calculateDependencies() {
-            if (isset($handler['provider']) && empty($handler['optional'])) {

This is the main change from the patch tbh.

Gábor Hojtsy’s picture

@damiankloip did the patch remove display of the broken handler info in any way? was it displayed elsewhere outside of the edit form before this patch?

damiankloip’s picture

Well, kind of. Before we had this:

+++ b/core/modules/views/src/Plugin/ViewsHandlerManager.php
@@ -119,12 +115,8 @@ public function getHandler($item, $override = NULL) {
-    if (!$optional) {
-      // debug(t("Missing handler: @table @field @type", array('@table' => $table, '@field' => $field, '@type' => $this->handlerType)));
-    }

Which got commented out (hurting tests with all the messages iirc) "temporarily", then wrapped for the optional functionality. Now removed here.

All I am saying is now you will definitely not be getting info that your view is broken unless you are looking at each handler on the view edit.

Gábor Hojtsy’s picture

Well, it was already dead code without a todo to fix it in any way? I mean we could add the commented line back but it would not change the behaviour a single bit :D

catch’s picture

@Damian we still have #1822048: Introduce a generic fallback plugin mechanism open. I haven't reviewed that patch recently but I'd expect us to show/log errors everywhere when there are missing/broken handlers not only on the form. Do we need a views-specific issue in case that one doesn't happen?

Removing this feature is so that we don't have to worry about supporting upgrade paths for optional handlers in configuration entities - i.e. reducing the number of views that are likely to end up with broken handlers. Once we have configuration validation throwing exceptions and all dependencies set up properly, then the only way to get into a bad state will be a module update which doesn't have an upgrade path written, or brute-force removing a module from the filesystem altogether. I'm don't think we need to do anything nice in either of those cases so it ought to be possible to be much more aggressive about the errors thrown.

Gábor Hojtsy’s picture

#2183983: Find hidden configuration schema issues is experimenting with throwing exceptions for undefined types wholesale when a config key has schema otherwise. Not sure what our policy is / should be though regarding requiring those or not.

Status: Fixed » Closed (fixed)

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