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.
Problem/Motivation
We currently (ab)use the #post_render_cache
mechanism to apply response-wide replacements.
But, in #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts, we are trying to get rid of #post_render_cache
, and the setting of the "is-active" class is the only problematic use of it. Quoting myself in #2469431-40: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts:
- I think we should consider removing
#post_render_cache
. The#attached
case for#post_render_cache
only has a single use case in core (history_attach_timestamp()
), which could also be handled by a placeholder that returns the empty string as#markup
, but does set#attached
.
The only case in core for which this is wrong, is setting the "active" class on links for anonymous users, which is done by theSystemController::\Drupal\system\Controller\SystemController::setLinkActiveClass()
#post_render_cache
callback. But, that's such a very very very special case, and actually, Symfony already has a nice abstraction for it: theFilterResponseEvent
. (IMO we should use that mechanism anyway, regardless of this issue.)
That'd allow us to keep things simple, and in fact, make things simpler compared to HEAD.
Proposed resolution
Symfony already has a nice abstraction for this: FilterResponseEvent
/KernelEvents::RESPONSE
. Use that instead.
Remaining tasks
TBD
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#22 | anon_active_links-2478443-22.patch | 24.56 KB | Wim Leers |
Comments
Comment #1
Wim LeersHere's the patch with minimal changes.
Comment #2
Wim LeersNow all logic and tests migrated, but still minimal changes in the tests.
Comment #4
Wim LeersPartially updated the unit tests; now no mention of render arrays anymore.
(Also rebased, which makes the patch applicable again.)
Comment #5
dawehnershould we namespace them, like response_filter.active_link?
docs :)
are we sure about that? Don't we want to do the active link handling also on modal/dialog requests for example?
Should we use the current_path service instead?
Any reason to choose that late handling?
Comment #7
Wim Leers… and the remaining changes. I think this is sufficiently clean.
Comment #8
Wim Leershook_page_attachments()
, which definitely only runs for HTML responses, because it is invoked byHtmlRenderer
.AFAIK we've never done "active link" handling inside modals/dialogs. Doing that can be a normal follow-up.
Comment #9
dawehnerWell, just document the intent, I think its fine to do so.
Comment #10
Wim LeersComment #12
Wim Leers#9: done.
Comment #14
Wim LeersFixed the failures in #1.
EDIT: AFAICT that fixed all failures!
Comment #16
Wim LeersComment #18
Fabianx CreditAttribution: Fabianx commentednit - wrong @see for SystemController
--
Besides that, as this only concerns the move from system_page_attachmetns to the finish response subscriber, this is good to go.
RTBC once this nit is fixed.
Train of thought
I still think things like CSRF tokens, etc. or the JS to add the active-class library belong on the link render array and not system wide.
The consequence of doing that system wide is that whenever we use placeholdering we also need to add that library as the FinishResponseSubscriber (and the previous #prc) won't cut it for fragments / placeholders. That however is a solvable problem and should be done in a follow-up.
Comment #19
Wim LeersRE: train of thought:
: agreed with that principle.Comment #20
dawehnerIt would be great if we could document in the issue summary that this is code just done for anonymous users, and by that will not require to run for bigpipe.
I thought we don't do yoda.
Comment #21
Wim LeersFixed the nit!
Comment #22
Wim Leers#20:
Comment #23
Fabianx CreditAttribution: Fabianx commentedYes, do yoda we don't. :-D
RTBC now, page_attachments_alter runs so late that its almost as late already as a FilterResponseSubscriber.
Comment #24
Crell CreditAttribution: Crell at Palantir.net commentedThere's no reason for this to be static. It's not getting used elsewhere (and should not be). If it really does need to be accessible elsewhere, then it should be its own service. Which given its complexity is not a bad idea anyway.
(Yes I know it's a static in the current class that's being replaced, but campfire rules: Don't just move code around, improve it.)
This will conflict with #2477461: Move X-Generator header to its own listener. Let's let that get in first as this would be easy to reroll.
Comment #25
Wim Leers2) http://drupal4hu.com/node/416.html
3) that'll complicate testability
Therefore please let's do that in a minor/normal follow-up. This is major for a reason.
Comment #27
catchYes agreed on the static method, we can discuss in a follow-up if at all.
I had the same question as dawehner about AJAX/modals but 1. it doesn't work now 2. I'm pretty sure it also doesn't work in 7.x since you'd not get the same for current_path() (at least for AJAX).
Committed/pushed to 8.0.x, thanks!
Comment #29
damiankloip CreditAttribution: damiankloip commentedHow will making the method non-static complicate testability? Also, using a static method to signify a method does not change the state of an object just doesn't really seem like a 'thing'.
I agree with Crell, there is just no need. Minor issue though now...
Comment #30
Fabianx CreditAttribution: Fabianx as a volunteer commentedI agree that we can improve that, but that was really out-of-scope for this issue.
Can we open a follow-up for that to cleanup, please? Thanks!
Comment #31
Wim LeersImportant follow-up just landed: #2929798: ActiveLinkResponseFilter breaks any text/html BinaryFileResponse or StreamedResponse for anonymous users.