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.Updated: Comment #15
Problem/Motivation:
When creating an identical copy of garland and minnelli for customization I found that I could not use the color module support to change colors. The images were generated but the paths in the CSS were wrong. I added some error_log debugging and discovered that the regular expression that is being used to determine image path for replacement (coalescing ..) for will not match a 2-character subtheme name. The paths are then not replaced, leading to invalid image references in the css.
Proposed resolution:
Patch at Comment #15 . This patch changes regular expression to take 2-character subtheme name.
Remaining tasks:
Testing patch is required if the regular expression is correct and also issue at https://drupal.org/node/466268 conflicts the code with the patch #15
User interface changes
n/a
API changes
n/a
Related Issues
n/a
Original report by @ahamid
When creating an identical copy of garland and minnelli for customization I found that I could not use the color module support to change colors. The images were generated but the paths in the CSS were wrong. I added some error_log debugging and discovered that the regular expression that is being used to determine image path for replacement (coalescing ..) for will not match a 2-character subtheme name. The paths are then not replaced, leading to invalid image references in the css.
The PCRE regex in my color.module is: `(^|/)(?!../)([^/]+)/../`
Here is some debug illustrating the problem.
First, Minnelli, which works:
[12:44:22.760] {http--8686-10} base_path(): /
[12:44:22.760] {http--8686-10} before: ../images/bg-navigation.png
[12:44:22.760] {http--8686-10} after: bg-navigation.png
[12:44:22.760] {http--8686-10} before: /themes/garland/minnelli/../images/bg-navigation.png
[12:44:22.761] {http--8686-10} Before after regex: /themes/garland/images/bg-navigation.png
[12:44:22.761] {http--8686-10} replacing '/themes/garland/images/bg-navigation.png' with 'bg-navigation.png in css
Now, my custom subtheme ('ww'):
[12:42:06.858] {http--8686-1} base_path(): /
[12:42:06.858] {http--8686-1} before: ../images/bg-navigation.png
[12:42:06.858] {http--8686-1} after: bg-navigation.png
[12:42:06.859] {http--8686-1} before: /sites/all/themes/custom-garland/ww/../images/bg-navigation.png
[12:42:06.859] {http--8686-1} Before after regex: /sites/all/themes/../images/bg-navigation.png
[12:42:06.859] {http--8686-1} replacing '/sites/all/themes/../images/bg-navigation.png' with 'bg-navigation.png in css
Notice that the 'before' path after the regex is wrong.
I verified this behavior via an online PCRE regex checker:
http://www.quanetic.com/regex.php
The Minnelli path yields:
Here's where the pattern was found:
/themes/garland[!MATCH!]images/bg-navigation.png
Parenthesized substring matches:
* substring 1: /
* substring 2: minnelli
My custom subtheme yields:
Here's where the pattern was found:
/sites/all/themes[!MATCH!]../images/bg-navigation.png
Parenthesized substring matches:
* substring 1: /
* substring 2: custom-garland
It is replacing the parent theme, not the subtheme. Interestingly, if the subtheme name is changed to 1 character, or more than 2 characters, the matching works.
I have not yet tried to fix the regular expression but may followup with a fix if I can.
| Comment | File | Size | Author |
|---|
Issue fork drupal-276384
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #1
ahamid CreditAttribution: ahamid commentedOk, the fix for the regex is to *escape* the dots in ../ :
`(^|/)(?!\.\./)([^/]+)/\.\./`
Since "dot" means "Match any single character" I suspect that this was leading the expression to be broken for JUST themes that were two characters long. Lucky me to stumble upon that ;)
This fix must be applied in modules/color.module as well as includes/common.inc _drupal_build_css_path.
Let me know if you need an official patch/diff for this.
Comment #2
Thalassa CreditAttribution: Thalassa commentedJust wanted to say thank you for posting this. Was banging my head for 24 hrs with my two character subtheme name.
Comment #3
TheComrade CreditAttribution: TheComrade commentedThis wacky module also fails in the same way if there are two character paths involved. e.g. if drupal is installed at http://site.com/util/cs/drupal.
Comment #4
torotil CreditAttribution: torotil commentedI was just hitting this very same issue in drupal 7.2. Why is the fix still not applied?
Comment #5
torotil CreditAttribution: torotil commentedComment #6
iSampo CreditAttribution: iSampo commentedPatch for escaping all of the dots.
Comment #7
torotil CreditAttribution: torotil commentedRTBC.
Comment #8
jpstrat CreditAttribution: jpstrat commentedI tested this patch with Drupal 7.15 (renamed to drupal_views) and found the problem still exists.
I started by examing the patch itself vs drupal-7.15/modules/color where I found color.module. I opened the module and found that the patch in item 6 above has been included in the Drupal-7.15 distribrution.
I then copied the Bartik theme from /drupal-views/themes to /drupal-views/sites/all/themes and renamed the directory xy from Bartik. Inside the Bartik directory I renamed the Bartik.info file xy.info.
I then went to the Appearance tab to look for my new xy theme. I didn't find it, but I did find a second copy of Bartik. I clicked the "enable and set default" button for the new Bartik then went to the home page. It looked just like a normal Bartik home page. I clicked Appearance and the settings for my new theme. I then manipulation the color wheel to show very different colors. The preview showed that the colors were changed. I clicked "Save Configuration" and went to the home page. The colors remained the basic Bartik blue.
I re-enabled the stock Bartik theme. I then changed my xy directory to xyz and my xy.info to xyz.info. I then enabled the xyz theme. I found the changes I had earlier saved were implemented without me making any further changes.
Additionally, I received the following errors with the xy naming:
"Notice: Undefined variable:hide_site_name in include() (line 99 of C:\sites\Uniserver\www\drupal-views\sites\all\themes\xy\templates\page.tpl.php)."
"Notice: Undefined variable:hide_site_name in include() (line 109 of C:\sites\Uniserver\www\drupal-views\sites\all\themes\xy\templates\page.tpl.php)."
Comment #9
iSampo CreditAttribution: iSampo commented@jpstrat:
- Just checked out 7.15 and the patch has not been applied there.
- Your xy theme on the Appearance page was named Bartik because you didn't change the theme 'name' value on the .info file.
- Your notices came from your copy of template.php. Rename all bartik_ functions (such as bartik_process_html, bartik_process_page, etc.) to YOURTHEMENAME_process_html, etc. That way you'll get rid of the notices.
Tested on these steps (+ applied the patch + cleared Drupal cache), and it works.
Comment #10
webchickAwesome, thanks for the fix, folks!
Two things:
1) We actually need to apply this to 8.x first (http://drupal.org/node/767608), so would someone mind re-rolling for that?
2) As long as you have your head in the regex and understand it, any way to add a one or two-liner comment above it to explain what the heck it's doing for the next person? :)
We normally require tests for bug fixes, but since color module doesn't currently have any tests, I don't think that should hold this patch up, since we're unlikely to touch this regex again once we've fixed it. I've created #1805118: Add tests for Color module as a follow-up, though.
Comment #11
steinmb CreditAttribution: steinmb commentedNeed an issue summary
Comment #12
markhalliwell#6 needs a reroll against D8. Let's commit to D8 first. Then verify patch in #6 is still viable for D7, if not reroll again.
Comment #13
dlu CreditAttribution: dlu commented#6: color_regex_fix-276384.patch queued for re-testing.
Comment #15
dlu CreditAttribution: dlu commentedRerolled to D8. Patch in #6 is being retested, so that may do the job of a backport to D7.
Comment #16
dlu CreditAttribution: dlu commentedThis is a reroll of the patch in #15 to D7.
Once #15 is committed to D8, this could be uploaded as a patch and the version changed to D7.
Comment #17
markhalliwellGreat patch @dlu! Thanks! It passes with flying colors, heh :)
I'm marking this as NW and adding tag though since we really should be testing that this regex works as expected (per #10 and core gates).
Comment #18
markhalliwelltagging for #16
Comment #19
dlu CreditAttribution: dlu commentedThis patch and https://drupal.org/node/466268 affect the same line of code. https://drupal.org/node/466268 includes the changes in this patch.
Comment #19.0
aparnakondala1 CreditAttribution: aparnakondala1 commentedUpdated issue summary.
Comment #19.1
aparnakondala1 CreditAttribution: aparnakondala1 commentedUpdated issue summary.
Comment #19.2
aparnakondala1 CreditAttribution: aparnakondala1 commentedUpdated issue summary.
Comment #19.3
aparnakondala1 CreditAttribution: aparnakondala1 commentedUpdated issue summary.
Comment #19.4
aparnakondala1 CreditAttribution: aparnakondala1 commentedUpdated issue summary.
Comment #19.5
aparnakondala1 CreditAttribution: aparnakondala1 commentedUpdated issue summary.
Comment #19.6
aparnakondala1 CreditAttribution: aparnakondala1 commentedUpdated issue summary.
Comment #20
sanguis CreditAttribution: sanguis commentedstarting on the tests
Comment #21
mgiffordThis still a concern in D8? Unassigned issue too.
Comment #30
quietone CreditAttribution: quietone as a volunteer commentedStill an issue, The regex has not changed in 9.2.x
Comment #32
SpokjeComment #33
SpokjeIssue Summary was updated after the tag was given (#11) in #19.1 up to #19.6 and is pretty clear to me.
Removing tag
Comment #35
SpokjeStill needs tests as per #17.
Comment #39
quietone CreditAttribution: quietone at PreviousNext commentedColor has been removed from core, #3270899: Remove Color module from core.