Set the 'is-active' class for anonymous users in a Response Filter instead of a #post_render_cache callback

1. +++ b/core/core.services.yml
   @@ -1351,3 +1351,9 @@ services:
   +  active_link_response_filter:
   

   should we namespace them, like response_filter.active_link?

2. +++ b/core/lib/Drupal/Core/EventSubscriber/ActiveLinkResponseFilter.php
   @@ -0,0 +1,234 @@
   +  /**
   +   * @param AccountInterface $current_user
   +   * @param RouteMatchInterface $route_match
   +   * @param PathMatcherInterface $path_matcher
   +   * @param LanguageManagerInterface $language_manager
   +   */
   

   docs :)

3. +++ b/core/lib/Drupal/Core/EventSubscriber/ActiveLinkResponseFilter.php
   @@ -0,0 +1,234 @@
   +    // Only care about HTML responses.
   +    if ($event->getRequest()->getRequestFormat() !== 'html') {
   +      return;
   +    }
   

   are we sure about that? Don't we want to do the active link handling also on modal/dialog requests for example?

4. +++ b/core/lib/Drupal/Core/EventSubscriber/ActiveLinkResponseFilter.php
   @@ -0,0 +1,234 @@
   +    $current_path = $this->routeMatch->getRouteName() ? Url::fromRouteMatch($this->routeMatch)->getInternalPath() : '';
   

   Should we use the current_path service instead?

5. +++ b/core/lib/Drupal/Core/EventSubscriber/ActiveLinkResponseFilter.php
   @@ -0,0 +1,234 @@
   +    $events[KernelEvents::RESPONSE][] = ['onResponse', -512];
   

   Any reason to choose that late handling?

Only one reason: because logically speaking, this should run *very* late. It should run after anything else that modifies the markup. That's all. Happy to change it!

Well, just document the intent, I think its fine to do so.

+++ b/core/modules/system/system.module
@@ -596,25 +597,15 @@ function system_page_attachments(array &$page) {
   // @see \Drupal\system\Controller\SystemController::setLinkActiveClass
+  // @see \Drupal\Core\EventSubscriber\ActiveLinkResponseFilter

nit - 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.

It 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.

+++ b/core/lib/Drupal/Core/EventSubscriber/ActiveLinkResponseFilter.php
@@ -0,0 +1,241 @@
+    if (FALSE === stripos($event->getResponse()->headers->get('Content-Type'), 'text/html')) {

I thought we don't do yoda.

Yes, do yoda we don't. :-D

RTBC now, page_attachments_alter runs so late that its almost as late already as a FilterResponseSubscriber.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/ActiveLinkResponseFilter.php
   @@ -0,0 +1,246 @@
   +  public static function setLinkActiveClass($html_markup, $current_path, $is_front, $url_language, array $query) {
   

   There'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.)

2. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
   @@ -165,6 +165,7 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
        $response = new CacheableResponse($content, 200,[
   +      'Content-Type' => 'text/html; charset=UTF-8',
          'X-Generator' => 'Drupal ' . $version . ' (https://www.drupal.org)'
        ]);
   

   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.

Yes 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!

How 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...

I 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!