Introduce a generic fallback plugin mechanism

Having a real error message would really help but i'm a bit worried about something like the following usecase:

• A view contains a lot of different fields, which contains to several modules
• Due to a site-update some of these modules get disabled, so the handlers doesn't exist anymore

In previous versions the actual executed view on the livepage didn't failed but just made these specific things hidden but sure you should really take care about that. Let's assume you have some access checking, so the user could see content which never was supposed to be seen.

One question i have: should a view track internally what modules it needs to run fine?

Sure, but my comment is still sort of valid if you plan

Perhaps we can throw a named exception and have the Views UI catch them if it's on?

You will certainly run into the config get's imported even if required modules aren't enabled, issue.

We should additionally notify the user when a view is being edited what the handlers are, and possibly provide an entry on the admin/reports patch.

Also, in order to report what module the broken unavailable handlers were originally from, I think we still need to store the providing module information in the config file. (It should be possible to ship a default view that includes optional handling that only becomes active when, e.g., locale is enabled, and it should be possible to inform the user that locale is needed for that handling.) What gets stored in state() is the status of which handlers are unavailable in which views.

This is the issue I created with this in mind: #1825896: Add module owner to plugin data on handlers

Related: #1601168: Orphan/obsolete keys and sub-keys *within* configuration objects are retained *forever*

stop-hiding-errors.patch queued for re-testing.

Once we have #1810480: Provide the plugin_id to support views metadata integration and the patch that adds the module name in, we have the required information
to move this issue forward, because then we can show the users which plugin they expect to be there.

The HTML formatting of #5 is key here, I think:

report what module the broken unavailable handlers were originally from

I'd think that a view should only be considered as broken, if it attempts to use a plugin that does not exist.

However, if the plugin is merely unavailable, because the providing module is not enabled, then the view should actually not be considered as broken.

So I'd think: broken != unavailable. Only in case of broken, it should blow up. If only unavailable (which can be easily and quickly determined at runtime), then it's not broken, just "limited."

Of course, that only applies to field/filter/argument plugins... If a display or style plugin is missing, then there's no way to omit those, I guess.

So I'd think: broken != unavailable. Only in case of broken, it should blow up. If only unavailable (which can be easily and quickly determined at runtime), then it's not broken, just "limited."

Well then the question is what do we define as broken, as currently our definition of broken is "unavailable".
"Broken" could be triggered by the plugin itself, even I'm not sure what this gives us.

Of course, that only applies to field/filter/argument plugins... If a display or style plugin is missing, then there's no way to omit those, I guess.

The question is how broken should it be. One thing we could do is to fallback to the default behavior, which would work at least for the style. For display plugins you end up in a situation in which the actual view is never rendered, because display plugin take care about the place in which a view is rendered.

If you're excluding plugins from disabled modules from that definition, what's left?
...
If a module chooses to remove a plugin, it could update the YAML file with an upgrade hook, so we don't need to support that.

Exactly. Something is "broken" when it tries to use something that cannot reasonably exist. If a plugin-providing module forgot to update all affected views — or hey, in case there's no sane facility to do so in the first place ;) and developers just simply did not write the 100k lines for a possibly required module update :P — then that is the definition of "broken." :)

[Oh, and in case a module got uninstalled, but Views did not implement hook_modules_uninstalled() to adjust existing views, because the module being uninstalled cannot do anything about that, then that's of course also a definition of "broken"... ;)]

But if something is just unavailable right now, and if the view can technically be executed without the thing being available (security is one reason, inability to fall back to sane defaults is another), there's no reason to not execute the view.

The primary reasons for reaching this situation are caused by a modular system. As with everything else, there are things that are able to happily operate further upon losing functionality (e.g., disabling action.module), but there are also things that cannot (e.g., disabling system.module).

So how can we detect the difference between something does not exist, as the views config is not up to date,
and it does not exist because something wasn't installed yet? Maybe i'm thinking to simple but I don't see a difference,
as the plugin simply can't be found in both cases. If a module is not enabled we don't know the plugins of it.

On the actual page I think we should always fallback to a non-broken behaviour, though in the admin UI we should better find a way to distinct between the two ways.

Lurking with interest.
The case of "a runtime object configured tu use a plugin that is provided by a missing module" is something that all subsystems had to deal with "somehow" so far (and in the case of Field API, it's a major PITA).
With plugin API, this now appears in slightly more similar terms across subsystems.

There is most probably not one good answer, a missing/unknown plugin means various levels of "broken" depending on the subsytem and plugin type. For missing field formatters, for example, we silently switch back to the 'default_formatter'. For missing field types, OTOH, this means we just cannot do anything with corresponding values (load, edit, save, display them), and getting those fields out of the regular flow of operations, and back in if the field type gets "known again", adds lots of code & concepts complexity.
"Field types as plugins" is not done yet, but I was actually kind of wondering whether having a BrokenFieldType, like views has, might simplify things a bit - so, I'm curious why you guys consider removing the one you have.

And, agreed with the previous comments than "never heard of it / was provided by a module that is disabled right now / was provided by a module whose code has just vanished from the filesystem" are just several flavors of "unknown" that we cannot tell apart - and I'm not sure what the difference would actually be.

Cross-linking #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed which aims to reduce the 6,000 potential variations of disappearance down at least slightly.

As per #11

Committed #20 7cffb9f and pushed to 8.x. Thanks!

Leaving this issue open as I feel this is but a sticking plaster...

Ran tests locally...

Drupal test run
---------------

Tests to be run:
 - Views Module tests (Drupal\views\Tests\ModuleTest)

Test run started:
 Friday, May 31, 2013 - 06:21

Test summary
------------

Views Module tests 52 passes, 0 fails, and 0 exceptions

Test run duration: 9 sec

Committed #23 5de3941 and pushed to 8.x. Thanks!

Setting back to needs work as per #21,#22...

/me sighs

The main reason why we had to comment that out was that the config got somehow imported before the module handler get resetted.

updated issue summary

Just put some of my ideas out.

Widgets / formatters could use this if the logic was delegated to a getFallbackPluginId() method (they need more complex logic since the fallback widget depends on the field type)

Sure why not. This also creates a unit test.

I could apply the patch, so no reroll is needed.

This is looking pretty cool.

1. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -99,6 +100,13 @@ class DefaultPluginManager extends PluginManagerBase implements PluginManagerInt
   +  protected $fallbackPluginId;
   
   @@ -112,12 +120,13 @@ class DefaultPluginManager extends PluginManagerBase implements PluginManagerInt
   +  public function __construct($subdir, \Traversable $namespaces, ModuleHandlerInterface $module_handler, $plugin_definition_annotation_name = 'Drupal\Component\Annotation\Plugin', $fallback_plugin_id = NULL) {
   ...
   +    $this->fallbackPluginId = $fallback_plugin_id;
   

   How about we don't have this as a constructor param and just assign the property definition above to '' or just leave as NULL (prob better). Then extending classes can either override the property, or do something to it in their constructor?

2. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -277,4 +286,40 @@ protected function findDefinitions() {
   +
   

   Nitpick alarm: Remove?

3. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -277,4 +286,40 @@ protected function findDefinitions() {
   +        $configuration['_exception'] = $exception;
   

   Seems like a good idea, would not have thought of that! ++

4. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -277,4 +286,40 @@ protected function findDefinitions() {
   +   * @param array $configuration
   ...
   +  protected function getFallbackPluginId($original_plugin_id, $configuration) {
   

   Should we typehint array here in the method signature too?

5. +++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php
   @@ -212,6 +215,45 @@ public function testCacheClearWithTags() {
   +   * Tests the plugin manager without a fallback plugin.
   ...
   +   * Tests the plugin manager with a fallback plugin.
   

   @covers

6. +++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php
   @@ -212,6 +215,45 @@ public function testCacheClearWithTags() {
   +      ->will($this->throwException(new PluginException()));
   

   Nice!

7. +++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php
   @@ -212,6 +215,45 @@ public function testCacheClearWithTags() {
   +    $plugin_manager = new TestPluginManager($this->namespaces, $this->expectedDefinitions, NULL, NULL);
   

   Do we need the NULL's? They are just the default anyway.

8. +++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php
   @@ -212,6 +215,45 @@ public function testCacheClearWithTags() {
   +    $apple = new Apple();
   

   mmmm apples.

9. +++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php
   @@ -212,6 +215,45 @@ public function testCacheClearWithTags() {
   +    $exception = new PluginException();
   

   This doesn't get thrown as soon as it is created/stored here?

10. +++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php
    @@ -212,6 +215,45 @@ public function testCacheClearWithTags() {
    +    $this->assertSame($apple, $plugin);
    

    Should/could we test the configuration on the plugin too?

Thank you so much for it!

Sure! This is a great idea!

Update the title.

Let's be honest here ... seriously a month without any review?

1. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -99,6 +100,13 @@ class DefaultPluginManager extends PluginManagerBase implements PluginManagerInt
   +   * Defines a fallback ID in case the plugin could not be created.
   

   Maybe this should mention 'plugin instance'

2. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -278,4 +296,39 @@ protected function findDefinitions() {
   +   * Returns the plugin ID to be used.
   

   'Returns the fallback plugin ID to be used'

3. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -278,4 +296,39 @@ protected function findDefinitions() {
   +   *   The plugin ID of the fallback instance.
   

   We should also mention to return nothing if this method decides no fallback ID should be used for whatever reason.

Thank you for your review!

I think this is ready.

Do we have use cases for allowing the factory to throw exceptions, or can we just as well check for the existence of the requested plugin ID and use the fallback it the requested ID is not available. It's faster and results in cleaner code.

Spoke to Xano about this on IRC:

- We want to keep it like this so we *can* catch any exception that maybe be thrown when an instance is trying to be created. This will not necessarily only be due to a missing definition.
- This is not the most common case, most plugins will be found, so we don't want to check the definition first every time. So this IMO is the cleaner and faster approach.

Looks like the issue summary needs an update since

If a plugin's module is not enabled, and it's not present in the state-based list, either throw an exception or write to watchdog, based on error-reporting.

Does not sound like the fallback mechanism implemented by the patch.

Plus it'd be nice to have at least one real world use case covered in the patch. Chatted with @dawehner in IRC and he said he was ok with converting views to use this is this patch.

Implemented the change for views and updated the issue summary.

47: vdc-1822048-47.patch queued for re-testing.

1. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -162,6 +170,16 @@ protected function alterInfo($alter_hook) {
   +  public function setFallbackPluginId($fallback_plugin_id) {
   
   @@ -277,4 +295,40 @@ protected function findDefinitions() {
   +  protected function getFallbackPluginId($original_plugin_id, array $configuration) {
   

   Minor nitpick : could we get the getter and the setter next to each other in the file ? :-)

2. +++ b/core/modules/views/lib/Drupal/views/Plugin/ViewsHandlerManager.php
   @@ -104,30 +106,26 @@ public function getHandler($item, $override = NULL) {
   +    // @todo This is crazy. Find a way to remove the override functionality.
   

   Does this @todo need an issue link ? (it has "crazy" in it...)
   [edit: oops, never mind, that's existing code being moved around]

Also, would be lovely if the custom fallback logic implemented in FormatterPluginManager / WidgetPluginManager's getInstance() could be replaced with an override of getFallbackPluginId(); If not, can also be a followup...

Here is a reroll for now, didn't bother with an interdiff for moving the method order. I'll see how this gets on, then implement #51.2

The '_exception' property leaves a lot of crap from the exception trace in there, which then subsequently gets serialized, or tries to anyway...

Fixed the failure.

Any feedback?

Love it, there's some tests in #2249303: Implement fallback plugin for Block plugins we could adapt here too, for when a block content UUID is non-existant.

Sure, here is hopefully all what is needed.

+++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
@@ -154,9 +154,8 @@ public function build() {
+        '#markup' => t('Block with uuid %uuid does not exist.', array(

Don't we need a fall back plugin ID for this to work?

Let's do that in a follow up.

I wanted to see the changes, just posting what I found for reference.

Clean 3way reroll.

Anyone up for a review?

@chx suggested to convert the FilterFormatPluginManager as well.

@chx suggested to convert the FilterFormatPluginManager as well.

And you are listening to him? What an idea.

+    try {
+      $instance = $this->factory->createInstance($plugin_id, $configuration);
+    }
+    catch (PluginException $exception) {
+      if ($fallback_plugin_id = $this->getFallbackPluginId($plugin_id, $configuration)) {
+        // Allow implementations show the exception message.
+        $configuration['_exception'] = new FlattenException($exception);
+        $instance = $this->factory->createInstance($this->getFallbackPluginId($plugin_id, $configuration), $configuration);
+      }
+      else {
+        throw $exception;
+      }
+    }

PHP allows nesting of try-catch statements and I would definitely wrap the fallback createInstance in one with an empty catch -- noone really cares that the fallback failed if it fails rather we should log/show the original exception. This should be very rare but it's my duty to think of edge cases :)

+ public function setFallbackPluginId($fallback_plugin_id) {

Why is this public? Why does this exist? What is the use case for this? I can see some demented plugin manager having logic in getFallbackPluginId and then what will this do? In every single case of the patch setFallbackPluginId can be replaced with protected $fallbackPluginId = '...' on the relevant plugin manager class. But if we must, can we please make this protected? Or can we just move it to the test manager like setFactory?

Tests the plugin manager with a disabled module

That will be some test given that the disabled module functionality is removed from D8. I know it's not written in this patch so if it's out of scope please follow up.

Let's see whether these changes actually still work.

+++ b/core/modules/views/src/Plugin/ViewsHandlerManager.php
@@ -11,6 +11,7 @@
+use Drupal\views\Plugin\views\HandlerBase;

Well, it is not accidental, but when I am in this file I can also fix the bug in there.

OK, this is completely untested, however as an approach I wanted to propose this. dawehner and I discussed this at length in irc.

The crux of this is that we can do this generically for all PluginManager. Essentially we check to see if our manager implements the FallbackPluginManagerInterface. If it does, we know there's a method for handling instantiation of fallback plugins. That method then is 100% responsible for delivering a new instance for the current needs. This has the nice upshot of removing fallback/broken plugins from the discovery pattern. They can still remain in the same directories, but they don't need an annotation. I didn't add an interface for fallback classes, but this might make sense too. In the case of views handlers, there's a broken() method on them. I checked this in my patch. I actually expect this patch to fail, but it's more a "hey does this make sense".

Let me know.

Eclipse

dawehner and I are discussing this further. He has issues with my factory method for fallback classes. I'm going to keep pushing forward on the patch for the moment just to make sure it works conceptually so we can weigh all our options.

Eclipse

What about using that interface but by default use the factory to create that instance?

So, I think the reason EclipseGC hadn't done that is related to this code which I also had thoughts about.

+++ b/core/modules/filter/src/Plugin/Filter/FilterNull.php
@@ -16,13 +16,6 @@
- *
- * @Filter(
- *   id = "filter_null",
- *   title = @Translation("Provides a fallback for missing filters. Do not use."),
- *   type = Drupal\filter\Plugin\FilterInterface::TYPE_HTML_RESTRICTOR,
- *   weight = -10
- * )

As with the views plugins later, if we don't want it to show up, maybe a "hidden" plugin attribute would be more useful? The line between UI gets complicated here though because you'll need to filter results between the getDefinitions and your UI.

Obviously if we're only instantiating it manually as the patch does we don't need the annotation but if we want to use the factory which seems reasonable since it contains normal logic for this.

I REALLY don't want the fallbacks to be discoverable. I think that's likely to cause issue. Likewise, a magic "hidden" key allows all sorts of stuff to be hidden. I'm not actually against that, but it feels like it's a whole OTHER thing. We can't do a magic 'fallback' key cause what if we have two? and on and on. So I went with a factory method to get our fallback. If we want to expand the FactoryInterface to have a createInstance() method that instantiates plugins and a public instantiateClass() method that is JUST the raw factory so that the class can be passed to raw instantiation logic, I'm not against it, but we can't effectively pass to createInstance() here since that could end in a loop (at least with my current logic) and I don't want fallbacks to be annotated.

Is my basic desire unreasonable here? Am I missing anything? I just sort of felt like expanding the FactoryInterface was unnecessary. It seems unlikely to most fallback classes will need anything fancy, and even if they do the manager is registered on the DIC so it's easy to get that stuff to the Manager. These are my basic thoughts at least.

Eclipse

there was related issue about widgets and and formatters, entity displays has own fragile fallback for this ones

Just a note that fallbacks are not necessarily special, "hidden, do not discover plz" plugin implementation.
FormatterPluginManager::getInstance() falls back to the "defaul formatter" for the field type if the requested formatter is unknown or not applicable to the field type - and same for widgets.

(granted, this relies on a Field API specific construct where each field type declare a default formatter and widget, that are supposed to always exist - i.e they are provided by the same module that provides the field type)

Right, I was sort of making the assumption that they might be. "Broken" in views for example wouldn't be something you'd want to show up.

I'll write up a patch for how I think we might do this.

Maybe something like this.

I really prefer that way, still having a method on the interface but not bypass the factory.

+++ b/core/modules/views/src/Plugin/ViewsHandlerManager.php
@@ -101,18 +102,11 @@ public function getHandler($item, $override = NULL) {
+      $handler = $this->createInstance($plugin_id, $definition);
+      if ($override && method_exists($handler, 'broken') && $handler->broken()) {
+        $handler = $this->createInstance($definition['id'], $definition);
       }
+      return $handler;
     }

<3

I've said this in IRC, I'll say it here again.

a.) I'm not going to fight this. If everyone not-me thinks this is the way forward, so be it... however
b.) By using this approach we are explicitly depending upon the very system we expect to fail under certain circumstances. We all acknowledge that any number of things could cause this sort of fallback behavior, and instead of having a reliable fallback, we're using discovery (a system that just failed if we get into this code) to find a fallback for us. Are the odds of it failing for the fallback low? of course. But someone could (for instance) alter the class that's used for the fallback, or unset it from the list or any number of other things. It just seems like a mistake to me.

yched,

Your specific example can probably be served rather easily by my proposed factory method. I'd be happy to explore that directly if we decide to more seriously consider my approach here.

@all,

I'll reiterate, I won't fight for this position any further if no one else has the same concerns.

Eclipse

+++ b/core/lib/Drupal/Component/Plugin/FallbackPluginManagerInterface.php
@@ -0,0 +1,27 @@
+<?php
+/**

meh, this needs a newline :(

Patch no longer applied - needed reroll, no conflicts just auto merging etc

So no interdiff :(

Also added newline.

Committed 099e0fa and pushed to 8.0.x. Thanks!

1. +++ b/core/lib/Drupal/Component/Plugin/FallbackPluginManagerInterface.php
   @@ -0,0 +1,28 @@
   +interface FallbackPluginManagerInterface {
   

   I debated with @dawehener whether or not this should extend PluginManager interface - I was not convinced by the idea that this keeps the interfaces composable but happy to have this debated in a later issue if it comes to that.

2. +++ b/core/lib/Drupal/Component/Plugin/PluginManagerBase.php
   @@ -8,6 +8,8 @@
   +use Drupal\Component\Plugin\FallbackPluginManagerInterface;
   

   Not needed use - same namespace. Removed on commit.

Can we open a followup to make widget anf formatters managers leverage this ? (sorry, not easy from my phone)

+++ b/core/lib/Drupal/Component/Plugin/FallbackPluginManagerInterface.php
@@ -0,0 +1,28 @@
+   *   The id of an existing plugin to use for ... why isn't this in the factory?

>80 and seems inappropriate

Re-opened #2249303: Implement fallback plugin for Block plugins

Fixed #89 in #2249303: Implement fallback plugin for Block plugins