Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.http://drupal.org/node/225125 says that color.module data in themes isn't inherited. Comments on that page suggest copying color/, images/ and styles.css to the subtheme directory, which is a waste of space. That is not true: with the exception of color.inc, which must be copied, it *is* possible to inherit color.module information. For example, in Minnelli color/color.inc:
'css' => array(
'../style.css',
),
[...]
'slices' => array(
'../images/body.png' => array(0, 37, 1, 280),
'../images/bg-bar.png' => array(202, 530, 76, 14),
[...]
This is made possible by line 443 of color.module, which replaces "some-directory/../" with "".
$before = preg_replace('`(^|/)(?!../)([^/]+)/../`', '$1', $before);
Unfortunately, it only replaces one instance of '../'. This is a problem with themes inside sites/all/themes/.
To demonstrate: Copy minelli/ to sites/all/themes/myexample/, create an approporiate myexample.info, and in color/color.inc, replace '../' with '../../../../themes/garland/'. (So ../style.css becomes ../../../../themes/garland/style.css.) Recoloring it won't work: the recolored CSS will have correct colors (e.g. text color, solid background color), but all the image slices it references (gradients etc) will be wrong. Line 443 does exactly that - replace addresses of recolored image slices.
A simple, though inelegant solution is to wrap the line in a while loop. It's probably possible to do it entirely with regular expressions, but this code isn't exactly performance critical.
while (preg_match('`(^|/)\.\./`', $before)) {
$before = preg_replace('`(^|/)(?!\.\./)([^/]+)/\.\./`', '$1', $before);
}
With this change, all instances of '../' are replaced correctly and the theme can be recolored.
I heard somewhere that color.module may go to contrib in 7.x, but this is still applicable for 6.x. The fix doesn't break any API and nobody relies on the old behavior (because the old behavior was half of the page colored incorrectly).
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | 466268.subtheme-inheritance.11.patch | 792 bytes | dlu |
| #1 | color-regex-D6.patch | 562 bytes | Tomi |
Comments
Comment #1
Tomi CreditAttribution: Tomi commentedAn attempt at a patch against 6.12.
Oh, and it seems I've got the line number wrong. It's not 443, but somewhere nearby.
Comment #2
Tomi CreditAttribution: Tomi commentedI gather that bugs are first fixed upstream, then backported. Setting to 7.x to get more attention for this issue.
Comment #4
Roi Danton CreditAttribution: Roi Danton commentedPatch does work as intended for Drupal 6.12. Should be rerolled for newer versions.
Comment #5
webchickThere should be a space after while for conformance to coding standards.
However, this fix looks rather ugly. :\ Is there really no other way to do this check?
Finally, anywhere we're dealing with regexes, we should have a brief summary comment as to what they're doing so someone can get the jist without having to read punctuation.
This review is powered by Dreditor.
Comment #6
vivianspencer CreditAttribution: vivianspencer commentedThis patch worked great for me, thanks
Comment #7
Thalassa CreditAttribution: Thalassa commentedNot sure this is the right place to say this but for newbies trying to get their color module working...
after copying minnelli folder per instructions above, in color/color.inc I had to replace "../" with 5 sets of dots not four, that is
"../../../../../themes/garland/style.css"
I can only guess why, dots confuse me
Comment #8
tim.plunkettNeeds a better regex.
Comment #9
markhalliwellBased on that alone I'm marking this as a "feature" since it works as expected. I also agree with @tim.plunkett, this needs better regex and/or logic to actually make this work as expected.
Comment #10
markhalliwellsigh, forgot to tag
Comment #11
dlu CreditAttribution: dlu commentedRerolled patch to D8. Discussed ideas for a cleaner fix with markcarver and cottser, so far don't have any…
The issue with the regex is that the sub theme does not need to live in the same tree as the base theme and may in fact be a sub-sub-theme.
This patch includes the fix for #276384 (and would fail if it were committed).
Comment #12
markhalliwellThanks @dlu! Great patch :) Going to mark this as NR so it triggers the test bot. Also changing my mind about being a bug. Yes, from a "programmer's stance" this may seem like a feature request, but as a "themer" we tend to think/expect
../to work in paths on a daily basis. I'm up in the air either way. @webchick could you weigh in on this, thanks :) If it's a FR, we need to move this to 9.x.Comment #15
sanguis CreditAttribution: sanguis commented11: 466268.subtheme-inheritance.11.patch queued for re-testing.
Comment #17
sanguis CreditAttribution: sanguis commentedso I have gone away for this for a while and as I come back to this issue. and re read the description. I noticed that the section for testing/reporducing this issue uses minnilli and garland as an example. these themes dont exist in d8. so this decription need to be reworked.
Also the path at #11 needs to be re rolled
Comment #25
pameeela CreditAttribution: pameeela commentedAs part of the Bug smash initiative, we are triaging issues that are marked 'Postponed (maintainer needs more info)'. Based on the fact that the obsolete Garland theme is still listed in the steps to reproduce and no updates have been posted since this was raised, we believe this issue is no longer relevant so I am marking it 'Closed (outdated)'.
If anyone believes this issue should not be closed, please provide updated steps to reproduce when reopening it.