Only set content-length header in specific situations

This changes the method to look like this:

  public function setContentLengthHeader(ResponseEvent $event): void {
    $response = $event->getResponse();
    if ($response instanceof StreamedResponse) {
      return;
    }

    // Do not add header for 100s and 500s, see
    // https://www.rfc-editor.org/rfc/rfc9110.html#name-content-length.
    if ($response->isInformational() || $response->isServerError() || $response->isEmpty()) {
      return;
    }

    if ($response->headers->has('Transfer-Encoding')) {
      return;
    }

    $content = $response->getContent();
    if ($content === FALSE) {
      return;
    }

    $response->headers->set('Content-Length', strlen($content), TRUE);
  }

Some of this is inspired by what's in \Symfony\Component\HttpFoundation\Response::prepare() which is the last thing called by \Drupal\Core\DrupalKernel::handle() before we send the request.

So as it turns out Symfony is already fixing quite a bit of this for us, but I still think we shouldn't be adding this header in when we know we're just going to remove it later.

One thing I wonder is just we remove

    if ($response instanceof StreamedResponse) {
      return;
    }

as this is covered by

    $content = $response->getContent();
    if ($content === FALSE) {
      return;
    }

Curious how it will work when gzip/brotli will be enabled at web-server side

@andypost how is that relevant to this issue? Do they require the content-length header? FWIW server side gzip/brotli must adjust the content-length header - as suggested here https://serverfault.com/questions/783029/apache-returns-invalid-content-...

So as it turns out Symfony is already fixing quite a bit of this for us, but I still think we shouldn't be adding this header in when we know we're just going to remove it later.

Yes this makes sense, as evidenced by having to get to this issue to find this out. Removing the explicit StreamedResponse check seems fine, if that somehow changes later, we'll find out.

I'm thinking that we should separate out the isServerError() check as that's the one thing that's not per RFC but down to some oddness in Drupal's error handling. Then if we fix that we could remove it. Pushed that change to the MR.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Looks like 🐛 Uncaught ajax.js error / exception Active is reporting this too.

There's one case in #3409885-15: Uncaught ajax.js error / exception → that this won't fix, but it's not clear to me how they ended up in that situation. Given that, I think we should go ahead here to at least now the failure cases down more.

I only pushed the rebase button to get the MR green again. This looks fine to me and I think we should go ahead.

Issue credits

Committed to 11.x and backported to 10.2.x

Thought about asking for a test-case with StreamedResponse with a callback, but then realized StreamedResponse::getContent returns FALSE so it doesn't matter.

Thanks all

larowlan → closed merge request !5121

This seems to have introduced a new test class with a typo, we now have FinishResponseSubscriberTest and FinishResponserSubscriberTest. Should this be reverted and we merge these together?

🐛 Regression from #3295790 content-length header set earlier than expected Fixed deletes that test and is also critical

Should we focus effort there?

Automatically closed - issue fixed for 2 weeks with no activity.