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.
Comment | File | Size | Author |
---|---|---|---|
#24 | 1964348-24.patch | 25.49 KB | damiankloip |
#24 | interdiff-1964348-24.txt | 848 bytes | damiankloip |
#22 | 1964348-22.patch | 25.48 KB | damiankloip |
#22 | interdiff-1964348-22.txt | 22.89 KB | damiankloip |
#19 | 1964348-19.patch | 26.13 KB | damiankloip |
Comments
Comment #2
damiankloip CreditAttribution: damiankloip commentedor.. we could make the 'type' key a 'types' array instead?
Comment #2.0
damiankloip CreditAttribution: damiankloip commentedUpdated issue summary.
Comment #2.1
damiankloip CreditAttribution: damiankloip commentedUpdated issue summary.
Comment #3.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #4
damiankloip CreditAttribution: damiankloip commentedThe default argument form for arguments was doing its own thing to filter summary style plugins.
Comment #6
xjmComment #7
dawehnerIt'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.
Comment #8
damiankloip CreditAttribution: damiankloip commentedYep, that sounds ok to me. Lets change it to
getType()
Comment #9
dawehnerWould be cool to have this code style thing fixed.
Comment #10
xjmYou guys have very interesting concepts of RTBC. ;)
Comment #11
xjmSo, 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:
DisplayPluginBase::getType()
has pretty much the same problem asDisplayPluginBase::getDisplayType()
, and I agree, so let's brainstorm a bit more for a better method name.Comment #12
dawehnerUps
Comment #13
dawehnerJust some ideas thrown out: getTag() getDisplayTag() getCategory() getDisplayCategory()
Comment #14
alexpottReading #11 this doesn't sound RTBC and can we get one patch to apply :)
Comment #15
xjm"Display category" is probably a better name for the... thing. So
getDisplayCategory()
might work.And yes, it's NW. :)
Comment #16
damiankloip CreditAttribution: damiankloip commentedThat sounds good, let's go with getDisplayCategory().
Comment #17
dawehnerShould we maybe change this parts as well?
Comment #18
damiankloip CreditAttribution: damiankloip commentedYep, Let's revamp that docblock, it could do with a description probably to explain its usage.
Comment #19
damiankloip CreditAttribution: damiankloip commentedTiny things :)
Comment #20
dawehnerI'm not a grammer nazi, so this looks fine for me. Thanks for updating and actually improving the docs!
Comment #21
webchickOk, 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. :\
Comment #22
damiankloip CreditAttribution: damiankloip commented*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 :)
Comment #24
damiankloip CreditAttribution: damiankloip commentedSorry, 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...
Comment #25
dawehnerNow 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.
Comment #26
webchickOk, that works! Thanks a lot.
Committed and pushed to 8.x.
Comment #27
dawehnerAwesome thank you!
Comment #28.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #29
Jaesin CreditAttribution: Jaesin commentedFollow 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.