Problem/Motivation

views_get_handler() catches exceptions and prints a debug message when a handler is missing.
This can happen if a view requires a module that isn't enabled, and you get a nice "Broken/missing handler" in your Views UI, not a fatal.

However, this masks errors when something is missing. For example, I think some of our test views have hidden dependencies on the module integration we left out, specifically search and language.

Proposed resolution

Store a list of views plugins and handlers in state(), which indicates which module it belongs to
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.

Remaining tasks

-

User interface changes

-

API changes

-

Files: 
CommentFileSizeAuthor
#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

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?

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

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

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.

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

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.

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.

Assigned:tim.plunkett» Unassigned
Status:Needs work» Postponed

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.

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

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.

Status:Postponed» Active

As per #11

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.

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

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

Status:Needs review» Needs work

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.

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

/me sighs

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"

Priority:Normal» Major

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

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

Issue summary:View changes

updated issue summary

Issue summary:View changes
Issue tags:+Needs reroll

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.

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)

Issue tags:-Needs reroll

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.

Issue tags:-Needs reroll

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?

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

Thank you so much for it!

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

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

Sure! This is a great idea!