Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
forms system
Priority:
Major
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
28 Aug 2015 at 06:48 UTC
Updated:
16 Sep 2015 at 13:24 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
wim leersComment #3
moshe weitzman commentedIts correct.
Comment #4
wim leersAnd test coverage. The test-only patch is also the interdiff.
Comment #5
wim leersIn other words: the patch verifies that
max-age = 0is only set if a form token is indeed present, and *if* a form token is present, that it's alwaysmax-age=0.Comment #6
effulgentsia commentedSince the form token is added conditionally on $user->isAuthenticated(), should we also add the 'user.roles:authenticated' context to $form?
Comment #8
wim leersFirst, a reroll after #2554233: Port Cross-site Request Forgery - Form API fixes from SA-CORE-2015-003 to Drupal 8, which conflicted with this.
Comment #9
wim leers#6: indeed. That was unnecessary in #2526472: Ensure forms are marked max-age=0, but have a way to opt-in to being cacheable, and I failed to analyze the logic used for generating form tokens sufficiently, or I'd have caught that fact.
Comment #11
wim leersComment #13
wim leersGreen.
Comment #14
effulgentsia commentedLooks great. Thanks.
Comment #15
fabianx commentedRTBC + 1
Comment #16
catchCommitted/pushed to 8.0.x, thanks!
Comment #18
wim leersHurray!
This was the last blocker for #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!). It was split off from #2526472: Ensure forms are marked max-age=0, but have a way to opt-in to being cacheable, and uses a more granular, more precise approach to ensure SmartCache doesn't cache forms that should not be cached.
So, I tested #2429617-339: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) + #13 (the patch here that just got committed), to see if SmartCache would still be green. But, unfortunately, the answer is : #2560959-4: Testing issue for #2429617 is red.
Because of the more granular (and better!) approach taken by this issue compared to #2526472, we need one more step: #2561775: Forms without $form['#action'] set get their action automatically generated based on current path + query args: cacheability metadata is missing. Then forms will have sufficient cacheability metadata. Hopefully see you there!