If a css file is included with an @import statement and that file is not in the same directory as the including file,
the base path changes to the path of the included file for futher parsing (which is desired).
When the included file is through, parsing continues in the including file but the base path is still
the one from the included file. This is a bug.
As a simple example let's assume the following files
/www/themes/mytheme/css/main.css
/www/themes/mytheme/css/a/a.css
/www/themes/mytheme/css/b/b.css
With main.css as follows:
/*
* Base dir is '/www/themes/mytheme/css/'.
* The following import will work as expected.
*/
@import url(a/a.css);
/*
* Base dir is now '/www/themes/mytheme/css/a/'.
*
* The following import will not work!
*/
@import url(b/b.css);
/*
* Instead this would work:
*/
@import url(../b/b.css);
This can be fixed by introducing a stack in drupal_load_stylesheet() (includes/common.inc) that saves and restores the base paths
for each recursive call.
Patch follows.
Comments
Comment #1
jox CreditAttribution: jox commentedComment #2
jox CreditAttribution: jox commentedIs that true? Own issue posts can not be edited? Only comments?Here is a correction. The line* Base dir is now '/www/themes/mytheme/css/otherdir/'.
should be* Base dir is now '/www/themes/mytheme/css/a/'.
Comment #3
jox CreditAttribution: jox commentedThis is six months old now. Giving this another go and targeting D8 first. Changed title to what this is really about.
Is there anything else needed?
Comment #4
xjmThanks for the patch! Based on the issue priority documentation, I believe this is a normal bug.
I've read the summary several times and I still don't quite understand the circumstances needed to trigger this bug, so maybe it would help to add some inline comments to the code in the patch. We'll also need an automated test based on the bug that fails without the patch and passes with it. Could you provide steps to reproduce the issue, or a test theme that demonstrates it?
Comment #5
xjmComment #6
jox CreditAttribution: jox commentedThanks for your input!
Ok, sorry, it's a bit difficult to describe. I'm trying it more generally:
These are the requirements:
Both cases are not garanteed in the current state.
There is already some tests checking drupal_load_stylesheet() (System -> CSS Unit Tests). It's funny though (not really), the tests contain some serious mistakes:
So I fixed those and added one round (4 tests) for my case. Also added some inline comments.
A patch with everything and one with only the (fixed/new) tests is attached.
Is there still need for "steps to reproduce the issue, or a test theme that demonstrates it"?
Comment #8
jox CreditAttribution: jox commentedFail was expected.
Comment #9
das-peter CreditAttribution: das-peter commentedJust hit this problem in D7 and solved it slightly different.
I don't see a need for an additional static variable. Because the function runs recursive it should be just fine to reset the base path to the parent one as soon as the file is processed.
All I've added is another variable to keep the parent base path and reset the base path later with it.
Comment #11
jox CreditAttribution: jox commented@das-peter: Yes you're right, thanks. Just slapped my forhead.
I lost the focus on the recursiveness somehow due to the nested function calling. And since we're recursive we can implicitly use _that_ stack (simply by using a local variable).
One more slap.
This issue is set to D8. That's why your patch failed.
I updated my patch against current 8.x-git, which also includes the fixed test cases.
Comment #13
xjmBit of trailing whitespace here.
Untagging as it has a test. Thanks!
Comment #14
das-peter CreditAttribution: das-peter commented@jox Thanks for re-rolling :)
Did something change with the testbot? I added "-d7" as suffix to the patch name. This should exclude the patch from testing as documented: "...Patches are skipped if the file name (before the .patch or .diff extension) ends with something like -d5 or _d4. For example somefile-d9.patch and somefile_d2.patch is skipped..."
Advertising :P : Drupal Code Sniffer really helps to avoid coding standard violations and thus annoying patch re-rolls just because of silly whitespaces.
Comment #15
webchickThanks for finding that documentation, which is no longer accurate. This was recently changed to "do-no-test" which is supposed to make a lot more sense. :)
Updated the docs. The description on the actual filefield is correct.
Comment #16
jox CreditAttribution: jox commented#13
Comment #17
jox CreditAttribution: jox commentedComment #18
a.ross CreditAttribution: a.ross commentedAny news on this? This is especially annoying when working with CSS preprocessors, like LESS.
Some related issues:
#1350646: @import from another directory not being included
#1331144: Multiple @import does not use expected pathing
Comment #19
xjmLooks like the patch in #16 is a year stale -- the first step is to update it to apply to the current 8.x branch. The test changes will need to be made manually, as all tests have moved around following the PSR-0 conversion.
Comment #20
pwieck CreditAttribution: pwieck commentedRemoving reroll tag
Comment #21
das-peter CreditAttribution: das-peter commentedHere's a re-rolled version.
Comment #22
fubhy CreditAttribution: fubhy commentedThis needs to be re-rolled based on CssOptimizer. @see #352951: Make JS & CSS Preprocessing Pluggable
Comment #23
das-peter CreditAttribution: das-peter commentedHere we go.
Patch with tests only should fail.
Full patch should pass.
Comment #25
das-peter CreditAttribution: das-peter commented#23: d8-CssOptimizer-fix-import-handling-1198904.patch queued for re-testing.
Comment #26
Wim LeersAwesome!
Only a tiny nitpick. Please change:
to:
Then it's consistent with the other test cases. Once that is changed, I'll RTBC :)
Comment #27
das-peter CreditAttribution: das-peter commented@Wim Leers: Thanks for the review!
And here we go, nitpick fixed.
Comment #28
Wim LeersComment #29
Wim Leers#511998: CSS aggregator change url path in imported files was a duplicate of this.
Comment #30
catchCommitted/pushed to 8.x.
Moving to 7.x for backport.
Comment #31
Wim LeersYay, thanks! :)
Comment #32
das-peter CreditAttribution: das-peter commentedD7 Backport.
Comment #34
das-peter CreditAttribution: das-peter commentedOh, looks like I missed some parts :)
Comment #35
JohnAlbinThis is totally breaking Zen 7.x-5.3 when used with just CSS and not the Sass. See #2058113: choosing "Aggregate and compress CSS files" disrupts styling
The patch in #34 totally fixes the problem. And the code changes to drupal_load_stylesheet() look good to go.
Comment #36
JohnAlbinNot sure how the version/component got changed. fixing.
Comment #37
JohnAlbinComment #38
Wim LeersYay :)
Comment #39
criscomWhen will this go into core?
Comment #40
Anonymous (not verified) CreditAttribution: Anonymous commentedDoes this also fix the following case,
Fails when aggregation is enabled (works without):
Always works:
(Difference is // versus https://)
Comment #40.0
Anonymous (not verified) CreditAttribution: Anonymous commentedFixed wrong path in example.
Comment #41
Garrett Albright CreditAttribution: Garrett Albright commentedFurther confirmation that #34 fixes the problem with regards to Zen. Looking forward to seeing this fix in the next core release.
Comment #42
Garrett Albright CreditAttribution: Garrett Albright commentedFurther confirmation that #34 fixes the problem with regards to Zen. Looking forward to seeing this fix in the next core release.
Comment #43
Garrett Albright CreditAttribution: Garrett Albright commentedCorrecting typo in title for improved searchability.
Comment #44
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/16fe2e9
Comment #46
melissavdh CreditAttribution: melissavdh commentedIn case this may help anyone who's styling seems a bit broken after the upgrade...
My theme uses LESS and lots of @import statements. After the upgrade to 7.25 some of my styling did not come through to the page and after lots of digging I found this issue. In order to fix my problem I have had to go through my LESS files and correct some of the @import statements - although they all worked in 7.24 some of them did not reference correctly in 7.25 and therefore the files were not imported.
Comment #47
Garrett Albright CreditAttribution: Garrett Albright commentedmelissadvh, could you give us more info? What were the compiled CSS @import commands coming out as?