Skip to content

[HttpFoundation] Send Content-Length when calling Response::send() and the content is a non-empty string #45092

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 12, 2022
Merged

[HttpFoundation] Send Content-Length when calling Response::send() and the content is a non-empty string #45092

merged 1 commit into from
Apr 12, 2022

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Jan 20, 2022 •
edited
Loading

Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR -

More hints for the client and the webserver are always good.

As discussed on Slack with @bastnic

@carsonbot carsonbot added this to the 4.4 milestone Jan 20, 2022
@bastnic
Copy link
Contributor

bastnic commented Jan 20, 2022

To add some context: I've a problem somewhere in my stack that truncate my big json response (2.5mo).

I expected Symfony to add the header Content-Length that would help but it seems not. Anybody knows a good reason not to ?

@stof
Copy link
Member

stof commented Jan 20, 2022

This prepare method is called on the kernel.response event at priority 0, while the WebProfilerBundle will inject the toolbar later. So this would then report the wrong content-length header, which seems worse to me. Maybe Response::setContent should update the Content-Length header if it is present.

@stof
Copy link
Member

stof commented Jan 20, 2022

And very early versions of Symfony used to put that header, which has been removed to fix an issue with gzip: #1846

@nicolas-grekas
Copy link
Member Author

Thanks for link @stof, PR updated to update the header in setContent() instead.

@nicolas-grekas nicolas-grekas force-pushed the response-length branch 2 times, most recently from fd0de27 to 2d59966 Compare January 20, 2022 14:58
@nicolas-grekas nicolas-grekas changed the title [HttpFoundation] send Content-Length when content is a non-empty string [HttpFoundation] send Content-Length when content is a string Jan 20, 2022
@stof
Copy link
Member

stof commented Jan 20, 2022

Given the potential impact (the old ticket also mentions that adding the Content-Length prevents Apache to use chunked transfer encoding), should this be merged as a bugfix or a new feature ?

@stof
Copy link
Member

stof commented Jan 20, 2022

Also, the PR description looks wrong. It is not fixing the bug from 2011 (which was already fixed by removing the content-length header at that time)

@nicolas-grekas nicolas-grekas changed the title [HttpFoundation] send Content-Length when content is a string [HttpFoundation] send Content-Length when content is a non-empty string Jan 20, 2022
@nicolas-grekas nicolas-grekas changed the base branch from 4.4 to 6.1 January 20, 2022 15:12
@nicolas-grekas nicolas-grekas modified the milestones: 4.4, 6.1 Jan 20, 2022
@nicolas-grekas
Copy link
Member Author

OK to target 6.1
PR updated. Setting the header in setContent() breaks HttpCache.
Now doing it in send()

94noni
94noni reviewed Jan 31, 2022
View reviewed changes
fabpot
fabpot reviewed Mar 4, 2022
View reviewed changes
@nicolas-grekas
Copy link
Member Author

Review welcome @symfony/mergers

@nicolas-grekas nicolas-grekas changed the title [HttpFoundation] send Content-Length when content is a non-empty string [HttpFoundation] Send Content-Length when calling Response::send() and the content is a non-empty string Apr 11, 2022
@fabpot
Copy link
Member

fabpot commented Apr 12, 2022

Thank you @nicolas-grekas.

@fabpot fabpot mentioned this pull request Apr 12, 2022
Closed
@fabpot fabpot merged commit aff969d into symfony:6.1 Apr 12, 2022
@fabpot fabpot deleted the response-length branch April 12, 2022 06:00
@fabpot fabpot mentioned this pull request Apr 15, 2022
Merged
nicolas-grekas added a commit to nicolas-grekas/symfony that referenced this pull request May 31, 2022
@nicolas-grekas
Revert "feature symfony#45092 [HttpFoundation] Send Content-Length …
7e24e5d 
…when calling `Response::send()` and the content is a non-empty string (nicolas-grekas)"

This reverts commit aff969d, reversing
changes made to 8b680f0.
This was referenced May 31, 2022
Merged
Closed
nicolas-grekas added a commit that referenced this pull request May 31, 2022
@nicolas-grekas
bug #46523 [HttpFoundation] Revert "Send Content-Length when callin…
a045fef 
…g `Response::send()` and the content is a non-empty string" (nicolas-grekas)

This PR was merged into the 6.1 branch.

Discussion
----------

[HttpFoundation] Revert "Send `Content-Length` when calling `Response::send()` and the content is a non-empty string"

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #46449
| License       | MIT
| Doc PR        | -

Let's revert #45092 as it's breaking BC. It's not worth it.

Commits
-------

7e24e5d Revert "feature #45092 [HttpFoundation] Send `Content-Length` when calling `Response::send()` and the content is a non-empty string (nicolas-grekas)"
nicolas-grekas added a commit that referenced this pull request May 31, 2022
@nicolas-grekas
Merge branch '6.1' into 6.2
f3a9ac1 
* 6.1:
  [Workflow] Add ZEturf as backer
  [Serializer] code cleanup
  [Serializer] Forget partially collected traces
  Added missing __call to TraceableEncoder
  Revert "feature #45092 [HttpFoundation] Send `Content-Length` when calling `Response::send()` and the content is a non-empty string (nicolas-grekas)"
  [PropertyInfo] Fix extracting int range type
  [FrameworkBundle] Add alximy as backer of version 6.1
  [WebProfilerBundle] Fix AJAX requests info are truncated in the WDT
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@94noni 94noni 94noni left review comments

@fabpot fabpot fabpot approved these changes

@Tobion Tobion Tobion approved these changes

@yceruto yceruto yceruto approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Feature HttpFoundation Ready Status: Reviewed
Projects
None yet
Milestone
6.1
Development

Successfully merging this pull request may close these issues.
9 participants
@nicolas-grekas @bastnic @stof @fabpot @Tobion @94noni @yceruto @chalasr @carsonbot