The skinr_submit_handler() function in includes/handlers.inc has a logic error:

      if ((isset($theme['widgets']) && count($theme['widgets'])) || isset($theme['advanced']['_additional'])) {
        ...
        if (is_array($theme['widgets'])) {

Note that it continues working with $theme['widgets'] even if it doesn't exist.

CommentFileSizeAuthor
#14 submithandlers-906764-HEAD.patch3.83 KBvrajak@gmail.com
#13 submithandlers-906764-D61x.patch2.67 KBvrajak@gmail.com
#9 submithandlers-906764-1x.patch2.79 KBvrajak@gmail.com
#8 submithandlers-906764-8.patch3.87 KBvrajak@gmail.com
#5 906764-2.x.patch1.39 KBjacine
#1 skinr-n906764.patch1.38 KBdamienmckenna

Comments

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Active » Needs review
StatusFileSize
new skinr-n906764.patch1.38 KB

Here's a quick patch to improve the logic in skinr_submit_handler().

Patch sponsored by Bluespark Labs.

ericduran’s picture

ericduran commented
Status: Needs review » Needs work

@DemienMcKenna, I don't see the logic error. It seems like you're doing the same exact thing but in a different style.

I might be wrong since it's pretty late :-/ and my eyes are tired lol, but maybe some comments? or a little more explanation.

jacine’s picture

jacine
she/her
Location New York
commented

I'd like to either fix this or close it, but I don't know if there is a logic error or not.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs work » Needs review

There were two modifications made:

1:

      if ((isset($theme['widgets']) && count($theme['widgets'])) || isset($theme['advanced']['_additional'])) {

changes to:

      if ((!empty($theme['widgets']) && is_array($theme['widgets'])) || isset($theme['advanced']['_additional'])) {

This change had a flaw where the $theme['widgets'] variable could be something other than array. According to the PHP manual this is syntactically valid but is logistically unclear and allows for $theme['widgets'] to be something other than an array (which is the desired data structure).

2:

        if (is_array($theme['widgets'])) {

changed to:

        if (!empty($theme['widgets']) && is_array($theme['widgets'])) {

If $theme['widgets'] does not exist the is_array() call will give an E_NOTICE. The first if statement (#1 above) allows for this block to be executed when $theme['widgets'] does not exist, so it is necessary to check if it exists first before working with it.

jacine’s picture

jacine
she/her
Location New York
commented
Version: 6.x-1.5 » 6.x-2.x-dev
StatusFileSize
new 906764-2.x.patch1.39 KB

@Damien that you so much for explaining that. I committed your patch, and it will be in the next release: http://drupal.org/cvs?commit=439792 :)

This is also a problem in the 2.x branches. Here's a patch for that.

vrajak@gmail.com’s picture

vrajak@gmail.com commented

I applied the patch locally, patches fine and the new code shows up in the skinr.handlers.inc file. I don't know if I need to test anything else with it.

*edit* will do some more testing as discussed in IRC :)

jacine’s picture

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

Actually, this needs work.

That logic exists in other submit hander functions, like for panels, etc. This needs to be applied to those as well.

vrajak@gmail.com’s picture

vrajak@gmail.com commented
Status: Needs work » Needs review
StatusFileSize
new submithandlers-906764-8.patch3.87 KB

I'm attaching a patch, which incorporates DamienMcKenna's changes in comment #4 to the panels.skinr.inc and views.skinr.inc files. Actually it also includes the changes from Jacine's patch above. I've tested it locally with and without skins and am no longer receiving the widget related errors when editing a skinr settings form. Would be great if someone else could review it as well just to make sure its working.

*edit* is for the D6 2x branch

vrajak@gmail.com’s picture

vrajak@gmail.com commented
StatusFileSize
new submithandlers-906764-1x.patch2.79 KB

Here's the same patch for the D6 1x Branch. Works for me but needs review.

Status: Needs review » Needs work

The last submitted patch, submithandlers-906764-1x.patch, failed testing.

vrajak@gmail.com’s picture

vrajak@gmail.com commented
Status: Needs work » Needs review

#9: submithandlers-906764-1x.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, submithandlers-906764-1x.patch, failed testing.

vrajak@gmail.com’s picture

vrajak@gmail.com commented
Version: 6.x-2.x-dev » 6.x-1.x-dev
StatusFileSize
new submithandlers-906764-D61x.patch2.67 KB

Here's another try at the D6 1x branch patch. It works for me (actually the last patch failed when I tested it, doh. This one I made sure its working.) If someone else could give it a try that would be great. It just changes the two lines of code in panels.skinr.inc and views.skinr.inc, handlers.inc already had the right code so I didn't have to change it.

vrajak@gmail.com’s picture

vrajak@gmail.com commented
Version: 6.x-1.x-dev » 6.x-2.x-dev
Status: Needs work » Needs review
StatusFileSize
new submithandlers-906764-HEAD.patch3.83 KB

Trying out a patch for the 7x HEAD version, same code changes as above.

Status: Needs review » Needs work

The last submitted patch, submithandlers-906764-HEAD.patch, failed testing.

moonray’s picture

moonray commented
Version: 6.x-2.x-dev » 6.x-1.x-dev

Let's make this patch apply to 6.x-1.x (and ignore all others for now), since that version needs a new release ASAP.

moonray’s picture

moonray commented
Status: Needs work » Needs review

#13: submithandlers-906764-D61x.patch queued for re-testing.

vrajak@gmail.com’s picture

vrajak@gmail.com commented
Version: 6.x-1.x-dev » 7.x-2.x-dev
vrajak@gmail.com’s picture

vrajak@gmail.com commented

#14: submithandlers-906764-HEAD.patch queued for re-testing.

vrajak@gmail.com’s picture

vrajak@gmail.com commented
Version: 7.x-2.x-dev » 6.x-1.x-dev
jacine’s picture

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

Committed! Thanks all ;)

6.x-1.x: http://drupal.org/cvs?commit=448998
6.x-2.x: http://drupal.org/cvs?commit=448992
HEAD: http://drupal.org/cvs?commit=449000

Status: Fixed » Closed (fixed)

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