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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

Status: Active » Needs review
FileSize
2.39 KB
1.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
FileSize
990 bytes
2.5 KB
1.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

FileSize
1.89 KB
2.59 KB

Let's actually expose some failures ;)

damiankloip’s picture

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
FileSize
1.96 KB
3.01 KB
1.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

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
FileSize
739 bytes
6.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.