Problem/Motivation
See http://drupal.stackexchange.com/questions/175697/menu-callback-and-csrf/... and #2541166: Browse available tokens not shown.
The problem is that Twig template variables that implement either CacheableDependencyInterface
(i.e. they carry cacheability metadata to bubble) or AttachmentsInterface
(i.e. they carry attachments to bubble) don't have their metadata bubbled. Twig just uses their strings, and that's it.
Proposed resolution
Just before actually using/printing these Twig template variables, do the necessary bubbling, much like we do in ThemeManager
.
Remaining tasks
Patch.Manual testing.- Test coverage.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#18 | interdiff.txt | 1.6 KB | dawehner |
#18 | 2575519-18.patch | 9.48 KB | dawehner |
#16 | interdiff.txt | 8.12 KB | dawehner |
#16 | 2575519-16.patch | 9.64 KB | dawehner |
#14 | 2575519-14.patch | 8.21 KB | alexpott |
Comments
Comment #2
mgiffordHow would this be tested, just with Core?
Comment #3
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented#2646328: CSRF in update module manual check links might show an example of this.
Maybe it's by design that if you don't generate the link as a render array then it doesn't get the cacheability data correct? But that seems somewhat unsatisfying since there are multiple ways to generate URLs and links and it would be nice for them to work equally well.
Comment #5
Wim LeersConfirmed while I worked on #2646328-31: CSRF in update module manual check links.
Quoting my analysis from there:
Comment #6
Wim LeersComment #7
Wim LeersHere's the patch from #2646328-31: CSRF in update module manual check links.
Comment #8
Wim LeersCleanup.
Comment #9
dawehnerCan't we avoid one level of nesting here actually? Basically keep the exact same code, beside the first if
Comment #10
alexpottRe #9 we could do an early return... and then lose the last if.
Comment #11
Wim LeersAddressed #9.
Addressed Alex' feedback from IRC:
I will write tests later today, unless somebody beats me to it.
Comment #12
Wim LeersNow with updated issue summary.
Comment #13
alexpottUnfortunately we have to fix this in more places :(
Comment #14
alexpottBeginning of some tests... we need more coverage for attachments and Renderable interface
Comment #15
dawehnerWorking on some more test coverage.
Comment #16
dawehnerHere is some simplification of the code + more test coverage aka. also take care into account libraries.
Comment #18
dawehnerThere we go
Comment #19
joelpittetThis has tests. I've ran the tests manually and read through the code and it seems to be doing what it needs to achieve this fix.
TwigExtensionTest
will fail before this fix and passess afterwards.ThemeRenderAndAutoescapeTest
needs the datastructure that is provided to theRenderContext()
via bubbling.Comment #21
BerdirCould be a random fail:
Didn't fail like that before.
Comment #22
BerdirRequested a re-test.
Comment #23
catchCommitted/pushed to both branches, thanks!
Comment #25
vorapoap CreditAttribution: vorapoap commentedI confirm that this patch also fix CSRF Token problem in this Drupal StackExchange Issue