The DefaultFactory plugin factory class will throw exceptions when a. a plugin id doesn't exist in the defintions or b. the class cannot be found but the defintion is there.

Currently, if for any reason, the views display plugin is either invalid or unfound, we get an uncaught exception. So let's fail in a slightly nicer way (like field plugins etc... do in views) and catch the plugin exceptions instead.

Here is a patch and some simple tests for it.

Files: 
CommentFileSizeAuthor
#69 1879774-68.patch14.59 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 56,107 pass(es).
[ View ]
#68 1879774-68.patch14.59 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 57,303 pass(es).
[ View ]
#63 1879774-63.patch14.58 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1879774-63.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#61 Screen Shot 2013-05-21 at 16.48.22.png65.68 KBdamiankloip
#58 1879774-58.patch14.57 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,931 pass(es).
[ View ]
#56 1879774-56.patch14.53 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 55,910 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#56 interdiff-1879774-56.txt1.48 KBdamiankloip
#51 1879774-51.patch13.01 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,834 pass(es).
[ View ]
#49 1879774-49.patch13 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 54,350 pass(es).
[ View ]
#49 interdiff-1879774-49.txt11.72 KBdamiankloip
#38 drupal-1879774-38.patch4.05 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 54,211 pass(es).
[ View ]
#38 interdiff.txt535 bytesdawehner
#36 drupal-1879774-36.patch4.05 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 54,073 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#36 interdiff.txt1.84 KBdawehner
#29 1879774-29.patch3.83 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 53,304 pass(es).
[ View ]
#25 drupal-1879774-25.patch3.66 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 52,672 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#22 1879774-22.patch3.8 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1879774-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#22 interdiff.txt1.67 KBdamiankloip
#19 1879774-19.patch3.63 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 52,260 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#19 interdiff.txt1.02 KBdamiankloip
#17 drupal-1879774-17.patch3.69 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 52,217 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#17 interdiff.txt1.22 KBdawehner
#12 drupal-1879774-12.patch3.68 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 50,423 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#11 drupal-1879774-11.patch2.74 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 50,613 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#5 1879774-5.patch3.77 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 50,629 pass(es).
[ View ]
#4 1879774-4.patch3.75 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 50,484 pass(es).
[ View ]
#4 interdiff.txt788 bytesdamiankloip
#2 1879774-2.patch3.73 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 50,442 pass(es).
[ View ]
d8.display-plugin-catch.patch3.75 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 50,450 pass(es).
[ View ]

Comments

+++ b/core/modules/views/lib/Drupal/views/DisplayBag.phpundefined
@@ -66,7 +67,17 @@ protected function initializePlugin($display_id) {
+      watchdog('views', $e->getMessage());
+      drupal_set_message(t('!message', array('!message' => $e->getMessage())), 'warning');

What about using debug()?

+++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/DisplayTest.phpundefined
@@ -141,4 +141,23 @@ public function testGetAttachedDisplays() {
+    $this->assertText('The plugin (invalid) did not specify an instance class.');

Afaik we use t() for assertText()

StatusFileSize
new3.73 KB
PASSED: [[SimpleTest]]: [MySQL] 50,442 pass(es).
[ View ]

Thanks!

I changed the set message for a debug. We do use debug everywhere else in views for these types of things.

I am not sure about the assertText, currently a mixture of with and without t() is used. But I guess we might as well, then both are run through t() and it's consistent.

Status:Needs review» Reviewed & tested by the community

Afaik the rule is easy: No-t for assertion messages, t() for everything else.

StatusFileSize
new788 bytes
new3.75 KB
PASSED: [[SimpleTest]]: [MySQL] 50,484 pass(es).
[ View ]

Small change, just added a variable to store the result of $e->getMessage().

StatusFileSize
new3.77 KB
PASSED: [[SimpleTest]]: [MySQL] 50,629 pass(es).
[ View ]

And a version with drupal_set_message, just in case. Someone else can decide which they want.

On the other hand we do have

    if (empty($this->displayHandlers[$display_id])) {
      $display_id = 'default';
      if (empty($this->displayHandlers[$display_id])) {
        debug(format_string('set_display() called with invalid display ID @display.', array('@display' => $display_id)));
        return FALSE;
      }
    }

already in ViewExecutable::setDisplay()

Yeah, which I think is not the right way to handle it. ;)

That code in setDisplay would not really help anymore, because the plugin exception would already be thrown. So I don't think that code can actually handle what we're trying to do here.

If we don't want displays to break, we have to implement this here really. So the only question is do we go with the debug, or the drupal_set_message version? I think I vote for the latter.

Status:Reviewed & tested by the community» Needs review

Well, part of the discussion I raised in #1881630: [meta] Determine how to respond to invalid plugins (plugin dependencies) is about doing the same thing consistently in the same circumstances in different modules, and about who should see what when. E.g., in this issue, what does "We don't want displays to break" mean? The goal is going to be different if we are loading the admin form versus rendering the view to an end user. I also think we should consider only displaying a message to users who have permission to administer Views.

Edit: I'd also rather add a report entry to the reports page, rather than logging messages to watchdog. But let's discuss in #1881630: [meta] Determine how to respond to invalid plugins (plugin dependencies).

StatusFileSize
new2.74 KB
FAILED: [[SimpleTest]]: [MySQL] 50,613 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

But just looking at the current code of development: Having an uncatched exception laying around is wrong, at least from my perspective.
Even small progress is progress.

Rerolled against the usual changes of views.

StatusFileSize
new3.68 KB
FAILED: [[SimpleTest]]: [MySQL] 50,423 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Forgot the yml file.

Status:Needs review» Reviewed & tested by the community

I think this should go in to stop the bleeding, and we can improve it holistically in the linked meta.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, drupal-1879774-12.patch, failed testing.

Mhhh the message is not shown, as I guess something before takes care about it.

Yeah, I think maybe the default display plugin is being used or something. Maybe this changed in commmit d0492226, but not sure?

Status:Needs work» Needs review
StatusFileSize
new1.22 KB
new3.69 KB
FAILED: [[SimpleTest]]: [MySQL] 52,217 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

From my perspective the problem is something different:

If you save a view, views_invalidate_cache() is called that automatically forces a menu rebuild. Now the actual hook_menu entry does not exists at all anymore
and you don't have an invalid display. We might should test the code directly.

Status:Needs review» Needs work

The last submitted patch, drupal-1879774-17.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.02 KB
new3.63 KB
FAILED: [[SimpleTest]]: [MySQL] 52,260 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Yeah, that sounds totally right actually. So I think before we had troubles with menu rebuilds (in tests). I.e. now we dont have to call menu_router_rebuild() in tests. I think we need to test this in the context of a page callback. To me that makes sense.

How about we circumvent the entity API to save the view, and do this directly in config, then the rebuild wont be called? Although, I think the fact that we now use the new routing system may give us different issues instead?

Status:Needs review» Needs work

The last submitted patch, 1879774-19.patch, failed testing.

So this works as expected if you test this normally, but in the test environment it doesn't.

StatusFileSize
new1.67 KB
new3.8 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1879774-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

So we could make these changes? I'm not sure why but the reason the tests are failing is because a database exception is being caught but when it's decoded it fatals. (decodeException()).

What is the advantage of using node here? It always feels wrong to use node in tests.

I'm not sure why but the reason the tests are failing is because a database exception is being caught but when it's decoded it fatals. (decodeException()).

That's why. So it works with node, but not with out views_test_data table.

Status:Needs work» Needs review
StatusFileSize
new3.66 KB
FAILED: [[SimpleTest]]: [MySQL] 52,672 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Rerolled the patch, though I still get the exception.

Status:Needs review» Needs work

The last submitted patch, drupal-1879774-25.patch, failed testing.

Yeah let's go with the route of damian and use node. These at least don't fail completely.

Status:Needs work» Needs review

#22: 1879774-22.patch queued for re-testing.

StatusFileSize
new3.83 KB
PASSED: [[SimpleTest]]: [MySQL] 53,304 pass(es).
[ View ]

rerolled.

Status:Needs review» Reviewed & tested by the community

That's ready to go now: We have test coverage, so let's get this in.

Issue tags:-VDC

#29: 1879774-29.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1879774-29.patch, failed testing.

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

#29: 1879774-29.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

This was already RTBC.

Priority:Normal» Major
Status:Reviewed & tested by the community» Needs work

@effulgentsia and I discussed this patch today, and then I talked about it with @tim.plunkett.

There are two things that happen when a display plugin exception is thrown:

  1. You see the exception on the View.
  2. You see the exception in Views UI, and cannot edit the view to fix the problem.

#2 is really, really bad--your site is broken and you can't fix it. #1, however, could be the "correct" behavior in some cases, DX-wise. Let's hardcode this catch, message display, and watchdog behavior only for Views UI. For the rendered View, for now, let's let that exception through (or re-throw it if needed), without the error message or watchdog logging.

Eventually, what I think we should do is discuss a complete solution in #1881630: [meta] Determine how to respond to invalid plugins (plugin dependencies). Ideally, what would happen in Views is that we'd collect any and all exceptions together (not just displays'), and then at the end of execution, decide what to do with them based on either the user's configuration or module input. This could range anything from failing silently to hiding the view entirely and displaying an error message.

Edit: But for now, for purposes of development, we want to see these exceptions on render.

Status:Needs work» Needs review
StatusFileSize
new1.84 KB
new4.05 KB
FAILED: [[SimpleTest]]: [MySQL] 54,073 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Removed the watchdog entry, just because it add a lot of entries.

I think we should make it clear why accessing the page still works, and test, that the display is not accessible, when the plugin fails here.

Status:Needs review» Needs work

The last submitted patch, drupal-1879774-36.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new535 bytes
new4.05 KB
PASSED: [[SimpleTest]]: [MySQL] 54,211 pass(es).
[ View ]

Note: If you you aren't sure about 403/404 choose the one you guessed first.

I'm much more comfortable with #38, especially with the test that ensures the invalid display is not accessible. Do we also want to add a similar tests for a block display to ensure that the block is not displayed on a page? Where in the render process does an invalid display plugin result in the view not being rendered normally?

Status:Needs review» Reviewed & tested by the community

There isn't much chance of anything getting rendered, as there would be no display plugin available to render it. So I think we are good. With pages being registered in hook menu, when the router is rebuilt, that display (executeHookMenu) is never called again, as it can;t find it. So really there is no way a view would be accessible, I don't think. This happens way early, when a new display instance is trying to be instantiated.

I would say this is good to go. Let's get this puppy in :) I think I'm ok to rtbc this....

Again, what about block displays?

Status:Reviewed & tested by the community» Needs review

I'm not sure what you're expecting to happen with the invalid displays? The tests originally were just to point out that a message is displayed when we get an invalid type. If a display plugin is not found it can't register much or do anything because no display plugin will be loaded.

Well, I think this sorts of make sense to test the behavior as well. We want to be sure that the behavior never changed?

Agree with #44. If a block has an invalid display plugin, what do we want to happen?

If we want it to do the equivalent of what the page display is now doing, that would be: not return any block, and throw or not throw an exception. So a test should probably test that there is no block wrapper on the page for a block display that uses a broken display plugin, and that the exception is rendered on the page thrown or not depending on our expectation.

But if I the plugin id is not valid, it doesn't really know it's even meant to be a block.

+++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/DisplayTest.phpundefined
@@ -143,4 +143,28 @@ public function testGetAttachedDisplays() {
+    $this->drupalGet('test_display_invalid');
+    $this->assertResponse(200);
+
+    // Change the page plugin id to an invalid one. Bypass the entity system
+    // so no menu rebuild was executed (so the path is still available).
+    $config = config('views.view.test_display_invalid');
+    $config->set('display.page_1.display_plugin', 'invalid');
+    $config->save();
+
+    $this->drupalGet('test_display_invalid');
+    $this->assertResponse(200);
+    $this->assertText(t('The plugin (invalid) did not specify an instance class.'));
+
+    // Rebuild the menu, and ensure that the path is not accessible anymore.
+    menu_router_rebuild();
+
+    $this->drupalGet('test_display_invalid');
+    $this->assertResponse(404);
+++ b/core/modules/views/tests/views_test_config/test_views/views.view.test_display_invalid.ymlundefined
@@ -0,0 +1,30 @@
+  page_1:
+    display_options:
+      path: test_display_invalid
+    display_plugin: page
+    display_title: Page
+    id: page_1
+    position: '0'

@damiankloip, see the existing test for what I mean. Am I completely misunderstanding?

Yep, I originally wrote that test :)

To me, this issue was just about catching that exception. But it seems that ship has sailed, ha.

OK, let's fix this and add the block tests. If we really want to protect people, we will have to change a few other bits of code in the code path for executing a display I think..

I'll post a new patch shortly.

StatusFileSize
new11.72 KB
new13 KB
PASSED: [[SimpleTest]]: [MySQL] 54,350 pass(es).
[ View ]

ok, here goes...

We have to make a few changes, So let's see how this fairs up.

+++ b/core/modules/views/lib/Drupal/views/DisplayBag.phpundefined
@@ -88,10 +88,10 @@ protected function initializePlugin($display_id) {
+    // If no plugin instance has been created, return NULL.
     if (empty($this->pluginInstances[$display_id])) {
-      // Provide a 'default' handler as an emergency. This won't work well but
-      // it will keep things from crashing.
-      $this->pluginInstances[$display_id] = $this->manager->createInstance('default');
+      return NULL;

Won't that break the UI at some point? I would be super conservative here.

+++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/DisplayTest.phpundefined
@@ -165,6 +165,30 @@ public function testInvalidDisplayPlugin() {
+
+    // Change the display plugin ID back to the correct ID.
+    $config = config('views.view.test_display_invalid');
+    $config->set('display.page_1.display_plugin', 'page');
+    $config->save();
+
+    // Place the block display.
+    $block = $this->drupalPlaceBlock('views_block:test_display_invalid-block_1', array(), array('title' => 'Invalid display'));
+
+    $this->drupalGet('<front>');
+    $this->assertResponse(200);
+    $this->assertBlockAppears($block);
+
+    // Change the block plugin ID to an invalid one.
+    $config = config('views.view.test_display_invalid');
+    $config->set('display.block_1.display_plugin', 'invalid');
+    $config->save();
+
+    // Test the page is still displayed, the block not present, and has the
+    // plugin warning message.
+    $this->drupalGet('<front>');
+    $this->assertResponse(200);
+    $this->assertText(t('The plugin (invalid) did not specify an instance class.'));

I'm wondering whether we should put that into the DisplayBlockTest and the code above into the DisplayPageTest and just reference from here via an @see ?

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -688,10 +688,13 @@ public function setDisplay($display_id = NULL) {
+    if ($display = $this->displayHandlers->get($display_id)) {
+      // Set a shortcut.
+      $this->display_handler = $display;
+      return TRUE;
+    }
-    return TRUE;
+    return FALSE;

Yeah that's cool!

+++ b/sites/default/default.settings.phpundefined
@@ -444,14 +444,6 @@
/**
- * Mixed-mode sessions:
- *
- * Set to TRUE to create both secure and insecure sessions when using HTTPS.
- * Defaults to FALSE.
- */

Don't hack core ;)

StatusFileSize
new13.01 KB
PASSED: [[SimpleTest]]: [MySQL] 55,834 pass(es).
[ View ]

Just a reroll for now.

Won't that break the UI at some point? I would be super conservative here.

The trouble is it's really difficult to know when rendering a block , for example that things have failed if it always returns the default display. It get tricky to know that that is not the display instance we wanted.

I will see how the reroll goes; then moving the block tests sounds like a good plan.

The trouble is it's really difficult to know when rendering a block , for example that things have failed if it always returns the default display. It get tricky to know that that is not the display instance we wanted.

This seems to be better then having a failing UI on which nothing can be fixed at all :(

+++ b/core/modules/views/lib/Drupal/views/DisplayBag.phpundefined
@@ -77,11 +78,20 @@ protected function initializePlugin($display_id) {
+      return NULL;

So what about somehow adding a flag whether we are in the UI or on the actual site?

We could add a property to the actual default display? like isFallback or something? Don;t take that name too seriously.

Whatever doesn't break the UI is great.

StatusFileSize
new1.48 KB
new14.53 KB
FAILED: [[SimpleTest]]: [MySQL] 55,910 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

OK, let's make sure we can get an instance of a display before rendering the tab too. So any invalid display plugin types will not get a tab. Nice :)

Status:Needs review» Needs work

The last submitted patch, 1879774-56.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.57 KB
PASSED: [[SimpleTest]]: [MySQL] 55,931 pass(es).
[ View ]

Rerolled.

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewTestBase.phpundefined
@@ -224,6 +225,43 @@ protected function executeView($view, $args = array()) {
+    $config_id = explode('.', $block->id());
+    $machine_name = array_pop($config_id);

It feels wrong to not use list() here.

I have seen it working on damians laptop, so a broken views config does not break the views UI, but maybe a screenshot of the behavior would be cool.

Status:Needs review» Reviewed & tested by the community

Forgot the rtbc

StatusFileSize
new65.68 KB

GOod plan, here is a screen grab when (for the sake of argument, the page plugin name was changed to 'pag'). You can see the message from the exception being set but also the page display tab is not available. So for instance, this would work just the same for disabled moduled. E.g. if you created a rest export display on a view but then disabled the rest module.

Screen Shot 2013-05-21 at 16.48.22.png

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

StatusFileSize
new14.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1879774-63.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled.

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks.

Status:Fixed» Reviewed & tested by the community

I rerolled the other patch. Unfortunatly, the patch in #63 no longer applies. Needs a re-roll.

#63: 1879774-63.patch queued for re-testing.

StatusFileSize
new14.59 KB
PASSED: [[SimpleTest]]: [MySQL] 57,303 pass(es).
[ View ]

Rerolled.

StatusFileSize
new14.59 KB
PASSED: [[SimpleTest]]: [MySQL] 56,107 pass(es).
[ View ]

Bot seems stuck on #68. Reuploading to see if that helps.

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks.

I'm not 100% sure this is related to this issue, but I think it is, so I'm posting it here. Feel free to direct me elsewhere, if I'm wrong.

I did a brand new install with D8 code downloaded less than an hour ago (which means the committed changes from this issue are in it) into a new database, and then performed the following steps:

  • Created a view (of users) with a block display
  • Placed that block in a region
  • Deleted the view
  • Displayed or refreshed a page that has the block on it

I get a WSOD. (When I did this last night, I was also getting an error message, but I'm not getting it now, so I can't post it).

I get this from Apache:
PHP Fatal error: Call to a member function access() on a non-object in /.../core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.php on line 56

If I delete the associated .yml file, everything is fine again.

Hi, Thanks for reporting! It's good that people are actually testing this stuff :)

I guess this is related, but this issue was specifically for dealing with displays that were not available. What you have just pointed out is definitely a problem though. If you also disable a view, you should see some problems too.

Let's create a new issue for that.

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