Paths

Table of Contentst

Differential D17045

Don't combine automatic output compression with "Content-Length"
ClosedPublic
Actions

Authored by epriestley on Dec 13 2016, 10:07 PM.

Tags

None

Referenced Files

	F14825733: D17045.id40997.diff
	Tue, Jan 28, 10:49 PM

	Unknown Object (File)
	Mon, Jan 27, 10:16 AM

	Unknown Object (File)
	Sun, Jan 26, 6:11 PM

	Unknown Object (File)
	Fri, Jan 24, 6:31 PM

	Unknown Object (File)
	Fri, Jan 24, 6:31 PM

	Unknown Object (File)
	Tue, Jan 21, 4:55 PM

	Unknown Object (File)
	Tue, Jan 21, 9:16 AM

	Unknown Object (File)
	Sat, Jan 18, 4:53 AM

Subscribers

Details

Reviewers

avivey
chad

Maniphest Tasks

T12013: Enabling zlib.output_compression results in wrong Content-Length

Commits

rP1cd64f9975d6: (stable) Don't combine automatic output compression with "Content-Length"
rP842710608e49: Don't combine automatic output compression with "Content-Length"

Summary

Fixes T12013. Send either "Content-Length" or enable output compression, but not both.

Prefer compression for static resources (CSS, JS, etc).

Test Plan

Ran curl -v ..., no longer saw responses with both compression and Content-Length.

Diff Detail

Repository

Branch

compress1

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 14886
Build 19496: Run Core Tests
Build 19495: arc lint + arc unit

Event Timeline

epriestley updated this revision to Diff 40997.Dec 13 2016, 10:07 PM

epriestley retitled this revision from to Don't combine automatic output compression with "Content-Length".

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added a reviewer: chad.

epriestley added a task: T12013: Enabling zlib.output_compression results in wrong Content-Length.

epriestley mentioned this in T12013: Enabling zlib.output_compression results in wrong Content-Length.Dec 13 2016, 10:10 PM

avivey added a subscriber: avivey.Dec 13 2016, 10:10 PM

avivey added inline comments.

src/aphront/response/AphrontFileResponse.php
134–136	maybe also enable the compression on `else`?

Yeahhhhh

src/aphront/response/AphrontFileResponse.php
134–136	It's enabled globally in `PhabricatorStartup` and I shudder to think what PHP might do if we enable it twice. It would, perhaps, be cleaner to bring the `CompressResponse` stuff up a level to `AphrontResponse`, and have that control `zlib.output_compression` in all cases, although when we fatal the "¯\_(ツ)_/¯" page won't get compressed.

Centralize compression logic in one place, in AphrontResponse.

Yeah, that looks like it will probably not PHP in new ways?

("PHP" in that sentence is short for "break in interesting ways").

This revision is now accepted and ready to land.Dec 13 2016, 10:23 PM

Hahaha. Yeah, this feels a little more solid.

Closed by commit rP842710608e49: Don't combine automatic output compression with "Content-Length" (authored by epriestley, committed by epriestley). · Explain WhyDec 13 2016, 10:25 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

src/

aphront/

response/

AphrontFileResponse.php

20 lines

AphrontResponse.php

4 lines

sink/

AphrontHTTPSink.php

2 lines

applications/

celerity/

controller/

CelerityResourceController.php

7 lines

Diff 40997

src/aphront/response/AphrontFileResponse.php

Loading...

src/aphront/response/AphrontResponse.php

Loading...

src/aphront/sink/AphrontHTTPSink.php

Loading...

src/applications/celerity/controller/CelerityResourceController.php

Loading...