Before I start testing the php4, I was going to see if I could get everything working with the php5 setup when I found this problem.

<style type="text/css" media="all">@import "/ictures/modules/core/data/gallery.css";</style>
<style type="text/css" media="all">@import "/ictures/modules/icons/iconpacks/silk/icons.css";</style>
<style type="text/css" media="all">@import "/ictures/themes/matrix/theme.css";</style>
<style type="text/css" media="all">@import "/link rel="stylesheet" href="/lib/slimbox/css/slimbox.css" type="text/css" media="screen" />";</style>
<style type="text/css" media="all">@import "/ictures/main.php?g2_view=imageframe.CSS&amp;g2_frames=none";</style>
<style type="text/css" media="all">@import "/files/css/bbef32b5adbd7043bb3723aab1c32095.css";</style>

Most of those lines are simply missing a "p" between the "/" and the "i", but the one with @import "/link rel="stylesheet" href="/lib/slimbox/css/slimbox.css" type="text/css" media="screen" />"; looks pretty messed up. I'm using the matrix theme that has an extra link tag in the header(theme.tpl), placed immediately below the default one.

Thanks for your help. Let me know if it's my customized theme or something else...

CommentFileSizeAuthor
#23 js_css_include5b.patch2.93 KBprofix898
#22 js_css_include5.patch3.02 KBprofix898
#18 js_css_include4.patch2.24 KBsewpafly
#10 js_css_include3.patch2.07 KBprofix898
#6 js_css_include2.patch1.96 KBprofix898
#5 js_css_include.patch1.26 KBprofix898
#4 css_include.patch867 bytesprofix898

Comments

profix898’s picture

profix898 commented

Thanks for your help. I recently modified the css parser, so it is likely there is a bug in the code.

What are your coding skills? Do you know how to apply a patch? Dont get me wrong, I'm just asking to find the most effective way for us to communicate. Should I simply add the patch to cvs directly or would you prefer the patch posted here for review?

profix898’s picture

profix898 commented

Can you please post the css lines of your standalone gallery for comparison?

sewpafly’s picture

sewpafly commented

I'm fairly technical (I am a software engineer) and I don't fear patches. I would suggest doing whatever is easiest, since that is my usual method...

Standalone HTML header - http://drupalgallery.1t2.us/1247
Embedded HTML header - http://drupalgallery.1t2.us/1249

For what it's worth, the embedded main (e.g. ?q=gallery) doesn't have these problems, it's only the sub albums.

profix898’s picture

profix898 commented
Status: Active » Needs review
StatusFileSize
new css_include.patch867 bytes

I dont know why, but G2 return different paths for css for the root album and subalbums (incl. admin pages). Here is a patch to unify the paths before inclusion in Drupal.

profix898’s picture

profix898 commented
StatusFileSize
new js_css_include.patch1.26 KB

Same is true for javascript also ...

profix898’s picture

profix898 commented
StatusFileSize
new js_css_include2.patch1.96 KB

... taking a look at http://drupal.org/node/145284 ... inline styles are not recognized and therefore inserted corrupted ... Attached patch solves both issues (1. wrong include paths, 2. unrecognized inline css). Hope it covers all cases now. Works with Matrix and Siriux for me.

sewpafly’s picture

sewpafly commented

I applied the first patch before I got the message about the second one and so I started investigating how to checkout the latest from CVS and I tried using the -r "DRUPAL-5--2" switch but obviously that doesn't get the latest changes... Do you know what tag should be used for the latest stuff?

profix898’s picture

profix898 commented

An anonymous checkout should work with ...

cvs -q checkout -r DRUPAL-5--2 -P contributions/modules/gallery
CVSROOT=:pserver:anonymous:anonymous@cvs.drupal.org:/cvs/drupal-contrib

At least it works for me. If you have a cvs account on drupal.org you can (of course) use your own username:password instead of anonymous:anonymous.

sewpafly’s picture

sewpafly commented

The patch solved everything except for the one really weird line:

<style type="text/css" media="all">@import "/link rel="stylesheet" href="/lib/slimbox/css/slimbox.css" type="text/css" media="screen" />";</style>

I then changed it in the theme from:

       <link rel="stylesheet" href="/lib/slimbox/css/slimbox.css" type="text/css" media="screen" />

to

       <link rel="stylesheet" type="text/css" href="/lib/slimbox/css/slimbox.css" media="screen" />

And everything appears normally.

profix898’s picture

profix898 commented
StatusFileSize
new js_css_include3.patch2.07 KB

Oh, yes. The regexp is incorrect. This one should work. Thanks for testing :)

profix898’s picture

profix898 commented

It's getting late where I am (Germany). I will commit the patch tomorrow if no more problems appear the next hours ...
FYI: You should take advantage of the 'status' field when updating issues, e.g. 'patch (code needs work)', etc.

sewpafly’s picture

sewpafly commented
Status: Needs review » Needs work

I don't know what happened, but when I tried the latest patch everything broke and I haven't been able to fix it either by rolling back the patches and/or reapplying any of the patches. I'm pretty confused. I'll look at it some more tomorrow.

Dahaniel’s picture

Dahaniel commented
Title: @import rules are incorrect » Question for patching

I never applied a patch before. I found the book page here on drupal.org and understand that I would need to download a tool for this. No problem so far, nut do I have to applie your patches one after another or is it enough to applie the newest?

Dahaniel’s picture

Dahaniel commented
Title: Question for patching » import rules are incorrect

I never applied a patch before. I found the book page here on drupal.org and understand that I would need to download a tool for this. No problem so far, nut do I have to applie your patches one after another or is it enough to applie the newest?

profix898’s picture

profix898 commented

@sewpafly: "I haven't been able to fix it either by rolling back"
It was just a small change compared to patch in #2, I dont see why this should break everything ...

@Dahaniel: "do I have to applie your patches one after another or is it enough to applie the newest?"
All patches are against the latest cvs version (DRUPAL-5--2 branch), they are just revisions. Its enough to apply the most recent one.
(Tell us if you have trouble applying patches, its really easy once you know how.)

sewpafly’s picture

sewpafly commented

Okay, I really don't know what's going on. On one computer (work) it looks fine, but on another (home) it's encountering the same old issues... Any ideas? I tried logging in and out and flushing gallery's cache. Drupal's caching feature is disabled. Could it be something else?

profix898’s picture

profix898 commented

@sewpafly: Do you have trouble only with the latest patch or with the others as well in your 'home' environment? I cant think of a good reason for this different behaviour. I hope you will tell me shortly that it is not related to the patch but to your infrastructure ... ;)

sewpafly’s picture

sewpafly commented
Status: Needs work » Needs review
StatusFileSize
new js_css_include4.patch2.24 KB

Okay, so I took a couple minutes out from work to look at this. Here's what I found: the drupal hooks want everything from the base_path(), but the GalleryEmbed class will give you the full path to the file if you use something other than www.site.name. At least on my system when you access from http://site.name/?q=gallery, it hates that and tries to send you the full paths of the css and js files. So I hammered out a fix that solves the problem for css by using the drupal_add_link function (Which doesn't add an <a href>, but a normal link tag). However, I couldn't get the javascript working perfectly,

My code looks like this:

   drupal_set_html_head("<script type=\"text/javascript\" src=\"$include\"></script>");

in place of

   $include = preg_replace("/^".preg_quote(base_path(),'/')."/","",$include);
   drupal_add_js($include);

But that puts the javascript files out of order, and I don't know if that causes other problems. Mine appears to work perfectly this way however. Here's a patch file that contains my changes

profix898’s picture

profix898 commented
Status: Needs review » Needs work

Your patch uses drupal_set_html_head() (or drupal_add_link() which uses drupal_set_html_head() internally). There are three issues with this approach:

  1. The includes dont cascade properly. That shouldnt be much of a problem for css, because it will still be included before the Drupal css and we can still override styles from Drupal modules. But for js it means that e.g. jquery.js will be included after the gallery js. And from my experience this causes several problems (and did so in the past, e.g. with lightbox2 modules/themes).
  2. drupal_set_html_head() does not check for duplicate includes. If you add 10 gallery image blocks to your page (what is possible in the latest version), you will have css/js included 10 times also.
  3. drupal_add_css/js() also allows Drupal to cache, preprocess or even compress css/js before inclusion and therefore to speedup/optimize page loads

I think we need some simple logic to handle different css/js paths returned by Gallery2. Actually I dont understand why G2 return different paths for different urls (with/without www. and even for album/subalbum) at all. This should probably be discussed with the G2 devs.

profix898’s picture

profix898 commented

There are four different cases to handle (applies to css and javascript paths):

// #1 root album
<link rel="stylesheet" type="text/css" href="/drupal5/gallery2/modules/core/data/gallery.css"/>
// #2 subalbum
<link rel="stylesheet" type="text/css" href="gallery2/modules/core/data/gallery.css"/>
// #3 subalbum (different)
<link rel="stylesheet" type="text/css" href="/gallery2/modules/core/data/gallery.css"/>
// #4 url without www.
<link rel="stylesheet" type="text/css" href="http://www.domain.net/drupal5/gallery2/modules/core/data/gallery.css"/>

They all must be transformed into case #2 (relative to base_path() which is /drupal5/ in this example). Am I missing something here? Any missing cases? Its really strange why G2 doesnt return uniform references ...

sewpafly’s picture

sewpafly commented

That sounds about right. On my site there I see three, but I might see a different one if my base_path() wasn't '/'.

  1. http://www.site.name/gallery/file.ext
  2. /path_to_separate_libs/file.ext - I have these because I modified the theme to include an additional CSS file and 2 additional JS files starting with the "/", but I could have alternately used the long form that was used above (http://www.site.com/path_to_separate_libs/file.ext).
  3. /gallery/filename.ext
profix898’s picture

profix898 commented
Status: Needs work » Needs review
StatusFileSize
new js_css_include5.patch3.02 KB

Ok, here is a another patch to get this right! It adds the ability to include external file paths (with redundancy check) in case G2 is installed on a different host, but uses drupal_add_css/js() if possible.

profix898’s picture

profix898 commented
StatusFileSize
new js_css_include5b.patch2.93 KB

Similar patch to #22 with some smaller changes (use of md5, always check redundancy).

sewpafly’s picture

sewpafly commented
Status: Needs review » Reviewed & tested by the community

Looks good from here.

profix898’s picture

profix898 commented
Status: Reviewed & tested by the community » Fixed

Committed to Drupal-5--2 branch. Thanks a lot for review sewpafly.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)