In this test case, the active admin theme is Blossom, a sub-theme of Seven.

The attached image shows that in seven_preprocess_html, the path_to_theme() function returns the path to Blossom's directory. This results in the two IE stylesheet files added through drupal_add_css() referring to non-existant files.

CommentFileSizeAuthor
#35 1125220-36-D7.patch3.1 KBroderik
#24 path_to_theme_tests-dev.patch4.11 KBjessebeach
#24 sub_themes.zip82.3 KBjessebeach
#14 drupal_get_path-1125220-14.patch2.2 KBjessebeach
#14 drupal_get_path-1125220-14-D7.patch3.58 KBjessebeach
#12 1125220-12.patch1.7 KBeffulgentsia
#12 1125220-12-D7.patch3.08 KBeffulgentsia
#6 drupal-core-1125220-1.patch3.1 KBjessebeach
#1 seven-1125220-1.patch1.42 KBjessebeach
path_to_theme.jpg102.03 KBjessebeach

Comments

jessebeach’s picture

jessebeach commented
StatusFileSize
new seven-1125220-1.patch1.42 KB

This patch changes path_to_theme to drupal_get_path('theme', 'seven'), which always returns the correct path to the Seven theme's directory, regardless of sub-theming.

David_Rothstein’s picture

David_Rothstein commented
Project: Seven » Drupal core
Version: 6.x-1.x-dev » 8.x-dev
Component: Code » theme system

Hm, this looks like a bigger issue (affects Bartik too, at a minimum)?

effulgentsia’s picture

effulgentsia commented
Issue tags: +Needs backport to D7

This looks good to me. Let's get bartik and garland into this patch as well. Note that for D8, this patch will be made obsolete by #522006: Conditional Styles in .info files, since drupal_add_css has it, but regardless of when that patch makes it into D8, we need this one in D7.

effulgentsia’s picture

effulgentsia commented
Title: Seven calls path_to_theme() in preprocess_html. In subthemes, this results in an incorrect path to a child theme's directory. » Core themes use incorrect path_to_theme() to refer to their own IE CSS files within hook_preprocess_html()
Status: Needs review » Needs work
jessebeach’s picture

jessebeach commented

Backported to a 7.x-dev issue as well.
http://drupal.org/node/1125624

jessebeach’s picture

jessebeach commented
StatusFileSize
new drupal-core-1125220-1.patch3.1 KB

Updated to include Seven, Bartik and Garland.

jessebeach’s picture

jessebeach commented
Issue tags: -Needs backport to D7

Removed issue tag that a backport to 7 is need. Here's the backport issue: http://drupal.org/node/1125624

jessebeach’s picture

jessebeach commented
Status: Needs work » Needs review

changed status to needs review.

seutje’s picture

seutje commented

hmz, might this be related to #621008: "theme path" is wrong for theme process and preprocess overrides ?

David_Rothstein’s picture

David_Rothstein commented
Issue tags: +Needs backport to D7

Patch looks good to me. We should keep this all in one place, though, so I'm going to close the other issue.

effulgentsia’s picture

effulgentsia commented

Looks good to me as well, but I'd like to see a review from a markup, bartik, garland, or seven maintainer before calling it RTBC.

Also, I asked for feedback on #621008: "theme path" is wrong for theme process and preprocess overrides for whether we can revert that issue, and if so, whether to add that reversion to this one.

effulgentsia’s picture

effulgentsia commented
StatusFileSize
new 1125220-12-D7.patch3.08 KB
new 1125220-12.patch1.7 KB

Reroll for HEAD.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Needs work
Issue tags: +Novice

Tagging as novice for the task of rerolling the Drupal 8.x patch on account of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

jessebeach’s picture

jessebeach commented
StatusFileSize
new drupal_get_path-1125220-14-D7.patch3.58 KB
new drupal_get_path-1125220-14.patch2.2 KB

Rerolled against 8.x and 7.x HEAD.

jessebeach’s picture

jessebeach commented
Status: Needs work » Needs review

Setting to need review.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -Novice

Thanks for the reroll.

Is there any way to add an automated test for this bug?

jessebeach’s picture

jessebeach commented

I can't come up with a good test case for this issue. It seems to me that the path_to_theme() and drupal_get_path() functions both work correctly.

For this issue, we simply have a case where drupal_get_path() should have been used in the first place, although it wasn't apparent until a sub-theme was introduced into the mix. I can't think of how to write a test case for using the wrong function.

xjm’s picture

xjm
she/her
Primary language English
commented

Well, the idea would be to create a test theme/subtheme that exposes the bug, and an automated test using that theme that fails without the patch and passes with it.

jessebeach’s picture

jessebeach commented

Do you mean just to illustrate the bug?

If the patch is applied to the existing core themes, then the fail state of such a test will never obtain unless someone changes the drupal_get_path function back to path_to_theme. This would be akin to changing any function in core to the wrong function.

Can you write this test that you have in mind? I really don't understand how one could write an automated test for using the wrong function. Sorry for being dense.

effulgentsia’s picture

effulgentsia commented
Status: Needs review » Needs work
Issue tags: +Needs tests

I think what xjm is suggesting is to create 2 themes in core/modules/simpletest/tests/themes, one that's a subtheme of Seven, and one that's a subtheme of Bartik. Do not add an ie.css file to them. Then write a test that requests a page using each of these themes, and assert that the HTML contains a reference to the base theme's ie.css file, and not to a non-existent ie.css file within the subtheme folder.

jessebeach’s picture

jessebeach commented

This doesn't need a test.

This is simply a matter of the wrong function being used. Requiring a test for this, which will not indicate anything more than drupal_get_path or path_to_theme, the functions, are broken (and I'm hoping these have their owns tests) is simply holding up a simple fix.

I'm not going to write a test because I don't think it adds any value. If I'm not understanding this and a test does add value, please someone who believes this to be the case write the test.

Until then, a bug lives in the base themes that will continue to vex anyone who creates a sub-theme of them.

effulgentsia’s picture

effulgentsia commented
Title: Core themes use incorrect path_to_theme() to refer to their own IE CSS files within hook_preprocess_html() » Subthemes of core themes trigger an undesired 404 and fail to inherit their parent theme's IE styling
Assigned: jessebeach » Unassigned

The reason a test is needed is there is a functional bug, and if a test isn't added, it might regress in the future. For example, some future contributor might look at these functions and think, "drupal_get_path() is so ugly, we should use path_to_theme() instead", and if that person submits a patch to do what they think is a minor code cleanup, we want the automated tests to spot the regression. Retitling the issue to reflect the functional bug being fixed.

But it's certainly not the obligation of the person who submitted a fix to also write the test. Thank you very much for the fix. Hopefully, someone will come along to write the test. I may do so if no one else does, and I can find a spare chunk of time. In the meantime, unassigning, so someone else who's inspired can claim it.

jessebeach’s picture

jessebeach commented

Ok, I wasn't understanding that the test is present to prevent changes by humans in the future, not to indicate that the drupal_get_path or path_to_theme functions have malfunctioned. That makes sense.

I want to see this patch get in. I'll try to write this test on Sunday at the Drupalcamp NJ sprint.

Thank you for explaining.

jessebeach’s picture

jessebeach commented
StatusFileSize
new sub_themes.zip82.3 KB
new path_to_theme_tests-dev.patch4.11 KB

Ok, I can reproduce the bug outside the tests, but when I run the test, the path to the the ie.css file is correctly printed as the path in the parent theme, not incorrectly printed as a path in the child theme.

The patch adds the test. I also attached two sub themes - one of Seven and one of Bartik.

STR

  1. Make either sub theme your admin theme.
  2. Navigate to an /admin page.
  3. You should see 404s in the console to non-existant ie files that are being referenced in the child theme, but exist in the parent theme.

Now run the Sub theme resource inclusion test. These 404'd files will be correctly referenced as located in the sub-theme's parent theme.

I'm looking in to why the test gives different results for the same code. Please feel free to bang your head on this as well.

jessebeach’s picture

jessebeach commented

Sorry, the patch in #24 is meaningless and I need to run for the moment. I'll post something meaningful shortly.

pwolanin’s picture

pwolanin commented

14: drupal_get_path-1125220-14.patch queued for re-testing.

pwolanin’s picture

pwolanin commented

I think this should go in without the need for a convoluted test. If we are worried about future reversion, we should add some code comments in the patch and also to path_to_theme().

The last submitted patch, 14: drupal_get_path-1125220-14.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Issue summary: View changes

Is it just me or is this bugfix not relevant to 8.x any longer? There are only two calls to path_to_theme() in core. It's very possible I'm completely misunderstanding the bug at hand, but maybe this is 7.x territory now?

hussainweb’s picture

hussainweb
Primary language English
commented

I don't think this is relevant to Drupal 8 anymore. There are no occurrences of path_to_theme at all (not even the function definition. See https://www.drupal.org/node/2324935. This could be about D7 only now, and if not even that, then it could be closed.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 8.0.x-dev » 7.x-dev
Issue tags: -Needs backport to D7

This is irrelevant for Drupal 8 now since we have now getActiveTheme() which works a little bit more the way its expected to.

stefan.r’s picture

stefan.r commented
Issue tags: -Needs tests +Novice, +Dublin2016, +Needs manual testing

Tagging this as novice for the tasks of manual testing and creating a reroll of the patch in #14

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs work » Needs review

Patch 14 is exactly the same as 12 except for some git-format-patch noise. So 12 should still apply; Trying to re-queue it.

The last submitted patch, 12: 1125220-12-D7.patch, failed testing. View results

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
StatusFileSize
new 1125220-36-D7.patch3.1 KB

Trying de-fuzzed one. (Minimal changes in context lines only.)

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.