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

Fix htmlcompressor Content-Length computation #15

Merged
merged 2 commits into from
Jul 7, 2013
Merged

Fix htmlcompressor Content-Length computation #15

merged 2 commits into from
Jul 7, 2013

Conversation

michaelbaudino
Copy link
Contributor

This PR fixes how htmlcompressor computes Content-Length.
It should be based on the number of bytes, rather than on the number of characters (which differs, e.g. with accented characters).

It should fix this issue : http://forum.middlemanapp.com/t/middleman-minify-html-content-length-header-error/697

Check out this IRB session:

2.0.0-p247 :001 > 'é'.length
=> 1
2.0.0-p247 :002 > 'é'.bytesize
=> 2

…r than on characters number (which is not the same, e.g. with accented characters)
@tdreyno
Copy link
Member

tdreyno commented Jul 7, 2013

Thanks so much for the patch. Did this already go into the upstream repo?

tdreyno added a commit that referenced this pull request Jul 7, 2013
…-contentlength

Fix htmlcompressor Content-Length computation
@tdreyno tdreyno merged commit 9ddb993 into middleman:master Jul 7, 2013
@michaelbaudino
Copy link
Contributor Author

I created a PR, but it's not merged yet : paolochiodi/htmlcompressor#9
And actually, I figured out few minutes after submitting that someone had already submitted the same PR : paolochiodi/htmlcompressor#7

Thanks for merging :-)

@michaelbaudino michaelbaudino deleted the feature/fix-htmlcompressor-contentlength branch July 7, 2013 01:26
@paolochiodi
Copy link

Hi, I just fixed this in paolochiodi/htmlcompressor#7
Sorry for the delay on this

@tdreyno
Copy link
Member

tdreyno commented Jul 8, 2013

Okay, this is out with v3.1.1

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

Successfully merging this pull request may close these issues.

None yet

3 participants