The panels_renderer_standard::add_meta() method uses the path_to_theme() function to check if the active theme defines a CSS file for the layout to render. This allows themes to use their own CSS for a given layout instead of the one that comes with Panels module.

But when the layout render operation occurs while a theme process is happening, the path_to_theme() function may return a wrong path if the theme hook defines a value for the "theme path" key (@see hook_theme API).

In such edge cases, the theme CSS if not used as expected.

Steps to reproduce this bug:

  1. enable the Panels and Views modules,
  2. enable a theme that has a CSS file called "twocol.css" in its root folder (you may add a dummy rule like body { background: red; },
  3. create a new view that uses panels fields (use the simple twocols layout),
  4. add a page display,
  5. go to the newly created page.

What we don't see:
You will not see the expected red background, because the path_to_theme() function returns sites/all/modules/views instead of the active theme path.

What we should see:
The red background.

CommentFileSizeAuthor
#12 panels-n2056701-12.patch1.7 KBDamienMcKenna
#8 panels-n2056701-8.patch1.74 KBDamienMcKenna
#2 panels-layout-renderer-should-not-rely-on-path-to-theme-2056701-3.patch1.35 KBPeacog
#1 panels-layout-renderer-should-not-rely-on-path-to-theme-2056701-1.patch1.27 KBB-Prod
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

B-Prod’s picture

B-Prod CreditAttribution: B-Prod commented
Status: Active » Needs review
FileSize
panels-layout-renderer-should-not-rely-on-path-to-theme-2056701-1.patch1.27 KB

The patch below fixes this.

B-Prod’s picture

B-Prod CreditAttribution: B-Prod commented
Issue summary: View changes

Fix a typo

Peacog’s picture

Peacog CreditAttribution: Peacog commented
Issue summary: View changes
FileSize
panels-layout-renderer-should-not-rely-on-path-to-theme-2056701-3.patch1.35 KB

Thanks for this patch. The use of path_to_theme() was throwing open_basedir restriction in effect warnings for me. path_to_theme() would return "modules/system" and the resulting css path, which was of the form "modules/system/../../../css/layouts/my-panels-layout/my-panels-layout.layout.css" was triggering the open_basedir warning.

I've rerolled the patch for the latest 7.x-3.x-dev

robmcbryde queued 2: panels-layout-renderer-should-not-rely-on-path-to-theme-2056701-3.patch for re-testing.

B-Prod’s picture

B-Prod CreditAttribution: B-Prod commented
Status: Needs review » Reviewed & tested by the community

Could this be committed?

The last submitted patch, 1: panels-layout-renderer-should-not-rely-on-path-to-theme-2056701-1.patch, failed testing.

mrjmd’s picture

mrjmd CreditAttribution: mrjmd commented
Parent issue: » #2401913: META: Plan for Panels 7.x-3.5 release
japerry’s picture

japerry
Location KVUO
CreditAttribution: japerry commented
Priority: Major » Normal
Status: Reviewed & tested by the community » Needs work

Somethings not right here... 

+++ b/plugins/display_renderers/panels_renderer_standard.class.php
@@ -407,14 +407,20 @@ class panels_renderer_standard {
       foreach($css as $file) {
...
+        // function when the theme hook defines the key 'theme path'.
+        $theme_path = isset($theme) ? drupal_get_path('theme', $theme) : '';
...
+          $this->add_css($theme_path . '/' . $this->plugins['layout']['css']);
         }

Why are we calling this within the foreach? This could potentially add the same file over and over again.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Status: Needs work » Needs review
FileSize
panels-n2056701-8.patch1.74 KB

I've moved the $theme_path definition outside of the loop, and fixed formatting of the two foreach() loops in that method.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Issue tags: +SprintWeekend2015
mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman commented
Status: Needs review » Reviewed & tested by the community

Applies, works as expected. I like that we can bypass straight to the theme (even discussed as first comment on path_to_theme() documentation.)

japerry’s picture

japerry
Location KVUO
CreditAttribution: japerry commented
Status: Reviewed & tested by the community » Needs work

I'm still a bit worried about this one.

+          $this->add_css($theme_path . '/' . $this->plugins['layout']['css']);

This is being looped over and over again as well. If I'm following the code correctly, I believe the if statement is trying to make sure that if there is a theme implementation of a css file, use it. otherwise fall back. But I don't think the if statement actually does it. Haven't given enough time to test and try a patch on it, but this code looks suspect to me still.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
FileSize
panels-n2056701-12.patch1.7 KB

Ah, one of the variables needed to be updated. How's about this?

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman commented
Status: Needs work » Needs review

Marking for review for updated patch

japerry’s picture

japerry
Location KVUO
CreditAttribution: japerry commented
Status: Needs review » Fixed

yah, that looks better! committed.

  • japerry committed 2fea256 on 7.x-3.x 
    Issue #2056701 by DamienMcKenna, B-Prod, Peacog: Panels layout renderer...

Status: Fixed » Closed (fixed)

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