Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#1 | 1164014-undefined-variables.patch | 4.95 KB | scroogie |
Comments
Comment #1
scroogie CreditAttribution: scroogie commentedNot 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
Comment #2
scroogie CreditAttribution: scroogie commented...
Comment #3
justintime CreditAttribution: justintime commentedI 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!
Comment #4
scroogie CreditAttribution: scroogie commentedI was the one who created the patch, so it has my blessing. :P
Comment #5
justintime CreditAttribution: justintime commentedDoh, 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.
Comment #6
scroogie CreditAttribution: scroogie commentedAre 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.
Comment #7
justintime CreditAttribution: justintime commentedLooks 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.
Comment #8
scroogie CreditAttribution: scroogie commentedI'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()?
Comment #9
justintime CreditAttribution: justintime commentedSorry, thanks for setting me straight. Somehow I was looking at a different function. Committed in your version, using $gid.