One of the editors I use (jEdit) has a nice PhP plug-in that parses scripts and points out errors, including use of variables that have not been defined.

Since I found some undefined vars in the install code, I decided to look at all the main node_gallery files in this and and put together this below. Haven't look at the logic around these to form an opinion of how to fix them, but I have verified that they are indeed undefined.

Hopefully, correcting these may help prevent/solve some problems.

Version: 6.x-3.x-dev ( AM Fri May 20 with realname patch)

File: node_gallery.inc

  Function: node_gallery_get_image_nids

    Line: 289 $reset undefined

  Function: node_gallery_get_images

    Line: 379 $reset undefined

  Function: node_gallery_delete_image

    Line: 892 $image undefined

File: node_gallery.pages.inc

  Function: node_gallery_manage_images_form

    Line: 236 $file undefined

  Function: node_gallery_content_add_more_js

    Line 645: $group_name undefined

  Function: node_gallery_sort_images_form

    Line 765: $title undefined
CommentFileSizeAuthor
#1 1164014-undefined-variables.patch4.95 KBscroogie
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

scroogie’s picture

Not bad. One of them is actually influencing functionality (the one in node_gallery_delete_image).

Here is a patch. Justin, the caching is your baby, so please have a glance at the $reset changes. :)

Greetings

scroogie’s picture

Status: Active » Needs review

...

justintime’s picture

Status: Needs review » Active

I fixed and committed the ones in node_gallery.inc -- they were pretty straightforward to me. The one from node_gallery_manage_images_form() is a leftover from the port from 2.x to 3.x, and I *think* we can just set that to FALSE, but I'm not for sure.

The other two, I'd love it if @scroogie could comment as he's the "owner" of the sort functionality. If he doesn't give his 2 cents, I'll fix those next week.

Thanks!

scroogie’s picture

I was the one who created the patch, so it has my blessing. :P

justintime’s picture

Status: Active » Fixed

Doh, crosspost!

@scroogie: I guess great minds think alike - we agreed on almost everything, except that you changed a few of the calls to set $reset to TRUE. Like you, I modified the function signature, but I didn't change the calls. Since we don't have any caching bugs (yet), I think we're safe to not reset the cache there. I did commit in your fixes in pages.inc.

@cgmonroe: That should wrap all of these up.

scroogie’s picture

Status: Fixed » Needs work

Are you sure that we can rely on $node->oldgid? I thought it's only there if "Enable changing an image's gallery on the Manage Images page" is activated. I think we can just use $gid declared earlier.

Looks good otherwise.

justintime’s picture

Status: Needs work » Fixed

Looks like it's status tug-o-war :)

On line 786, we check to make sure the oldgid attribute does indeed exist on the node object. This code executes if you move an image that was the cover of gallery A to gallery B. It can't be the cover for gallery A because it doesn't reside within it anymore, so we set it to NULL.

scroogie’s picture

I'm a bit lost. The code is in node_gallery_delete_image(), right? It's executed when an image is deleted. If that image was a cover, we choose a different cover. If there are no images left, we set it to NULL. I don't see where oldgid is set in that flow.

Perhaps you're confusing the appearance with the one in node_gallery_set_gallery_cover_image()?

justintime’s picture

Sorry, thanks for setting me straight. Somehow I was looking at a different function. Committed in your version, using $gid.

Status: Fixed » Closed (fixed)

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