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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

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

damiankloip’s picture

FileSize
3.73 KB

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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

damiankloip’s picture

FileSize
788 bytes
3.75 KB

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

damiankloip’s picture

FileSize
3.77 KB

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

xjm’s picture

dawehner’s picture

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

xjm’s picture

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

damiankloip’s picture

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.

xjm’s picture

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

dawehner’s picture

FileSize
2.74 KB

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.

dawehner’s picture

FileSize
3.68 KB

Forgot the yml file.

tim.plunkett’s picture

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.

dawehner’s picture

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

damiankloip’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.22 KB
3.69 KB

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.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.02 KB
3.63 KB

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.

damiankloip’s picture

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

damiankloip’s picture

FileSize
1.67 KB
3.8 KB

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

dawehner’s picture

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

damiankloip’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.66 KB

Rerolled the patch, though I still get the exception.

Status: Needs review » Needs work

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

dawehner’s picture

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

damiankloip’s picture

Status: Needs work » Needs review

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

damiankloip’s picture

FileSize
3.83 KB

rerolled.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

xjm’s picture

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.

tim.plunkett’s picture

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

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

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

This was already RTBC.

xjm’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.84 KB
4.05 KB

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
535 bytes
4.05 KB

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

xjm’s picture

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?

damiankloip’s picture

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

xjm’s picture

Again, what about block displays?

xjm’s picture

Status: Reviewed & tested by the community » Needs review
damiankloip’s picture

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.

dawehner’s picture

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

xjm’s picture

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.

damiankloip’s picture

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

xjm’s picture

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

damiankloip’s picture

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.

damiankloip’s picture

ok, here goes...

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

dawehner’s picture

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

damiankloip’s picture

FileSize
13.01 KB

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.

dawehner’s picture

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 :(

dawehner’s picture

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

damiankloip’s picture

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

dawehner’s picture

Whatever doesn't break the UI is great.

damiankloip’s picture

FileSize
1.48 KB
14.53 KB

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.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
14.57 KB

Rerolled.

dawehner’s picture

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Forgot the rtbc

damiankloip’s picture

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

dcam’s picture

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.

damiankloip’s picture

FileSize
14.58 KB

Rerolled.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

longwave’s picture

Status: Fixed » Reviewed & tested by the community
Dries’s picture

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

xjm’s picture

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

damiankloip’s picture

FileSize
14.59 KB

Rerolled.

effulgentsia’s picture

FileSize
14.59 KB

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

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

oadaeh’s picture

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.

damiankloip’s picture

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.