Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
The 'Search Results' row plugin is always available, regardless of the display type. This should work correctly with display type filtering.
E.g.
If you try to use this in a feed view, it will break too.
Steps to reproduce
- Go to Structure > Views > Taxonomy term > Edit > Feed
- Select the link "Show: Content"
- Expected: The "Search results" format should not be available.
- Actual: The option is available, and the resulting preview prints invalid markup containing
ArrayArray
The same issue can also be seen for pagers right now. Again, on the feed display, all pagers are available whereas only 'basic' ('Display all items' and 'Display a specified number of items') type pagers should be available as usesPager() for the display is false.
Proposed resolution
Add a 'display_types' default value of ['normal'] to the ViewsPluginManager defaults. This will improve DX (otherwise you will always have to specify at least display_types = {"normal"} in your annotations).
We need to do this after the regular default values have been merged. Otherwise we will get arrays being concatenated during the deep merging. I.e. 'normal' will be added to every display_types array.
This can then be overridden like RestExport or Feeds do, or set to NULL if no type really is desired (this will be rare).
Remaining tasks
User interface changes
None
API changes
None
| Comment | File | Size | Author |
|---|---|---|---|
| #34 | interdiff_28-34.txt | 855 bytes | Ratan Priya |
| #34 | 2401637-34.patch | 5.75 KB | Ratan Priya |
| #31 | 2401637-after_patch.png | 48.56 KB | Abhijith S |
| #31 | 2401637-before_patch.png | 56.71 KB | Abhijith S |
| #29 | Screenshot after patch.png | 269.95 KB | ambikahirode |
Issue fork drupal-2401637
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedComment #2
dawehnerIt is too bad that we have to do that, but meh
We can now use trailing commas, isn't that awesome?
Comment #3
damiankloip CreditAttribution: damiankloip commented1. Yeah, it is that, or having some custom default handling in the the annotation classes. This feels like the better place to put this tbh.
2. Yeah, I know I know - we have been able to do this for some time. I just thought we might as well take them out to follow suit?
Let's look at some tests for this.
Comment #4
damiankloip CreditAttribution: damiankloip commentedComment #5
damiankloip CreditAttribution: damiankloip commentedHow about a quick assertion like this? To test the search specific bug in there we would need to also enable search in that test, just for the current missing display_type, so once the patch is it, it's a useless dependency anyway.
Comment #8
baisongNotes from triage with @metzlerd
Comment #9
xjm(Saving proposed issue credit for discussion and triage participants at LA.)
Comment #10
damiankloip CreditAttribution: damiankloip commentedThis is definitely still an issue. Having styles showing up for displays they don't work with is a huge WTF.
Comment #11
LendudeNeeded a reroll.
Currently, the wrong styles still show up in the list, but I don't see the ArrayArray stuff showing up in the markup when I select the search result option for the taxonomy feed.
But +1 for showing only stuff that's useful.
Comment #12
damiankloip CreditAttribution: damiankloip commentedYeah, the ArrayArray problem was never anything to do with this issue, so that's cool. I guess it was fixed with all the translatable markup stuff before 8.0.0 release.
Re-roll looks good to me, thanks Lendude! Another issue I forgot about!
Comment #15
cilefen CreditAttribution: cilefen commented@timplunkett, @alexpott, @dawehner, @xjm and I discussed this in a triage session at DrupalCon Dublin. We agree this is not ideal but if somebody uses this plugin in the wrong context the result is something weird but they should not be using the plugin in that context in the first place because it makes no sense. So, yes, it's a bug, but it isn't major.
Comment #25
smustgrave CreditAttribution: smustgrave at Mobomo commentedConfirmed this is still an issue. Needs a reroll
Comment #26
karishmaamin CreditAttribution: karishmaamin at Specbee commentedRe-rolled patch against 9.5.x.Please review
Comment #28
smustgrave CreditAttribution: smustgrave at Mobomo commentedFixing test case
Comment #29
ambikahirode CreditAttribution: ambikahirode as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedPatch in #28 working Fine on local drupal 9.5
Comment #30
Manibharathi E R CreditAttribution: Manibharathi E R at Srijan | A Material+ Company for Srijan | A Material+ Company commentedPatch #28 working Fine on local Drupal 9.5.
Comment #31
Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commentedApplied patch #28 on 9.5.x and it works fine.
Before patch:
After patch:
Comment #32
smustgrave CreditAttribution: smustgrave at Mobomo commentedIf you feel the issue is fixed please move to RTBC
Comment #33
borisson_,.only one should be enough I think ;)Comment #34
Ratan Priya CreditAttribution: Ratan Priya at OpenSense Labs commented@borisson_ ,
I made the changes you required at comments #33
Needs review.
Comment #35
smustgrave CreditAttribution: smustgrave at Mobomo commentedlooks like isse #33 has been addressed
Comment #36
quietone CreditAttribution: quietone at PreviousNext commentedRemoved credit for second set of screenshots on the same patch, per How is credit granted for Drupal core issues.
Comment #38
borisson_Not sure why the bot put this back to NW, last patch is still green.
Comment #39
alexpottAre we sure about the way we've fixed this? The current fix I think means that all display plugins in contrib have have
display_types = {"normal"}in their annotation will have to be changed. If that's the fix then we need to issue a deprecation telling them that. However if we do that then I question why we don't fixed this automatically for them.Comment #42
acbramley CreditAttribution: acbramley at PreviousNext commentedPatch #34 still applies to 11.x with slight fuzz but would be good to get this into MR form.
Before that happens though I think we still need an answer for #39
Comment #44
LendudeThe current fix changes way more than just the row plugins, as pointed out in the IS, this also affects things like pagers. This isn't necessarily a bad thing but we do need to check if we are not hiding things we don't want hidden and maybe expand the test coverage to also cover other plugin types.
For #39, I don't think plugins that have
display_types = {"normal"}set need to change anything, it just becomes redundant, because we are addingdisplay_types = {"normal"}for all plugins that don't set anything. I think....But yeah changing this in ViewsPluginManager feels a little bit more invasive than it did when this was opened, I feel we do need a clearer list of all affected plugin types if we do it in ViewsPluginManager