ActiveLinkResponseFilter breaks any text/html BinaryFileResponse or StreamedResponse for anonymous users

sends a BinaryFileResponse with the content type set to text/html,

What is the use case for doing this?

This is also an issue for StreamedResponse as it does not support setContent() either.

Our use case was for some html that we cache gzip & brotili compressed versions of, on cache miss we generate the html, gzip and return it but then create the brotli compressed version and update the cache after flushing to the client (brotli is very slow!). I'm sure this could be done with an event listener or something as well, but this should work imo.

I can see the possible concern with this fix being that a developer has no warning that active links will not get is-active classes on links (and as this is only for anon users, could easily be missed) but i do not see any good way around this? I imagine it should be pretty rare to be serving full drupal rendered html pages to anon users in a streamed or binary file response so unless anyone has a better solution then personally i vote standard symfony responses working > extreme edge case DX.

Have updated to include StreamedResponse and added some tests.

Not sure why the output assertion fails in CI environment, possibly something to do with the buffer?

Whatever it was the output is not relevant to the test, updated to simply pass if no exception and improved the test file name while i was at it.

Test proven, fix passed.

@wim I won't claim there are "good" reasons but if you've got a html file on the disk you want to deliver say from a private file store or outside the doc root somewhere then you'd use BinaryFileResponse() and the content type would be 'text/html'.

A developer doing this would have surely backed themselves into a terrible corner because there are likely better solution to the problem but... it could happen.

1. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/ActiveLinkResponseFilterTest.php
   @@ -397,4 +407,58 @@ public function testSetLinkActiveClass($html_markup, $current_path, $is_front, $
   +    $language = new LanguageDefault([
   +      'id' => $this->randomMachineName(2),
   +      'name' => $this->randomMachineName(),
   +      'uuid' => $this->randomMachineName(),
   +    ]);
   +    $language_manager = new LanguageManager($language);
   

   Why mock everything else but make a concrete language? I don't know why I highlighted this other then there is a lot of code for mocking that could be a lot smaller and lighter.

2. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/ActiveLinkResponseFilterTest.php
   @@ -397,4 +407,58 @@ public function testSetLinkActiveClass($html_markup, $current_path, $is_front, $
   +    $file = __DIR__ . '/../../../../fixtures/active_link_response_filter_test.html';
   +    $responses[] = new BinaryFileResponse($file, 200, $headers);
   

   __FILE__ will work, we don't care about the content's as they won't go anywhere and we won't have to add a fixture.

3. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/ActiveLinkResponseFilterTest.php
   @@ -397,4 +407,58 @@ public function testSetLinkActiveClass($html_markup, $current_path, $is_front, $
   +      // If no exception is thrown in the event handler then it is a pass.
   +      // @see \Symfony\Component\HttpFoundation\StreamedResponse::setContent().
   +      ob_start();
   +      $subscriber->onResponse($event);
   +      $response->send();
   +      ob_end_clean();
   +      $this->assertTrue(TRUE, 'No exception was thrown');
   

   I'm not sure we need to send the response, that just makes extra complexity especially since we just toss the output.

   Also, assertTrue(TRUE) is almost always a bad idea even when just testing the absence of an exception. There is generally something else you should be asserting at the same time like a return value. There is no return here but we can use the mocks to make that better assertion. It means mocking a service but we where already doing that and the path stack has some gnarly dependencies so its a prime candidate.

Just updating the tests as noted and using instance_of since its slightly faster than is_a and meets our requirements.

bah, i used the wrong session for the test... split the tests so they could be documented better.

In order to send that HTML out as an "attachment" (i.e. browser will save instead of navigating to) then I need the "setContentDisposition()" method, and that's only available on BinaryFileResponse.

While setDisposition() is only available on BinaryFileResponse, nothing prevents you from manually setting the Content-Disposition header on your response, which I think is preferred over (ab-)using BinaryFileResponse just to make the output downloadable. Here is a snippet that triggers a download for a regular HTML response:

 public function download() {
    $build['#markup'] = 'I\'m a downloadable HTML response.';
    $build['#attached']['http_header'][] = ['Content-Disposition', 'attachment; my-file-name.html'];
    return $build;
  }

... if you've got a html file on the disk you want to deliver say from a private file store or outside the doc root somewhere then you'd use BinaryFileResponse() and the content type would be 'text/html'.

IMO, this is a valid use case and reason to adjust ActiveLinkResponseFilter::onRespond() so it does not attempt invoke setContent() when this would trigger an exception. The following currently would not work and throws an exception:

public function download() {
  $response = new BinaryFileResponse('/var/www/html/modules/custom/html_download/download.html');
  $response->headers->set('Content-Type', 'text/html');
  return $response;
}

Reviewing the current proposed patch:

+    // Ignore response objects that do not support setContent.
+    $not_supported = $response instanceof BinaryFileResponse;
+    $not_supported = $not_supported || $response instanceof StreamedResponse;
+    if ($not_supported) {
+      return;
+    }

Rather than explictly checking on instanceof StreamedResponse | BinaryFileResponse, I think we should rely on the fact that classes inheriting from Response can opt to override ::getContent() and return FALSE. Both StreamedResponse::getContent() and BinaryFileResponse::getContent() will return FALSE, as might other derived classes not mentioned in this issue.

So the code at the end of ActiveLinkResponseFilter::onRespond() would become:

$response = $event->getResponse();
if ($content = $response->getContent()) {
  $response->setContent(static::setLinkActiveClass(
    $content,
    ltrim($this->currentPath->getPath(), '/'),
    $this->pathMatcher->isFrontPage(),
    $this->languageManager->getCurrentLanguage(LanguageInterface::TYPE_URL)->getId(),
    $event->getRequest()->query->all()
  ));
}

It would have to be === FALSE so it handles a 204 but that might work.

Lets see how it works.

I'm still not sure how I feel about it. On the one hand we're relying on undocumented complexity and assumptions about how two methods interact(because there is no interface) in external libraries. On the other we're hard coding a list of classes that limits other libraries and contrib in how it can interact with this behavior. Its probably fine I'm just not 100% comfortable backdooring an interface where there isn't one. :-/

It looks like the uploaded patch in #21 is the same patch as #16?

It would have to be === FALSE so it handles a 204 but that might work.

Would it? If getContent() were to return an empty string instead of FALSE, ActiveLinkResponseFilter::setLinkActiveClass() would not do anything (there is nothing to replace), so it feels like we might as well do if ($content) {...} instead of if ($content !== FALSE) {...}?

hm... I have the right patch still. Here it is.

This resolves the issue for me, which breaks jsonapi_explorer.

If getContent() were to return an empty string instead of FALSE

True. But an empty string wouldn't even be processed by the setLinkActiveClass function since the while loop would break immediately and process nothing. The real check here is for responses which explicitly return FALSE because the content is streamed.

Thanks, that's interesting. Sounds like its blocking a contrib project then and that technically makes it "Major"

Fixing up some of the code commentary. Some of it was wrong - for example - the filter only works for anonymous users not logged in users and some of it was overly wordy and not as per standards.

Removing unused use statements.

Committed and pushed ba9eff6b48 to 9.0.x and a2c8fd1361 to 8.9.x. Thanks!

Asking other committers for a +1 for backport to 8.8.x

🙌 thank you alexpott

@catch +1'd the backport

Nice work here! I added ActiveLinkResponseFilter in #2478443: Set the 'is-active' class for anonymous users in a Response Filter instead of a #post_render_cache callback 4.5 years ago, this clearly was an oversight. Thanks! 🙏