Improved and fixed minify() function #2795

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants

invisal commented Dec 27, 2013

Properly remove

  <!-- <script></script> -->

Prevent from removing multiple space from tag attribute. For example:

 <input type='text' value='Hello          World'>
Improved and fixed minify() function
(*) Properly remove <!-- <script></script> -->
(*) Prevent from removing multiple space from tag attribute. For example: <input type='text' value='Hello          World'>

invisal commented Dec 27, 2013

The content of a tag is compressed.

This can be easily added.

I wonder if CI should really be attempting to do HTML minification in the first place. Conventional wisdom (supported by issues such as this) seems to be that trying to match HTML tags with regex is a bad idea and I doubt a full HTML parser is within the scope of CI.

I do agree that it is beyond the scope of CI. Writing accurate and proper "HTML minification" takes great effort (studying all the possible case and able to parse ill-form HTML take great effort). Since, our current minification cannot ensure the accuracy and the accuracy is crucial, it is the same as not having the feature at the first place.

This regex works well (removes spaces between elements) and it does not touch script tags (unsafe if users miss semi-colons): $output = preg_replace( '/(?:(?<=>)|(?<=/>))(\s+)(?=</?)/', '', $output );

I've been using it for a while now (in CI3) as the current implementation breaks inline-block layouts.

licarna commented Jun 5, 2014

I haven't tried this fix yet, but the old one is definitely easily broken. HTML comments like this (although valid) will cause elements on the page to go missing. Obviously the issue is with the closing !--> but I've seen a few more instances of stuff like this.

<div class="row">
        <div class="col-sm-7">
                        <p>Hello World</p>
                        <p>
                            <!-- This comment will cause elements to go missing!-->
                            Hello world is fun
                        </p>
                        <div class="row" >
                            <div class="col-xs-6">A</div>
                            <div class="col-xs-6">B</div>
                        </div>
           </div>
</div>
Contributor

narfbg commented Jun 5, 2014

HTML comments are supposed to be closed via /-->.

licarna commented Jun 5, 2014

In all browsers i've seen --> closes HTML comments. It just so happens someone had put an exclamation point to emphasize a word before closing the comments and then a whole bunch of elements were missing when minifying with codeigniter. Its just an edge case that you may want to include in your tests or account for in your regex. The original code also breaks javascript code with comments. I can send that too if you want.

Anyway, i was just letting you know.

BTW, i love CI. For me, it's the perfect framework. Thanks for all your hard work.

Contributor

mwhitneysdsu commented Jun 5, 2014

http://www.w3.org/TR/html-markup/syntax.html#comments

Comments consist of the following parts, in exactly the following order:
1. the comment start delimiter "<!--"
2. text
3. the comment end delimiter "-->"

narfbg added a commit that referenced this pull request Dec 15, 2014

Remove output minifier
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.

@narfbg narfbg closed this Dec 15, 2014

ghost pushed a commit to goreilly/CodeIgniter that referenced this pull request Jan 3, 2015

Remove output minifier
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment