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:

  1. Create a view with 2 page displays, change the machine name of the second display from page_2 to page in the advanced settings.
  2. After saving, add an attachment display to both pages.
  3. Navigate to the url of the first display. The attachment is shown.
  4. 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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mirroar created an issue. See original summary.

Mirroar’s picture

Status: Active » Needs review
FileSize
766 bytes

Here 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. :)

Mirroar’s picture

Issue summary: View changes
Issue tags: +Needs tests
ckaotik’s picture

Version: 8.3.x-dev » 8.4.x-dev

Still necessary on 8.4, patch applies with offset 2.

dani3lr0se’s picture

I 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. :)

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mvantuch’s picture

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

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

acbramley’s picture

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

The last submitted patch, 9: 2903831-9--failing-test.patch, failed testing. View results

jibran’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -361,9 +361,7 @@ public function useMoreText() {
-    // To be able to accept attachments this display have to be able to use
-    // attachments but at the same time, you cannot attach a display to itself.

Can we please add a test to make sure attachment is not getting attached to itself?

Simon Georges’s picture

I can confirm the patch works for me (and this bug is really frustrating ;-)).

Lendude’s picture

Status: Needs work » Needs review
FileSize
590 bytes
1.51 KB

This adds a case for attaching the attachment to the attachment itself. This is also caught by the same usesAttachments check.

quicksketch’s picture

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

quicksketch’s picture

Same patch as #14 with the tests amended.

The last submitted patch, 14: 2903831-14-views-attachment.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 15: 2903831-15-views-attachment.patch, failed testing. View results

longwave’s picture

Status: Needs work » Needs review

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

longwave’s picture

Version: 8.6.x-dev » 8.8.x-dev
Status: Needs review » Reviewed & tested by the community
FileSize
1.51 KB

Reuploading #13 and marking RTBC.

longwave’s picture

I 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?

natts’s picture

Did 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?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 2903831-13.patch, failed testing. View results

longwave’s picture

Status: Needs work » Reviewed & tested by the community

@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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I 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!

  • alexpott committed 456ea85 on 9.0.x
    Issue #2903831 by quicksketch, acbramley, Lendude, longwave, Mirroar,...

  • alexpott committed ca42948 on 8.9.x
    Issue #2903831 by quicksketch, acbramley, Lendude, longwave, Mirroar,...

  • alexpott committed ee70ee4 on 8.8.x
    Issue #2903831 by quicksketch, acbramley, Lendude, longwave, Mirroar,...

Status: Fixed » Closed (fixed)

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