Problem/Motivation

ViewsHandlerManager::getHandler() catches exceptions and creates a fallback plugin instance.

Proposed resolution

Let's provide this functionality for other modules.
Introduce a fallback plugin ID on the DefaultPluginManager which could be also used by some field module.

Remaining tasks

-

User interface changes

-

API changes

-

Files: 
CommentFileSizeAuthor
#84 1822048-fallback-plugins-84.patch5.88 KBmartin107
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,618 pass(es).
[ View ]
#80 1822048-fallback-plugins.patch6.81 KBneclimdul
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1822048-fallback-plugins.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#80 1822048-fallback-plugins.interdiff.txt10.22 KBneclimdul
#73 plugin_fallback-1822048-73.patch12.03 KBEclipseGc
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,825 pass(es).
[ View ]
#70 interdiff.txt4.66 KBdawehner
#70 plugin_fallback-1822048-70.patch14.25 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,611 pass(es).
[ View ]
#67 interdiff.txt1.05 KBdawehner
#67 plugin_fallback-1822048-67.patch13.92 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,613 pass(es).
[ View ]
#56 interdiff.txt1.06 KBdawehner
#56 plugin_fallback-1822048-56.patch13.06 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,198 pass(es).
[ View ]
#54 interdiff-1822048-54.txt1.72 KBdamiankloip
#54 1822048-54.patch12.98 KBdamiankloip
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,968 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#38 interdiff.txt869 bytesdawehner
#38 fallback_plugin-1822048-38.patch8.37 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,987 pass(es).
[ View ]
#36 interdiff.txt6.94 KBdawehner
#36 fallback_plugin-1822048-36.patch8.32 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,739 pass(es).
[ View ]
#33 interdiff.txt5.47 KBdawehner
#33 fallback_plugin-1822048-32.patch7.24 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,385 pass(es).
[ View ]
#30 fallback_plugin-1822048-30.patch2.64 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,391 pass(es).
[ View ]
#23 vdc-1822048-23.patch699 bytestim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch vdc-1822048-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 vdc-1822048-20.patch584 bytestim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch vdc-1822048-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
stop-hiding-errors.patch604 bytestim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 48,123 pass(es), 50 fail(s), and 11 exception(s).
[ View ]

Comments

dawehner’s picture

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?

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett
Status:Active» Needs review

The patch was meant to expose bugs, not a solution.

dawehner’s picture

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.

Status:Needs review» Needs work

The last submitted patch, stop-hiding-errors.patch, failed testing.

xjm’s picture

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.

damiankloip’s picture

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

xjm’s picture

xjm’s picture

Status:Needs work» Needs review
Issue tags:-VDC

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

Status:Needs review» Needs work
Issue tags:+VDC

The last submitted patch, stop-hiding-errors.patch, failed testing.

dawehner’s picture

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.

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned
Status:Needs work» Postponed
sun’s picture

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.

tim.plunkett’s picture

"if it attempts to use a plugin that does not exist"

If you're excluding plugins from disabled modules from that definition, what's left?
Someone hand-writing a view YAML file with a typo?

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.

As I see it, unavailable == broken, no matter how you redefine them

If we want to treat filter handlers different from field/sorts, then that's fine, but executing a view with broken/unavailable filter could be a security risk.

dawehner’s picture

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.

sun’s picture

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).

dawehner’s picture

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.

yched’s picture

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.

catch’s picture

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.

jibran’s picture

Status:Postponed» Active

As per #11

tim.plunkett’s picture

Status:Active» Needs review
StatusFileSize
new584 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch vdc-1822048-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Apparently this debug is now messing with the testbots.

alexpott’s picture

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

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

xjm’s picture

Status:Needs review» Needs work
tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new699 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch vdc-1822048-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I forgot we had tests for that! My apologies.

alexpott’s picture

Status:Needs review» Needs work

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...

dawehner’s picture

/me sighs

tim.plunkett’s picture

More targeted issue for optional. Feel free to close as a dupe, but I think we could solve that quickly.
#2016953: Indicate when an optional handler is missing, that it is not "broken"

tim.plunkett’s picture

Priority:Normal» Major

We still have commented out code and tests to prevent SimpleTest from choking, we can't ship like this.

dawehner’s picture

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

dawehner’s picture

Issue summary:View changes

updated issue summary

martin107’s picture

Issue summary:View changes
Issue tags:+Needs reroll
dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new2.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,391 pass(es).
[ View ]

Just put some of my ideas out.

yched’s picture

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)

martin107’s picture

Issue tags:-Needs reroll
dawehner’s picture

Issue tags:+Needs reroll
StatusFileSize
new7.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,385 pass(es).
[ View ]
new5.47 KB

Sure why not. This also creates a unit test.

drabik’s picture

Issue tags:-Needs reroll

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

damiankloip’s picture

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?

dawehner’s picture

StatusFileSize
new8.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,739 pass(es).
[ View ]
new6.94 KB

Thank you so much for it!

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
@@ -278,4 +296,39 @@ protected function findDefinitions() {
+      if (isset($this->fallbackPluginId)) {
...
+        $instance = $this->factory->createInstance($this->getFallbackPluginId($plugin_id, $configuration), $configuration);

This feels strange. Couldn't this just be
if ($fallback_plugin_id = $this->getFallbackPluginId($plugin_id, $configuration)) {?

dawehner’s picture

StatusFileSize
new8.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,987 pass(es).
[ View ]
new869 bytes

Sure! This is a great idea!

dawehner’s picture

Title:Consider improving/removing the concept of "broken" handlers» Introduce a generic fallback plugin mechanism

Update the title.

dawehner’s picture

Component:views.module» plugin system

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

damiankloip’s picture

  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.

dawehner’s picture

StatusFileSize
new8.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,391 pass(es).
[ View ]
new1.27 KB

Thank you for your review!

damiankloip’s picture

Status:Needs review» Reviewed & tested by the community

I think this is ready.

Xano’s picture

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.

damiankloip’s picture

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.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

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.

dawehner’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new12.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,678 pass(es), 0 fail(s), and 31 exception(s).
[ View ]
new4.25 KB

Implemented the change for views and updated the issue summary.

Status:Needs review» Needs work

The last submitted patch, 47: vdc-1822048-47.patch, failed testing.

dawehner’s picture

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

The last submitted patch, 47: vdc-1822048-47.patch, failed testing.

yched’s picture

  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...

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new12.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,740 pass(es), 0 fail(s), and 31 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 52: 1822048-52.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new12.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,968 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.72 KB

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

Status:Needs review» Needs work

The last submitted patch, 54: 1822048-54.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new13.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,198 pass(es).
[ View ]
new1.06 KB

Fixed the failure.

dawehner’s picture

Any feedback?

larowlan’s picture

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.

dawehner’s picture

StatusFileSize
new15.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,314 pass(es), 44 fail(s), and 19 exception(s).
[ View ]

Sure, here is hopefully all what is needed.

Status:Needs review» Needs work

The last submitted patch, 59: plugin_fallback-1822048-59.patch, failed testing.

larowlan’s picture

+++ 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?

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new12.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch plugin_fallback-1822048-62.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new0 bytes

Let's do that in a follow up.

martin107’s picture

StatusFileSize
new2.79 KB

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

Status:Needs review» Needs work

The last submitted patch, 62: plugin_fallback-1822048-62.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new12.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,609 pass(es).
[ View ]

Clean 3way reroll.

Anyone up for a review?

dawehner’s picture

StatusFileSize
new13.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,613 pass(es).
[ View ]
new1.05 KB

@chx suggested to convert the FilterFormatPluginManager as well.

tim.plunkett’s picture

I'm definitely +1 on this.

  1. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
    @@ -212,6 +220,7 @@ public function processDefinition(&$definition, $plugin_id) {
       }

    +
       /**

    Not needed.

  2. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
    @@ -241,4 +250,50 @@ protected function findDefinitions() {
    +        // Allow implementations show the exception message.

    Allow implementations to show the exception message.

  3. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
    @@ -241,4 +250,50 @@ protected function findDefinitions() {
    +  public function setFallbackPluginId($fallback_plugin_id) {

    Why is this public (outside of needing it in one test)? And if it needs to be, why not put it on an interface? And finally, might as well return $this.

  4. +++ b/core/modules/filter/src/FilterPluginManager.php
    @@ -37,19 +37,8 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
    -  /**
    -   * {@inheritdoc}
    -   */
    -  public function getDefinition($plugin_id, $exception_on_invalid = TRUE) {
    -    $definitions = $this->getDefinitions();
    -    // Avoid using a ternary that would create a copy of the array.
    -    if (isset($definitions[$plugin_id])) {
    -      return $definitions[$plugin_id];
    -    }
    -    // If the requested filter is missing, use the null filter.
    -    return $definitions['filter_null'];
    +    $this->setFallbackPluginId('filter_null');

    This is awesome.

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

    Looks accidental/unneeded.

  6. +++ b/core/modules/views/src/Plugin/ViewsHandlerManager.php
    @@ -97,26 +99,25 @@ public function getHandler($item, $override = NULL) {
    +      $this->setFallbackPluginId('broken');
    +      $result = $this->createInstance($definition['id'], $definition);
    +      $this->setFallbackPluginId(NULL);

    Please document why we set the fallback ID back to NULL.

chx’s picture

@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.

dawehner’s picture

StatusFileSize
new14.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,611 pass(es).
[ View ]
new4.66 KB

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.

EclipseGc’s picture

StatusFileSize
new12.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,789 pass(es), 24 fail(s), and 116 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 71: plugin_fallback-1822048-71.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new12.03 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,825 pass(es).
[ View ]

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

dawehner’s picture

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

neclimdul’s picture

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.

EclipseGc’s picture

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

andypost’s picture

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

yched’s picture

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)

neclimdul’s picture

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.

neclimdul’s picture

StatusFileSize
new10.22 KB
new6.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1822048-fallback-plugins.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Maybe something like this.

dawehner’s picture

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

EclipseGc’s picture

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

dawehner’s picture

Status:Needs review» Reviewed & tested by the community
+++ b/core/lib/Drupal/Component/Plugin/FallbackPluginManagerInterface.php
@@ -0,0 +1,27 @@
+<?php
+/**

meh, this needs a newline :(

martin107’s picture

StatusFileSize
new5.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,618 pass(es).
[ View ]

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

So no interdiff :(

Also added newline.

The last submitted patch, 80: 1822048-fallback-plugins.patch, failed testing.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

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.

  • alexpott committed 099e0fa on 8.0.x
    Issue #1822048 by dawehner, tim.plunkett, damiankloip, EclipseGc,...
yched’s picture

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

larowlan’s picture

+++ 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

larowlan’s picture

larowlan’s picture

Status:Fixed» Closed (fixed)

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