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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jox’s picture

Status: Active » Needs review
FileSize
1.44 KB
jox’s picture

Is 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/'.

jox’s picture

Title: drupal_load_stylsheet base path mess with @import » drupal_load_stylsheet() does not comply with standards when using @import
Version: 7.x-dev » 8.x-dev
Component: base system » file system
Priority: Normal » Major
FileSize
1.46 KB

This 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?

xjm’s picture

Issue tags: +Needs tests

Thanks 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?

xjm’s picture

Priority: Major » Normal
jox’s picture

Thanks for your input!

Ok, sorry, it's a bit difficult to describe. I'm trying it more generally:

These are the requirements:

  1. For any (also nested) @import statement, the path that is used as base path (or current directory) to resolve a relative url, must be the path (directory) of the file containing the @import statement.
  2. This base path must not be modified by any preceding @import statements (in the same file).

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:

  1. Wrong filenames are given to drupal_load_stylesheet() in two places.
  2. One variable is named incorrectly.
  3. The content of one "expected output" file (to compare the results with) is faulty. But since it gets compared to itself (see 1.) the test will always succeed anyway.

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"?

Status: Needs review » Needs work

The last submitted patch, drupal_load_stylesheet-1198904-6-test-only.patch, failed testing.

jox’s picture

Status: Needs work » Needs review

Fail was expected.

das-peter’s picture

Just 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.

Status: Needs review » Needs work
jox’s picture

Status: Needs work » Needs review
FileSize
4.9 KB
6.04 KB

@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.

Status: Needs review » Needs work

The last submitted patch, drupal_load_stylesheet-1198904-11-test-only.patch, failed testing.

xjm’s picture

Issue tags: -Needs tests
+++ b/core/includes/common.incundefined
@@ -3576,17 +3576,22 @@ function drupal_load_stylesheet($file, $optimize = NULL, $reset_basepath = TRUE)
+  // Set the current base path to process prossible child imports.  ¶

Bit of trailing whitespace here.

Untagging as it has a test. Thanks!

das-peter’s picture

@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.

webchick’s picture

Thanks 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.

jox’s picture

jox’s picture

Status: Needs work » Needs review
a.ross’s picture

Any 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

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs backport to D7, +Needs reroll

Looks 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.

pwieck’s picture

Issue tags: -Needs reroll

Removing reroll tag

das-peter’s picture

Status: Needs work » Needs review
FileSize
7.41 KB

Here's a re-rolled version.

fubhy’s picture

Status: Needs review » Needs work

This needs to be re-rolled based on CssOptimizer. @see #352951: Make JS & CSS Preprocessing Pluggable

das-peter’s picture

Here we go.
Patch with tests only should fail.
Full patch should pass.

Status: Needs review » Needs work
Issue tags: -Needs backport to D7

The last submitted patch, d8-CssOptimizer-fix-import-handling-1198904.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7
Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +CSS aggregation

Awesome!

Only a tiny nitpick. Please change:

+      // Subfolder file. Tests:
+      // - CSS import path is properly interpreted.

to:

+      // File in subfolder. Tests:
+      // - CSS import path is properly interpreted. (https://drupal.org/node/1198904)

Then it's consistent with the other test cases. Once that is changed, I'll RTBC :)

das-peter’s picture

Status: Needs work » Needs review
Issue tags: -CSS aggregation
FileSize
689 bytes
4.07 KB

@Wim Leers: Thanks for the review!
And here we go, nitpick fixed.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +CSS aggregation
Wim Leers’s picture

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 8.x.

Moving to 7.x for backport.

Wim Leers’s picture

Yay, thanks! :)

das-peter’s picture

Status: Patch (to be ported) » Needs review
FileSize
4.65 KB

D7 Backport.

Status: Needs review » Needs work

The last submitted patch, drupal_load_stylesheet-1198904-32.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
7.17 KB

Oh, looks like I missed some parts :)

JohnAlbin’s picture

Version: 7.x-dev » 7.9
Component: file system » forms system
Status: Needs review » Reviewed & tested by the community

This 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.

JohnAlbin’s picture

Version: 7.9 » 7.x-dev
Component: forms system » file system

Not sure how the version/component got changed. fixing.

JohnAlbin’s picture

Title: drupal_load_stylsheet() does not comply with standards when using @import » drupal_load_stylsheet() fails to load @import files in different directories
Wim Leers’s picture

Yay :)

criscom’s picture

When will this go into core?

Anonymous’s picture

Does this also fix the following case,

Fails when aggregation is enabled (works without):

@import url("//fonts.googleapis.com/css?family=Aclonica:400,700");

Always works:

@import url("https://fonts.googleapis.com/css?family=Aclonica:400,700");

(Difference is // versus https://)

Anonymous’s picture

Issue summary: View changes

Fixed wrong path in example.

Garrett Albright’s picture

Issue summary: View changes

Further confirmation that #34 fixes the problem with regards to Zen. Looking forward to seeing this fix in the next core release.

Garrett Albright’s picture

Further confirmation that #34 fixes the problem with regards to Zen. Looking forward to seeing this fix in the next core release.

Garrett Albright’s picture

Title: drupal_load_stylsheet() fails to load @import files in different directories » drupal_load_stylesheet() fails to load @import files in different directories

Correcting typo in title for improved searchability.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

melissavdh’s picture

In 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.

Garrett Albright’s picture

melissadvh, could you give us more info? What were the compiled CSS @import commands coming out as?