Skip to content

Adds config option to minify HTML output#965

Merged
philsturgeon merged 14 commits into
bcit-ci:developfrom
homstie-dev:feature/minify_output
Jun 14, 2012
Merged

Adds config option to minify HTML output#965
philsturgeon merged 14 commits into
bcit-ci:developfrom
homstie-dev:feature/minify_output

Conversation

@homstie-dev

Copy link
Copy Markdown
Contributor

If the $config['minify_output'] setting is set to TRUE, CodeIgniter's output class will process the output with a basic minify function. Will apply minify rules according to the currently set content type (defaults to "text/html" and currently only supports "text/html" "text/css" and an extremely rudimentary minifier for "text/javascript").

Output is minified before the cache is written, so savings are also on diskspace.

According to my tests it typically reduces the content by 8-15%.

PS, the first two commits are an artifact of me not knowing how to branch properly when I started this. Those are part of my "http cache headers" pull request.

@gaker

gaker commented Jan 25, 2012

Copy link
Copy Markdown
Contributor

This is a fantastic idea. Have you given thought on newline characters & how we might handle that with pre/code tags?

@homstie-dev

Copy link
Copy Markdown
Contributor Author

I'll modify it to leave <pre> and <code> content untouched.

There are a number of open source minifiers around, but there are probably licence incompatibilities. :(

@homstie-dev

Copy link
Copy Markdown
Contributor Author

Well, I'm happy. My landing page is now 30% smaller, and most other pages 10-15%.

Comment thread system/core/Output.php Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add a space in there? The most trivial thing in the world but.. , $expire).

@arenowebdev

Copy link
Copy Markdown

👍 This would help me a ton :-)

@dbpolito

Copy link
Copy Markdown

+1

3 similar comments
@mpmont

mpmont commented Jan 25, 2012

Copy link
Copy Markdown
Contributor

+1

@eweap

eweap commented Jan 26, 2012

Copy link
Copy Markdown
Contributor

+1

@tarciozemel

Copy link
Copy Markdown

+1

@timw4mail

Copy link
Copy Markdown
Contributor

Cool. I've had my own makeshift version of this myself that clobbers <pre> tags. So this is really nice to have in the framework.

Comment thread system/core/Output.php Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any advantage to the newline as opposed to a space?

And I think $output = preg_replace("!\s+!", " ", $output) is a little more readable.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Replacing only 2+ space occurences should be faster than each single one.
I'd also vote for spaces as opposed to newlines though.

@homstie-dev

Copy link
Copy Markdown
Contributor Author

The newline instead of space was based on an old and perhaps unfounded superstition I have that having excessively long line-lengths can be an issue. Since newline and space are otherwise functionally the same I saw no harm in it. But if line-lengths are not actually an issue,

As far as s{2,} vs s+, I imagine the former is more efficient since it is more restrictive. The latter does a replace on every single space, tab, and/or newline, whereas the former only does the replace if it results in a reduction. If there's still a push for changing this I'd like to do a benchmark comparison just to get an idea, since I really have no idea what the effect is for sure.

@timw4mail

Copy link
Copy Markdown
Contributor

I don't think long lines matter much. In fact, I've found that any whitespace between tags is generally superfluous. Look at the source for https://timshomepage.net/, and see how long it takes to find a browser that chokes on it.

I basically use a variation of this: https://gist.github.com/1338288

Comment thread system/core/Output.php Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it makes much of a difference, but this is how I minify CSS: https://gist.github.com/1685711

More verbose in terms of code,but perhaps equivalent in functionality.

@philsturgeon

Copy link
Copy Markdown
Contributor

Yet to have any problems, even on some tricky pages http://d.pr/95kB

@philsturgeon

Copy link
Copy Markdown
Contributor

I understand the benefits of using s{2,} so that sounds good, and I don't think we need to add newlines in. Apache will never care about long lines unless it's in the HTTP Request, and then it's a 8kb limit.

So it looks like we're golden here guys. @derekjones @ericbarnes @gaker got anything to say about this one? I've had it enabled for pyrocms and the whole thing seems fine, shaved 1-2kb off each page!

Comment thread system/core/Output.php Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Space :p

@minerbog

minerbog commented Feb 4, 2012

Copy link
Copy Markdown

I was currently using

function sanitize_output($buffer)
{
$search = array(
'/>[^\S ]+/s', //strip whitespaces after tags, except space
'/[^\S ]+</s', //strip whitespaces before tags, except space
'/(\s)+/s' // shorten multiple whitespace sequences
);
$replace = array(
'>',
'<',
'\1'
);
return preg_replace($search, $replace, $buffer);
}

to strip any white space from outside < > tags. Not sure if you could find this useful :)

@homstie-dev

Copy link
Copy Markdown
Contributor Author

@minerbog it already does this with a single, more efficient regular expression.

@minerbog

minerbog commented Feb 4, 2012

Copy link
Copy Markdown

Apologies, regex has never been my strongest point!!

@homstie-dev

Copy link
Copy Markdown
Contributor Author

@minerbog No worries.

@timw4mail

Copy link
Copy Markdown
Contributor

👍👍

@narfbg

narfbg commented Jun 14, 2012

Copy link
Copy Markdown
Contributor

Can we get this one going again?
It needs to sync with the latest changes, for a start. :)

@homstie-dev

Copy link
Copy Markdown
Contributor Author

Somebody else do it. I'm pretty disillusioned with the whole process.

On Thu, Jun 14, 2012 at 12:27 PM, Andrey Andreev <
reply@reply.github.com

wrote:

Can we get this one going again?
It needs to sync with the latest changes, for a start. :)


Reply to this email directly or view it on GitHub:
#965 (comment)

@philsturgeon

Copy link
Copy Markdown
Contributor

@atiredmachine don't give up now, it's nearly there. Sometimes you have to bump these things. If you hadn't noticed there are quite a few pull requests knocking around! :)

@philsturgeon

Copy link
Copy Markdown
Contributor

Ignore that, I've merged it without conflict. Good work and sorry for the delay. I remember thinking this had been merged, searching for it and finding nothing.

@philsturgeon philsturgeon merged commit 1b8d0ef into bcit-ci:develop Jun 14, 2012
@philsturgeon

Copy link
Copy Markdown
Contributor

Bug reported related to this change #1499

@svezina

svezina commented Jul 17, 2012

Copy link
Copy Markdown

Hey folks, I'm testing this out and getting an encoding problem for the "à" character. Other french characters seem fine, but this one gives the infamous UTF-8 question mark. Preview : http://www.tribalsolutions.ca/external/minify_encoding.jpg

@narfbg

narfbg commented Jul 17, 2012

Copy link
Copy Markdown
Contributor

@svezina Is that character written in UTF-8 or some other encoding?

@svezina

svezina commented Jul 17, 2012

Copy link
Copy Markdown

@narfbg Yep everything is UTF8 coming from a language file.

I just did a bit more troubleshooting to make sure it wasn't my app that had a problem.

  • I installed a fresh copy of the develop branch.
  • I copied all of the foreign characters from foreign_chars.php in a language file. (Figured I might as well test for all characters.)
  • Loaded it in the default "Welcome to Codeigniter" view.

Here is the result: http://www.tribalsolutions.ca/external/minify2.jpg

ADDED: everything is OK if you don't have spaces before and after characters. It only happens if you have spaces around.

@narfbg

narfbg commented Jul 17, 2012

Copy link
Copy Markdown
Contributor

Well, that makes sense ... Any UTF-8 character that doesn't need to use both of the bytes that represent it is prefixed (or suffixed, don't remeber) by \x0, which is the termination character in single-byte encodings. What minify does is to remove empty characters and this one is probably considered as one.

@svezina

svezina commented Jul 17, 2012

Copy link
Copy Markdown

Thanks for the explanation!

I just made another interesting discovery. It seems to be server-related as well:
XAMPP with php 5.2.9 = BUG
XAMPP with php 5.3 = OK
Shared Hosting with PHP 5.2.17 = OK

I'll just use my 5.3 server for dev work then.

@narfbg

narfbg commented Jul 17, 2012

Copy link
Copy Markdown
Contributor

It's more likely to be related to the PCRE version that you have available on the server than PHP itself.

But anyway ... you can't always have all the exotic features turned on. This feature is way too far from being perfect and I personally would consider it experimental - it was expected that it can cause problems in cases like yours, so for now all I can tell is - if it causes problems, turn it off. :)

narfbg added a commit that referenced this pull request Dec 15, 2014
This feature has proven to be problematic and it's not nearly
as flexible as a dedicated minifier library like Minify
(http://www.minifier.org/, https://github.com/matthiasmullie/minify).

The same results in terms of saving traffic can also be achievied via
gzip compression (which should also be done on the httpd level, but we
also support anyway) and stuff like mod_pagespeed.

Reverts PR #965

Related issues as a track record proving how problematic this has been:

#2078 #1499 #2163 #2092 #2387 #2637 #2710 #2120 #2171 #2631 #2326 #2795
#2791 #2772

Additionally, the count of contributors suggesting that the only way
to fix the minifier problems is to remove it, is around the same as
the count of people suggesting the feature to be implemented in the
first place. It was experimental anyway ... the experiment failed.
ghost pushed a commit to goreilly/CodeIgniter that referenced this pull request Jan 3, 2015
This feature has proven to be problematic and it's not nearly
as flexible as a dedicated minifier library like Minify
(http://www.minifier.org/, https://github.com/matthiasmullie/minify).

The same results in terms of saving traffic can also be achievied via
gzip compression (which should also be done on the httpd level, but we
also support anyway) and stuff like mod_pagespeed.

Reverts PR bcit-ci#965

Related issues as a track record proving how problematic this has been:

bcit-ci#2078 bcit-ci#1499 bcit-ci#2163 bcit-ci#2092 bcit-ci#2387 bcit-ci#2637 bcit-ci#2710 bcit-ci#2120 bcit-ci#2171 bcit-ci#2631 bcit-ci#2326 bcit-ci#2795
bcit-ci#2791 bcit-ci#2772

Additionally, the count of contributors suggesting that the only way
to fix the minifier problems is to remove it, is around the same as
the count of people suggesting the feature to be implemented in the
first place. It was experimental anyway ... the experiment failed.
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.