Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
after #1798296: Integrate CSRF link token directly into routing system we need to convert those.
following the shortcut example in the patch on this issue, we should do the same for
grep -r -E "1798296|755584" core/ core/modules/comment/lib/Drupal/comment/Controller/CommentController.php: // https://drupal.org/node/1798296. core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.php: // http://drupal.org/node/1798296. core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php: // generation. Add token support to routing: http://drupal.org/node/755584.
Comment | File | Size | Author |
---|---|---|---|
#18 | 2109303-18.patch | 17.72 KB | damiankloip |
#11 | interdiff-2109303-11.txt | 1.49 KB | damiankloip |
#11 | 2109303-11.patch | 24.52 KB | damiankloip |
Comments
Comment #1
ParisLiakos CreditAttribution: ParisLiakos commentedforgot the tags
Comment #2
damiankloip CreditAttribution: damiankloip commentedHere is a start on this. To convert Views UI ajax stuff too, I think we need to sort out how entity operation links work, as we are currently appending $op to the entity uri.. I have overridden the links build array in ViewListController here.
Comment #4
damiankloip CreditAttribution: damiankloip commentedThis should fix those up.
Comment #5
damiankloip CreditAttribution: damiankloip commentedPostponing this for now on #2133439: Dynamically create token value string based on route path.
Comment #6
damiankloip CreditAttribution: damiankloip commentedThat issue is fixed now. Let's make those changes and see what happens.
Comment #7
Crell CreditAttribution: Crell commentedI really like how much code this patch removes, and how many dependencies it eliminates. It means we did it right. :-)
Comment #9
damiankloip CreditAttribution: damiankloip commentedSorry, that will fail. Needs a couple of fixes that I had in my branch that then didn't include.
Comment #11
damiankloip CreditAttribution: damiankloip commentedAnd fix those tests to reflect the new fix.
Comment #12
Crell CreditAttribution: Crell commentedHm. I'm not sure about those path changes. We should be migrating things more toward always using the leading slash, not away. Why is that necessary?
Comment #13
damiankloip CreditAttribution: damiankloip commentedPretty much because of _system_path on the request.
Comment #14
Crell CreditAttribution: Crell commentedUgh. We've really got to fix that at some point.
Comment #15
damiankloip CreditAttribution: damiankloip commentedComment #16
damiankloip CreditAttribution: damiankloip commented11: 2109303-11.patch queued for re-testing.
Comment #18
damiankloip CreditAttribution: damiankloip commentedrerolled. Patch is much smaller as overlay changes are not needed anymore!
Comment #19
damiankloip CreditAttribution: damiankloip commentedComment #21
damiankloip CreditAttribution: damiankloip commented18: 2109303-18.patch queued for re-testing.
Comment #22
damiankloip CreditAttribution: damiankloip commentedBack to rtbc. That was just a reroll.
Comment #23
webchickWow, this is really great clean-up!
While this is undoubtedly better DX than all of the manual PHP checking that this patch removes, it also seems like it's super easy to miss adding this parameter to a route, which could lead to security issues. :(
OTOH, I can't really spot anything in the routing.yml declaration that would allow automated application of this property. We should make sure to document when to use it and why in the routing system docs, in addition to adding a change notice for this.
Committed and pushed to 8.x. Thanks!
Comment #24
webchickAh, nevermind. Looks like CN/docs are handled over at #1798296: Integrate CSRF link token directly into routing system instead.
Comment #25
Crell CreditAttribution: Crell commentedwebchick: I don't think this makes it any easier or harder to know "I SHOULD use a CSRF token here". It just makes the step of actually doing so once you know you should about 10x easier, especially since you don't need to remember to do so in every place you have a link. So it's still a big win, although not the win you're talking about. (Which would be a win if we knew how to make that more obvious somehow, but that's a separate issue.)