peruskallio / c5_mainio_minco

Minifier & combiner add-on for concrete5
http://www.mainiotech.fi
MIT License
13 stars 1 forks source link

wrong path to picture in CSS #1

Open Remo opened 11 years ago

Remo commented 11 years ago

After I've added this add-on to a site of mine, I saw some issues with pictures in the CSS file. The path looks like this right now .ccm-menu-icon{background:url(ncrete5.6.1/images/icons_sprite.png) no-repeat top left}

It should have been ../images/icons_sprite.png

Remo commented 11 years ago

Not sure how to fix this but I found the source of the problem. The following script helped me to track down the problem:

$cssFile = DIR_BASE . '/concrete/css/ccm.app.css';
$cssContent = file_get_contents($cssFile);

Loader::library("3rdparty/minify-2.1.5/min/lib/Minify/CSS", "mainio_minco");

echo Minify_CSS::minify($cssContent, array('currentDir' => '/home/remo/public_html/concrete/css'));

The problem is caused by currentDir which in my case points to /concrete/css but is somewhere else in reality. I'm using symlinks to manage my cores. When Minify_CSS tries to find the new path, it probably uses FILE and finds something different and can't correctly determine the new path.

Remo commented 11 years ago

Ah, just when I wanted to close the file I saw this:

return Minify_CSS_UriRewriter::rewrite(
                $css
                ,$options['currentDir']
                ,$options['docRoot']
                ,$options['symlinks']
            );  

Looks like there's a symlinks option!

Remo commented 11 years ago

I was hoping that this would fix the problem. While the url's change, they are still wrong:

$symlinks = array();
// check if concrete5 core is symlinked
$core = DIR_BASE . DIRECTORY_SEPARATOR . 'concrete';
if (is_link($core)) {
    $symlinks[$core] = readlink($core);
}

echo Minify_CSS::minify($cssContent, array('currentDir' => '/home/remo/public_html/concrete/css', 'symlinks' => $symlinks));
ahukkanen commented 11 years ago

It might be just handling the rewriting incorrectly: http://code.google.com/p/minify/wiki/UriRewriting

But I think the problem here is that we're loading the cached copies into the /files directory from the original /themes directory so their URLs should be probably rewritten differently (by pointing the rewrite logic to the original /themes directory instead).

ahukkanen commented 11 years ago

And the reason for loading up the cached copies is to load them through the /index.php/tools which will rewrite the file paths correctly into the CSS files (because if you use $this->getStyleSheet() in your theme, you're naturally using relative URLs in your themes).

ahukkanen commented 11 years ago

So one solution for this would be to replace all /themes/your_theme/ references in the cached CSS copy with relative path of MINCO_RESOURCES_SAVE_DIR but of course that would be quite inefficient since there would then be:

  1. replacement of the url() paths by the concrete5 tool that serves the CSS
  2. replacement of the url() paths by MinCo
  3. replacement of the url() paths by the Minify library

I'm not sure if this could be done in any better way...

ahukkanen commented 11 years ago

And by the way, if you don't use $this->getStyleSheet() in the theme, you probably don't have this problem.

ahukkanen commented 11 years ago

It might be considerable to just rewrite the url() definitions in the CSS by the library and then disable "rewriteCssUris" option from the Minify options. Or do you have any other ideas regarding to this?

Remo commented 11 years ago

I was thinking that we might be able to use some of the core functionality but I haven't been able to have a closer look. I've also done some work there which might be nice to have in this add-on: https://github.com/concrete5/concrete5/pull/886

But in general I'd probably go with "rewriteCssUris" disabled too

ahukkanen commented 11 years ago

Ok, I'll take a look at this with better time. And your image solution seems interesting but one thing I've also noticed is that if you combine everything in 1 file, it might hit you back sometimes in the page load speed if the CSS file size grows too big.

Remo commented 11 years ago

Well, if you reference big background pictures in the CSS, it's probably a bad idea but if you have 20 small icons, you'll get a much better load time.

ahukkanen commented 11 years ago

Yes, agree on that. Just was referencing to the background as many sites might have one defined in the CSS.