canton7 / fuelphp-casset

Better asset management library for fuelphp (with minification!)
MIT License
103 stars 29 forks source link

@Import issues #39

Closed jhuriez closed 11 years ago

jhuriez commented 11 years ago

If i use Casset for CSS with "combine" => true, google fonts from @import are not loaded. Why ?

My line example :

@import url(http://fonts.googleapis.com/css?family=Open+Sans:400,700,300,400italic|Lobster);
canton7 commented 11 years ago

Probably a bug in the minification library. Have a look in the generated CSS file and see whether your @import is being mangled. I suspect it is. Try the latest version of CSSMin.php from https://github.com/mrclay/minify/blob/master/min/lib/CSSmin.php. If that fails, raise an issue with them. If not, I'll upgrade the version I'm using.

As a workaround, you can disable minification for that one file. In your call to Casset::css, pass the file again as the second param (e.g. Casset::css('myfile.css', 'myfile.css')), or, in your config:

...
'files' => array(
   array('myfile.css', 'myfile.css'),
),
...
jhuriez commented 11 years ago

Ok i'll try that tomorrow,

But beware, it's work when i enable "minification" and disable "combine". It's only when i enable "combine" that doesn't work. Even when I enable only the combination and disable the rest (inline, minification, etc..)

canton7 commented 11 years ago

Ah interesting! I wonder what's going wrong - combining is literally concatting the files together. Got a sample that's broken?

jhuriez commented 11 years ago

Yes,

I have 2 files css :

"test1.css"

body {
    background: gray;
}

"test2.css"

@import url(http://fonts.googleapis.com/css?family=Open+Sans:400,700,300,400italic|Lobster);

h2 {
    font-family: Lobster;
}

PHP file :

<?php echo Casset::css('test1.css', false, 'css_test'); ?>
<?php echo Casset::css('test2.css', false, 'css_test'); ?>
<?php echo Casset::render_css('css_test'); ?>

Config casset.php file :

return array(
    'groups' => array(
        'css' => array(
            'css_test' => array(
                'files' => array(),
                'combine' => true,
                'min' => true,
                'inline' => false,
            ),
        ),
    )
);

It's just the combining is wrong. If i do :

<?php echo Casset::css('test2.css', false, 'css_test'); ?>
<?php echo Casset::css('test1.css', false, 'css_test'); ?>
<?php echo Casset::render_css('css_test'); ?>

It's work. I think u need to place "@import" lines at the top of the file in the concates files procedure, i've right ?

canton7 commented 11 years ago

What's the result when those two test files are combined? Just want to make very sure nothing stupid is happening. I won't be near a computer with a php stack on it until this evening.

canton7 commented 11 years ago

Interestingly, looks like Assetic has the same issue: https://github.com/kriswallsmith/assetic/issues/144

jhuriez commented 11 years ago

Maybe use preg_replace_callback for get all line "@import" in static array. And at the end, implode the array at the top of the file ?

protected static $importLines = array();

preg_replace_callback('/@import+.*?\);/', array('TheClass', 'addImportLine'), $content);

protected static function addImportLine($m) {
            self::$importLines[] = $m[0];
            return '';
}

// At the end :
if (!empty(self::$importLines)) {
 $res = implode(PHP_EOL, self::$importLines) . $res;
}

return $res;

I will see that tomorrow

jhuriez commented 11 years ago

Ok, i added these lines in "casset/classes/casset.php", in the function "combine", before the line "file_put_contents" :

                        // Import Lines
                        if ($type == 'css') {
                            preg_replace_callback('/@import+.*?\);/', array('Casset', 'addImportLine'), $content);
                            if (!empty(self::$importLines)) {
                                   $content = implode(PHP_EOL, self::$importLines) . PHP_EOL . $content;
                                   self::$importLines = array();
                            }
                        }

Add the array in static :

        public static $importLines = array();

And the function callback :

        protected static function addImportLine($m) {
            self::$importLines[] = $m[0];
            return '';
        }

It's work fine, but i don't know if it's a good practise ?

canton7 commented 11 years ago

Right! I finally have time to look at this properly. Thanks for being patient.

It looks like @import doesn't have to end with a ), so your regex is a little off. Since fuel requres PHP 5.3, we can use a lambda in the callback as well.

I've added a new commit to the develop branch - can you check it does what you expect? If so I'll merge it into master and cut a new release.

canton7 commented 11 years ago

I'm still not hugely happy with the regex, as it'll take a commented-out @import line and effectively uncomment it. That's probably rare enough that we can ignore it though... Maybe...

jhuriez commented 11 years ago

Thank's for your commit, it's working fine.

Yep the uncommented @import line is an issue... We can delete comments before records @import lines, like csscompressor function (it's work if we set "minify" to true), but this is not really the purpose of the value "combine"

canton7 commented 11 years ago

Yeah, if we're minifying then comments are stripped by that point anyway, so it's a non-issue. If PHP's regex supports negative and positive lookbehinds it'll be easy enough to add, but I'm still not sure if it's worth it. I'll have a look at some point.

canton7 commented 11 years ago

OK, I've merged this into master and cut a new release. Thanks for your help!