Skip to content
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.

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

Avoid setting Content-Encoding header for static view responses. #2810

Merged
merged 2 commits into from
Nov 16, 2016

Conversation

davisagli
Copy link
Member

@davisagli davisagli commented Nov 10, 2016

This was causing clients to decode the content of gzipped files when downloading them.

We discussed this on IRC and @mmerickel pointed out that it may be a goal in some cases to serve gzipped precompiled assets (i.e. CSS/Javascript) with this header. But that's a new feature that would require thought about how to specify which files to serve that way. And it can already be
implemented for a project using a tween. This just aims to fix the existing use case of serving files for download.

This was causing clients to decode the content of gzipped files
when downloading them.

We discussed this on IRC and @mmerickel pointed out that it may be a goal in some cases
to serve gzipped precompiled assets (i.e. CSS/Javascript) with this header.
But that's a new feature that would require thought about how to
specify which files to serve that way. And it can already be
implemented for a project using a tween. This just aims to
fix the existing use case of serving files for download.
@davisagli davisagli added the bugs label Nov 10, 2016
to an encoding guessed using Python's ``mimetypes`` module.
This was causing clients to decode the content of gzipped files
when downloading them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can add a link to the PR here similar to other changes please.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmerickel yep, done

@davisagli
Copy link
Member Author

What's the protocol now? Shall I merge this myself? And backport to maintenance branches?

@mmerickel
Copy link
Member

This is technically bw-incompat and so I don't think we should backport it.

I usually let things sit for a couple days just incase someone else feels like reviewing and then I'll merge it.

@davisagli
Copy link
Member Author

Can we never backport bugfixes then? Or is there something bw-incompatible you're seeing apart from the bugfix itself?

@mmerickel
Copy link
Member

My hesitation was that it's a "bug" that's been there for a very long time. At what point does a bug become a feature? Thus it made sense to me to wait until a new major release to fix it (pyramid 1.8 is coming soon) where people may be more expecting of weird / breaking changes.

However, I agree with you it's a bug so if you feel strongly enough about it then go ahead and push backported PRs to 1.7 and 1.6 branches.

@davisagli
Copy link
Member Author

I don't feel strongly enough about it. :)

@mmerickel mmerickel merged commit f576919 into Pylons:master Nov 16, 2016
@digitalresistor
Copy link
Member

I would argue that this introduces a backwards incompatible change that is being labeled as fixing a "bug".

@davisagli davisagli deleted the fix-static-content-encoding branch November 16, 2016 05:55
@mmerickel
Copy link
Member

@bertjwregeer sorry I'm not quite sure what you're trying to say.

@digitalresistor
Copy link
Member

CHANGES.txt says "Fix ..." as if it was fixing a bug. I'd argue that this should be "Change ...".

I would also argue that this should be listed as a backwards incompatibility, as a new feature, even if it were intended to be originally as it is now.

@mmerickel
Copy link
Member

I see. I will update the changelog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants