Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Issue #2078: refinement of the minify function for CSS and scripts. #2079

Merged
merged 5 commits into from Dec 19, 2012

Conversation

Projects
None yet
2 participants
Contributor

AndrewPodner commented Dec 18, 2012

Created a protected method for minification of CSS and Javascript, whether a file type or inside script and style tags. The idea was to isolate the opening and closing tags (if applicable) and then walk each character of the remaining file contents. If the method detects that a string is opened with a single or double quote, stripping of spaces is switched off until the string is closed with another of that same quote.

This should cure the observations made in issue #2078.

Contributor

narfbg commented Dec 18, 2012

Please use tabs for indentation, not spaces. See our styleguide for more tips.

Contributor

AndrewPodner commented Dec 18, 2012

That should clean it up. Sorry about that, wrong setting in IDE

@narfbg narfbg and 1 other commented on an outdated diff Dec 18, 2012

system/core/Output.php
+ }
+
+ // Remove CSS comments
+ $output = preg_replace('!/\*[^*]*\*+([^/][^*]*\*+)*/!', '', $output);
+
+ // Remove spaces around curly brackets, colons,
+ // semi-colons, parenthesis, commas
+ $output = preg_replace('!\s*(:|;|,|}|{|\(|\))\s*!', '$1', $output);
+
+ // Remove spaces
+ $in_string = FALSE;
+ $in_dstring = FALSE;
+ $array_output = str_split($output);
+ foreach ($array_output as $key => $value)
+ {
+ if ($in_string === FALSE and $in_dstring === FALSE)
@narfbg

narfbg Dec 18, 2012

Contributor

Again, per the style guide: and -> &&

@AndrewPodner

AndrewPodner Dec 18, 2012

Contributor

Will fix and recommit

@narfbg narfbg and 1 other commented on an outdated diff Dec 18, 2012

system/core/Output.php
+ // Remove CSS comments
+ $output = preg_replace('!/\*[^*]*\*+([^/][^*]*\*+)*/!', '', $output);
+
+ // Remove spaces around curly brackets, colons,
+ // semi-colons, parenthesis, commas
+ $output = preg_replace('!\s*(:|;|,|}|{|\(|\))\s*!', '$1', $output);
+
+ // Remove spaces
+ $in_string = FALSE;
+ $in_dstring = FALSE;
+ $array_output = str_split($output);
+ foreach ($array_output as $key => $value)
+ {
+ if ($in_string === FALSE and $in_dstring === FALSE)
+ {
+ if ($value == ' ')
@narfbg

narfbg Dec 18, 2012

Contributor

Use === here, just in case of a zero, empty string, etc.
Could do the same for lines 850, 855 although it wouldn't matter there.

@AndrewPodner

AndrewPodner Dec 18, 2012

Contributor

will do

@narfbg narfbg and 1 other commented on an outdated diff Dec 18, 2012

system/core/Output.php
+ $in_string = FALSE;
+ $in_dstring = FALSE;
+ $array_output = str_split($output);
+ foreach ($array_output as $key => $value)
+ {
+ if ($in_string === FALSE and $in_dstring === FALSE)
+ {
+ if ($value == ' ')
+ {
+ unset($array_output[$key]);
+ }
+ }
+
+ if ($value == "'")
+ {
+ $in_string = !$in_string;
@narfbg

narfbg Dec 18, 2012

Contributor

Spaces must be put around each occurence of the negation operator (in this case, just add one between it and the variable).
Same goes for line 857

@narfbg narfbg commented on an outdated diff Dec 18, 2012

system/core/Output.php
+
+ if ($value == '"')
+ {
+ $in_dstring = !$in_dstring;
+ }
+ }
+
+ $output = implode($array_output);
+
+ // Remove breaklines and tabs
+ $output = preg_replace('/[\r\n\t]/', '', $output);
+
+ // Put the opening and closing tags back if applicable
+ if (isset($open_tag))
+ {
+ $output = $open_tag . $output . $closing_tag;
@narfbg

narfbg Dec 18, 2012

Contributor

No spaces around the concatenation dots please. :)
I'm not sure if the styleguide says anything about it, but it's done without spaces everywhere in CI - let's be consistent. :)

Contributor

AndrewPodner commented Dec 18, 2012

@narfbg I had one other comment in my inbox regarding strpos, but it is not in this list. Was it removed or did you want to discuss it further?

Contributor

narfbg commented Dec 18, 2012

I had missed a condition when I wrote it - removed it afterwards, ignore it. :)

Contributor

AndrewPodner commented Dec 18, 2012

Ok, just wanted to make sure before I send this commit. Thanks! :)

@narfbg narfbg and 1 other commented on an outdated diff Dec 18, 2012

system/core/Output.php
@@ -782,22 +782,91 @@ public function minify($output, $type = 'text/html')
case 'text/css':
case 'text/javascript':
- //Remove CSS comments
- $output = preg_replace('!/\*[^*]*\*+([^/][^*]*\*+)*/!', '', $output);
+ $output = $this->_minify_scripts_css($output, $type);
@narfbg

narfbg Dec 18, 2012

Contributor

Sorry to bounce it back again, but this method doesn't exist. _minify_script_style() maybe?

@AndrewPodner

AndrewPodner Dec 18, 2012

Contributor

Nope, my fault. renamed the method at the last minute. Thanks for catching it. Eventually I will get better at this! :)

Contributor

AndrewPodner commented Dec 18, 2012

@narfbg Should be clean now, thanks for the help and guidance

@narfbg narfbg commented on an outdated diff Dec 18, 2012

system/core/Output.php
@@ -728,13 +728,13 @@ public function minify($output, $type = 'text/html')
preg_match_all('{<style.+</style>}msU', $output, $style_clean);
foreach ($style_clean[0] as $s)
{
- $output = str_replace($s, $this->minify($s, 'text/css'), $output);
+ $output = str_replace($s, $this->_minify_script_style($s, $type), $output);
@narfbg

narfbg Dec 18, 2012

Contributor

Isn't $type always going to be 'text/html' here? Is that correct? (same on line 737)

Contributor

AndrewPodner commented Dec 18, 2012

@narfbg Based on your comment above, would it make more sense if I changed the 2nd parameter to a boolean? TRUE would be that the output has tags that need to be dealt with, and FALSE would be no tags.

Contributor

narfbg commented Dec 19, 2012

Probably, if you think that would be better. I only had the concern that it was effectively ignored (since 'text/html' is always passed to it).

Contributor

AndrewPodner commented Dec 19, 2012

I think I will go ahead with the boolean, it is a little cleaner, and I agree, the variable was not needed.

@narfbg narfbg added a commit that referenced this pull request Dec 19, 2012

@narfbg narfbg Merge pull request #2079 from AndrewPodner/develop
Issue #2078: refinement of the minify function for CSS and scripts.
a916250

@narfbg narfbg merged commit a916250 into bcit-ci:develop Dec 19, 2012

1 check failed

default The Travis build failed
Details

@narfbg narfbg added a commit that referenced this pull request Dec 19, 2012

@narfbg narfbg [ci skip] Some micro-optimizations and style changes
(following PRs #2049, #2079)
72ed4c3

@nonchip nonchip pushed a commit to nonchip/CodeIgniter that referenced this pull request Jun 29, 2013

@narfbg narfbg Merge pull request #2079 from AndrewPodner/develop
Issue #2078: refinement of the minify function for CSS and scripts.
4f5cfa1

@nonchip nonchip pushed a commit to nonchip/CodeIgniter that referenced this pull request Jun 29, 2013

@narfbg narfbg [ci skip] Some micro-optimizations and style changes
(following PRs #2049, #2079)
315c303
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment