Problem/Motivation

Dropbuttons uses $button = \Drupal::service('renderer')->renderPlain($element[$key]); which lets it lose metadata.

This can cause the following problems:

  • People seeing links they should not be able to see
  • Links might appear in the wrong language?

Proposed resolution

Ensure the cacheablity metadata of those links is bubbled up properly.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#19 2718655-19.patch2.89 KBjofitz
#11 interdiff.txt529 bytesdawehner
#11 2718655-11.patch2.9 KBdawehner
#9 interdiff.txt498 bytesdawehner
#9 2718655-9.patch62.28 KBdawehner
#7 interdiff.txt1.88 KBdawehner
#7 2718655-2.patch2.88 KBdawehner
#2 2718655-2.patch1 KBdawehner

Comments

dawehner created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new 2718655-2.patch1 KB

This issue came up as part of #2718635: Use dropbutton to select bundle to add

dawehner’s picture

dawehner
Primary language German
commented
Related issues: +#2718635: Use dropbutton to select bundle to add
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +D8 cacheability
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2495779: Make #theme => links take cacheability metadata as an argument

Looks correct. Still needs tests indeed.

This is another consequence of having some of the crazy render elements, like this one. Somewhat reminds me of #2495779: Make #theme => links take cacheability metadata as an argument.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

NW for tests.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 2718655-2.patch2.88 KB
new interdiff.txt1.88 KB

Here is some test coverage

Status: Needs review » Needs work

The last submitted patch, 7: 2718655-2.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new 2718655-9.patch62.28 KB
new interdiff.txt498 bytes

Ts

Status: Needs review » Needs work

The last submitted patch, 9: 2718655-9.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new 2718655-11.patch2.9 KB
new interdiff.txt529 bytes

Weird

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

dawehner
Primary language German
commented

Anyone wants to share reviews?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Makes sense, Wim already +1'd before, not a huge fan of those form-test mix classes, but looks ok and covers the bug.

This might also help with #2473873: Entity operations lack cacheability support, resulting in incorrect dropbuttons

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

cilefen’s picture

cilefen commented
Title: #type actions with dropbutton looses bubbleable metadata » #type actions with dropbutton loses bubbleable metadata
Issue summary: View changes

Fixed loose vs lose ;-).

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 8.4.x-dev » 8.3.x-dev
Issue tags: +Needs issue summary update

Looks like a patch-safe bugfix on a quick scan. Thanks!

Is this major? IS could use more info on the implications and consequences. Leaving RTBC though.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 2718655-11.patch, failed testing.

jofitz’s picture

jofitz
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new 2718655-19.patch2.89 KB

Re-rolled.

dawehner’s picture

dawehner
Primary language German
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I am not sure about all the possible consequences, but at least I sort of came up with two of them.

  • alexpott committed 6b62f6b on 8.4.x 
    Issue #2718655 by dawehner, Jo Fitzgerald, Wim Leers: #type actions with...

  • alexpott committed b5fbc4d on 8.3.x 
    Issue #2718655 by dawehner, Jo Fitzgerald, Wim Leers: #type actions with...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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