Problem/Motivation
When you adjust a views display's machine name to match its plugin id (e.g. have a page display named page
), attachments will no longer be displayed with this view. This is clearly not intended behavior.
One way to reproduce:
- Create a view with 2 page displays, change the machine name of the second display from
page_2
topage
in the advanced settings. - After saving, add an attachment display to both pages.
- Navigate to the url of the first display. The attachment is shown.
- Navigate to the url of the second display. The attachment is not shown.
Proposed resolution
The culprit here is DisplayPluginBase::acceptAttachments(). The comparison of $this->definition['id'] == $this->view->current_display
compares a display name with a plugin id, which causes this unintended behavior.
The intention of the code probably was to check the display name of the attachment against that of the display being rendered, as the comment above the check states. However, the method documentation states that it "Determines whether this display can use attachments.", so this check should not even be made here, and we do not have any information about which displays would be attached anyway.
Therefore, I think the check can be removed completely, especially since it always returns FALSE
when using default display machine names like page_1
.
Aditionally, the views UI does not allow a user to attach an attachment to another attachment anyway, let alone itself.
For now, a workaround would be to rename the display in question in order to have attachments show up again (Don't forget to check the "attach to" setting of you attachment after renaming the display machine name).
Remaining tasks
We should probably add a test that catches this bug in the current version to prevent regression.
Comment | File | Size | Author |
---|---|---|---|
#19 | 2903831-13.patch | 1.51 KB | longwave |
#15 | 2903831-15-views-attachment.patch | 1.54 KB | quicksketch |
#14 | 2903831-14-views-attachment.patch | 803 bytes | quicksketch |
#13 | 2903831-13.patch | 1.51 KB | Lendude |
#13 | interdiff-2903831-9-13.txt | 590 bytes | Lendude |
Comments
Comment #2
Mirroar CreditAttribution: Mirroar at werk21 commentedHere is the proposed patch to see if it breaks anything currently. I'm not sure when I'll have time to add tests yet, so if somebody finds the time, that would be great. :)
Comment #3
Mirroar CreditAttribution: Mirroar at werk21 commentedComment #4
ckaotikStill necessary on 8.4, patch applies with offset 2.
Comment #5
dani3lr0se CreditAttribution: dani3lr0se at Hook 42 commentedI hope this is still necessary, but I tested via simplytest.me on 8.4.x, and the patch worked great. I recreated a view with attachment displays using the steps above. When I changed the machine name of "page_2" to "page" and created an attachment, I still had the option to "attach to" "page 2" based on the display name. I can confirm that the attachment display is shown on both pages. Again, my apologies if this was already taken care of. I couldn't really tell if the patch still needed to be reviewed or if it was just waiting on tests. I'll leave as "Needs review" since it's tagged as "Needs test". I'm learning how to write tests and would be interested in giving this a try, but if someone beats me to it, it's all good. Thanks for the patch. :)
Comment #7
mvantuch CreditAttribution: mvantuch commentedWow, this one was reallly annoying to come accross. If you get unlucky, view attachments simply don't work at all.
For me it simply failed when I tried attaching to a page with id
page
.Comment #9
acbramley CreditAttribution: acbramley at PreviousNext for University of Adelaide commentedYup, just lost a few hours trying to debug this. I agree that removing the check is the right approach as when choosing which displays to attach an attachment to, the option for the display you are attaching isn't there (if that makes sense??).
Here's a change to an existing test to show the failure, and a patch with the test change and fix included.
Comment #11
jibranCan we please add a test to make sure attachment is not getting attached to itself?
Comment #12
Simon Georges CreditAttribution: Simon Georges at Makina Corpus commentedI can confirm the patch works for me (and this bug is really frustrating ;-)).
Comment #13
LendudeThis adds a case for attaching the attachment to the attachment itself. This is also caught by the same usesAttachments check.
Comment #14
quicksketchI think what @jibran was requesting is that the code comments specify a situation that still needs to be handled: you shouldn't be able to attach a display to itself.
It looks like the property used was simply incorrect. Instead of
$this->definition['id']
(which would be the display type like "page", "attachment", etc.) it should be$this->display['id']
(which is typically "page_1", "attachment_1", etc.)Comment #15
quicksketchSame patch as #14 with the tests amended.
Comment #18
longwaveI think #13 is correct. Attachments and feeds implement attachTo() but don't set $usesAttachments. Protecting against a custom plugin that both implements attachTo() and sets $usesAttachments = TRUE seems to be an extreme edge case, and any protection would have to be in attachTo() anyway as far as I can see. #14/#15 fail I think because the "current display" isn't necessarily what you expect at the time that acceptAttachments() is called; it is too early to know which display is actually being attached.
Comment #19
longwaveReuploading #13 and marking RTBC.
Comment #20
longwaveI guess that all the $uses* flags in DisplayPluginBase should be interfaces instead, is it worth opening an issue for this, or is this just going to be a huge BC break?
Comment #21
nattsDid this make it into the 8.8.0 release? I couldn't see it listed on any of the release notes (alpha, beta, RC1, release).
If not, is it scheduled for a future release?
Comment #23
longwave@natts No this is not part of any release yet.
Tests failed with "RuntimeException: Unable to start the web server.", retesting and re-marking RTBC
Comment #24
alexpottI agree with #18 catering for the edge case of something that can be an attachment and also uses attachments feels unnecessary especially when it causes this bug - which is far easier to come across.
Committed and pushed 456ea857fe to 9.0.x and ca42948626 to 8.9.x and ee70ee4003 to 8.8.x. Thanks!