If you try to save the tab options form, you will have no joy.

The validation falls through to displayPluginBase::validateOptionsForm when it assumes any section with _options appended, such as style_options, tab_options is a plugin type. This is true for style but not for tab. So it then goes on to try and find a plugin manager for 'plugin.manager.views.tab' which does not exist.

We need to stop this from happening, so we can either check this is a plugin type, or change the logic a bit more. I think the first is fine. Also needs a UI test for the tab options...

Comments

damiankloip’s picture

Status: Active » Needs review
StatusFileSize
new2.39 KB
new1.63 KB

Status: Needs review » Needs work

The last submitted patch, 2012502-with-fix.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
StatusFileSize
new990 bytes
new2.5 KB
new1.74 KB

Waaaa, Let's try that again.

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

The last submitted patch, 2012502-3.patch, failed testing.

damiankloip’s picture

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

#3: 2012502-3.patch queued for re-testing.

damiankloip’s picture

StatusFileSize
new1.89 KB
new2.59 KB

Let's actually expose some failures ;)

damiankloip’s picture

StatusFileSize
new2.59 KB
new1.81 KB

Sorry, just tests and tests with fix. interdiff from #6 is still correct.

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

The last submitted patch, 2012502-7.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review

#7: 2012502-7.patch queued for re-testing.

damiankloip’s picture

#7: 2012502-7-tests-only.patch queued for re-testing.

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

The last submitted patch, 2012502-7.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
StatusFileSize
new1.96 KB
new3.01 KB
new1.82 KB

OK, Pull yourself together. This is really not a hard patch...

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

The last submitted patch, 2012502-12.patch, failed testing.

damiankloip’s picture

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

#12: 2012502-12.patch queued for re-testing.

damiankloip’s picture

Priority: Normal » Major

I think this is major.

dawehner’s picture

index 07423cb..b6f56af 100644
--- a/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php

--- a/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -811,13 +811,20 @@ public function usesFields() {

@@ -811,13 +811,20 @@ public function usesFields() {
   public function getPlugin($type) {
     // Look up the plugin name to use for this instance.
     $options = $this->getOption($type);
-    $name = $options['type'];
+
+    // Return now if no options have been loaded.
+    if (empty($options) || !isset($options['type'])) {
+      return;
+    }
 
     // Query plugins allow specifying a specific query class per base table.
     if ($type == 'query') {
       $views_data = Views::viewsData()->get($this->view->storage->get('base_table'));
       $name = isset($views_data['table']['base']['query_id']) ? $views_data['table']['base']['query_id'] : 'views_query';
     }
+    else {
+      $name = $options['type'];

Some dedicated test here could help.

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/DisplayPath.phpundefined
@@ -55,4 +55,31 @@ public function testDeleteWithNoPath() {
+    $this->drupalPost("admin/structure/views/nojs/display/test_view/page_1/path", array('path' => $this->randomString()), t('Apply'));

Nitpick ... this should use single quotes ...

damiankloip’s picture

StatusFileSize
new3.01 KB
new6.63 KB

Good idea, Let's add some tests for getPlugin(). This should cover the functionality.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great! I assume it will be green.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/DisplayUnitTest.phpundefined
@@ -87,7 +93,35 @@ public function testDefaultOptions() {
+    // Get a plugin twice, and make sure the same instance is returned.
+    $view->destroy();
+    $view->initDisplay();
+    $first = spl_object_hash($display_handler->getPlugin('style'));
+    $second = spl_object_hash($display_handler->getPlugin('style'));
+    $this->assertIdentical($first, $second, 'A cached plugin instance was returned.');

The assertion message here does not seem correct...

damiankloip’s picture

@alexpott, what do you think this should say? 'A statically cached plugin instance was returned', 'The same plugin instance was returned'?

damiankloip’s picture

Assigned: Unassigned » alexpott
Status: Needs work » Needs review
StatusFileSize
new739 bytes
new6.63 KB

@alexpott, rerolled with that comment tweak.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

The new assertion message looks fine.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4b815b3 and pushed to 8.x. Thanks!

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