Since alpha one was released last week-ish, we need this on our radar.

However, a stable release in the 6.x-1.x branch is still waiting in a fix: #922136: Ensure features and hooks are properly updated for Panels 3.7, and I would really appreciate it, if we could just get that out of the way before moving on to 7.x where Panels in concerned.

CommentFileSizeAuthor
#10 skinr_947790-10.patch7.35 KBmoonray
#7 skinr_947790-7.patch7.24 KBmoonray
#6 skinr_947790-6.patch7.25 KBmoonray
#4 skinr_947790-4.patch7.24 KBmoonray
#1 skinr_947790.patch6.3 KBmoonray

Comments

moonray’s picture

moonray commented
Status: Active » Needs review
StatusFileSize
new skinr_947790.patch6.3 KB

And here's a patch to make panels work in D7.

jacine’s picture

jacine
she/her
Location New York
commented
Issue tags: +Needs tests

This needs tests.

jacine’s picture

jacine
she/her
Location New York
commented
Status: Needs review » Needs work

This does not contain all the corrections made here: http://drupal.org/cvs?commit=439848

moonray’s picture

moonray commented
Status: Needs work » Needs review
StatusFileSize
new skinr_947790-4.patch7.24 KB

Looks like I missed a spot. Here's an updated patch.
The update functionality (in skinr.install) should stay in D6; we are implementing hook_update_last_removed() to make sure all D6 updates are run before D7 is allowed to be updated after an upgrade.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work 
+++ modules/panels/skinr.inc
@@ -7,72 +7,42 @@
+dpm('theme_panels_skinr_style_render_region');
...
+  ¶

@@ -82,14 +52,39 @@ function theme_panels_skinr_style_render_panel($display, $panel_id, $panes, $set
+  if($skinr = skinr_get(NULL, $hook, $sid)) {
...
+  ¶

Various obvious issues here.

Powered by Dreditor.

moonray’s picture

moonray commented
Status: Needs work » Needs review
StatusFileSize
new skinr_947790-6.patch7.25 KB

Various issues pointed out by sun fixed (that'll teach me to post a patch at the end of a day).

moonray’s picture

moonray commented
StatusFileSize
new skinr_947790-7.patch7.24 KB

Let's try that again without whitespace.

jacine’s picture

jacine
she/her
Location New York
commented
Status: Needs review » Needs work

The regions don't render at all when styles are applied, so you get no content at all. Not sure why yet.

jacine’s picture

jacine
she/her
Location New York
commented

Ok, so disregard my previous comment. I was experiencing some odd behavior where panes worked and regions didn't, but it was resolved after a cache clear.

+++ modules/panels/skinr.inc
@@ -82,14 +51,39 @@ function theme_panels_skinr_style_render_panel($display, $panel_id, $panes, $set
+  // Add our contextual links here because panels does some crazy custom stuff.
+  $variables['content']->admin_links[] = array(
+    'title' => t('Edit skin'),
+    'href' => 'admin/appearance/skinr/edit/nojs/panels/' . $sid,
+    'query' => drupal_get_destination(),
+  );

This is totally weird. I wonder if this a potential security issue, or does contextual links make sure you have access to a path before returning the link?

Other than that, it looks good and works ;)

Powered by Dreditor.

moonray’s picture

moonray commented
Status: Needs work » Needs review
StatusFileSize
new skinr_947790-10.patch7.35 KB

The code in #9 is actually an exact duplicate of the way panels adds their contextual links. It doesn't actually use contextual links to render its panels links, though it does include the links from contextual.

From what I can tell there are no access checks being done for rendering the links. We need to add a wrapper around that piece of code to check for "access skinr" permission.

Find adjusted patch attached.

jacine’s picture

jacine
she/her
Location New York
commented
Status: Needs review » Fixed

Committed, thank you :) http://drupal.org/cvs?commit=443548

jacine’s picture

jacine
she/her
Location New York
commented
Status: Fixed » Needs work

This needs tests though.

moonray’s picture

moonray commented
Title: Port Panels functionality to 7.x » Write simpletests for panels plugin
moonray’s picture

moonray commented
Status: Needs work » Fixed

Since we're getting rid of third-party form integration for skinr config we won't be needing this anymore; all the panels code will be removed.
Closing.

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests

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

Issue tags: +Needs tests

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.