Problem/Motivation

Currently, Display plugins have a getStyleType() method. This is used to filter style and row plugins by a type specified by this method. So if the type is 'data', only plugins with either no 'type' or a type of 'data' will be listed. (This has been something I've wanted to add to views in 7.x-3.x but let's just add it here first. Then other plugins, like access, cache, exposed_form plugins etc.. can also use this.)

An example of this working in its current form is for the REST Export display plugin; This declares a type of 'data', the serializer plugin (and row plugins) have this declared in their plugin definition, so on they are available when using that display.

Currently, in most cases, Views displays an unfiltered plugin list E.g. access plugins, which results in plugins being listed in contexts where they should not be used. For example, admin_views in D7 has a custom access plugin that only really works when using the system display type. However, there is no way to filter access plugins. So the system display shows all types of access plugins, and the admin views access callback is available on all other display types.

Proposed resolution

Use the existing method more consistently so that [example].

Details

  • Rename the getStyleType() method to getDisplayType(), I think this makes more sense.
  • Change the logic slightly in views_fetch_plugin_names() to check if 'type' is set.
  • Pass this method to more views_fetch_plugin_names() calls to filter plugins.

Remaining tasks

Fix it.

User interface changes

More finely filtered plugin listings in certain cases.

API changes

Changed method name: getStyleType() => getDisplayType().
More flexibility for plugins and their definitions (in the UI).

Initial patch to get things moving and test the water attached.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, vdc.getDisplayType.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
16.41 KB
24.74 KB

or.. we could make the 'type' key a 'types' array instead?

damiankloip’s picture

Issue summary: View changes

Updated issue summary.

damiankloip’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, 1964348-2.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
822 bytes
25.55 KB

The default argument form for arguments was doing its own thing to filter summary style plugins.

Status: Needs review » Needs work

The last submitted patch, interdiff-1964348-4.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
dawehner’s picture

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -1472,7 +1472,7 @@ public function buildOptionsForm(&$form, &$form_state) {
-          '#options' => views_fetch_plugin_names('access', NULL, array($this->view->storage->get('base_table'))),
+          '#options' => views_fetch_plugin_names('access', $this->getDisplayType(), array($this->view->storage->get('base_table'))),

@@ -1507,7 +1507,7 @@ public function buildOptionsForm(&$form, &$form_state) {
-          '#options' => views_fetch_plugin_names('cache', NULL, array($this->view->storage->get('base_table'))),
+          '#options' => views_fetch_plugin_names('cache', $this->getDisplayType(), array($this->view->storage->get('base_table'))),

@@ -1598,7 +1598,7 @@ public function buildOptionsForm(&$form, &$form_state) {
-          '#options' => views_fetch_plugin_names('style', $this->getStyleType(), array($this->view->storage->get('base_table'))),
+          '#options' => views_fetch_plugin_names('style', $this->getDisplayType(), array($this->view->storage->get('base_table'))),

It's nice that we support it in general on all the places, but don't have a requirement, so you can still have a plugin type for all of the display types.

I'm wondering whether we can come up with a better name then display type, maybe just getType? The problem with displayType is that, at least for myself, it reminds me a lot of the display plugin id.

damiankloip’s picture

FileSize
6.9 KB
25.46 KB

Yep, that sounds ok to me. Lets change it to getType()

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -2556,7 +2556,7 @@ function preview() {
+  protected function getType() { return 'normal'; }

Would be cool to have this code style thing fixed.

xjm’s picture

FileSize
870 bytes
25.46 KB

You guys have very interesting concepts of RTBC. ;)

xjm’s picture

Status: Reviewed & tested by the community » Needs work

So, a couple minor things to fix on this patch. First of all, I uploaded the wrong patch in #10. Whoops! Applying my interdiff on top of the patch there should work.

Then, two things:

+++ b/core/modules/views/tests/views_test_data/lib/Drupal/views_test_data/Plugin/views/display/DisplayTest.phpundefined
@@ -31,6 +31,13 @@ class DisplayTest extends DisplayPluginBase {
+   * Overrides \Drupal\views\Plugin\views\display\DisplayPluginBase::getDisplayType().
+   */
+  protected function getType() {
  1. These docblocks currently don't match the method name. Maybe a good opportunity to @inheritdoc?
  2. @webchick pointed out that DisplayPluginBase::getType() has pretty much the same problem as DisplayPluginBase::getDisplayType(), and I agree, so let's brainstorm a bit more for a better method name.
dawehner’s picture

Status: Needs work » Reviewed & tested by the community

Ups

dawehner’s picture

Just some ideas thrown out: getTag() getDisplayTag() getCategory() getDisplayCategory()

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Reading #11 this doesn't sound RTBC and can we get one patch to apply :)

xjm’s picture

Status: Needs review » Needs work

"Display category" is probably a better name for the... thing. So getDisplayCategory() might work.

And yes, it's NW. :)

damiankloip’s picture

Status: Needs work » Needs review
FileSize
7.17 KB
25.77 KB

That sounds good, let's go with getDisplayCategory().

dawehner’s picture

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -2553,10 +2553,14 @@ function preview() {
+   * Returns the plugin type that this display requires.
...
+   *   The required plugin type. Defaults to 'normal'.
...
+  protected function getDisplayCategory() {
+    return 'normal';
+  }

Should we maybe change this parts as well?

damiankloip’s picture

FileSize
1.11 KB
26.13 KB

Yep, Let's revamp that docblock, it could do with a description probably to explain its usage.

damiankloip’s picture

FileSize
891 bytes
26.13 KB

Tiny things :)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I'm not a grammer nazi, so this looks fine for me. Thanks for updating and actually improving the docs!

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Ok, sorry. I did indeed express confusion in #11, but it was in reference to #7's assertion that "The problem with displayType is that, at least for myself, it reminds me a lot of the display plugin id." Since "getDisplayType" and "getType" are effectively synonyms, I didn't quite understand how that was better. (Though to be fair, I also don't understand how someone could ever mistaken a "display type" for a "display plugin ID". I'd expect that function to be called $display->getPluginID() or similar. :))

However, I don't think this patch flies, because now we just made two different names for the same thing: on row plugins they're referring to "types" but on displays they're called "category." That's going to screw people up, IMO.

Since Fields/Widgets/Formatters have a similar construct, where fields can declare themselves as e.g. "text" and widgets can register as only showing up on "text" fields, we should probably look to that for a pattern to employ here.

That would mean the annotation gets renamed to "display_types" and the method on the display gets renamed to getType(). (probably, since once fields are converted to CMI in #1735118: Convert Field API to CMI they get a simple "type: text" property on them.)

IOW, now that I understand this a bit better (thanks, Tim!), I think #10 was pretty close to what we want. :\ Sorry for my accidental derailment. :\

damiankloip’s picture

Status: Needs work » Needs review
FileSize
22.89 KB
25.48 KB

*shakes fist at webchick*

Ok, so here is a reroll moving it back to use getType() and updating the plugin annotations to use display_types. I think we're good again :)

Status: Needs review » Needs work

The last submitted patch, 1964348-22.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
848 bytes
25.49 KB

Sorry, missed a spot.

At least it's good to know that views test coverage picks these things up nowadays :) I think we're getting somewhere...

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Now we seem to be all convinced, that this is a good naming. One thing I particular like is the fact, that we refer to the display in the other plugin types.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, that works! Thanks a lot.

Committed and pushed to 8.x.

dawehner’s picture

Awesome thank you!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

Jaesin’s picture

Follow up added #2562811: Make \Drupal\views\Plugin\views\display\DisplayPluginBase::getType() public and add it to the interface. to make getType public and add it to DisplayPluginInterface.