I am testing things.. Can someone try to duplicate this...
D614, wamp, fresh test install, everything is set and working as expected.
Steps:
set upload limits and directory
intentionally try to upload more than the limit
system error, inform me that that I can only upload x number of images.
navigate away from upload page, clear browser cache and cookies
try to upload x number of images under my limit
it seems the system thinks i have the failed uploads in the system
login as admin, delete all galleries, clear cache.... try again

the system still thinks i have these images, but they are not in the files directory

The selected file 013.jpg could not be uploaded. You can only upload a maximum of 6 images.

Ideas, anyone

CommentFileSizeAuthor
#34 601186-2.patch2.67 KBjustintime
#26 601186-1.patch1.46 KBjustintime
#20 601186.patch844 bytesjustintime

Comments

dbeall’s picture

dbeall commented

the error was as an anonymous user..

dbeall’s picture

dbeall commented
Title: false error after going over image number upload limit » Image directorys and uploaded image information not being flushed on failed uploads &/or delete

Change title to better reflect issue. I hope that says it better.

I have reproduced this error as anonymous and authenticated user.

The images are remaining in the files directory after delete gallery and/or delete image(s) from the manage images page.
If you delete the gallery node from the Drupal content list, the gallery and image nodes are gone from the list, but the not the directories.
The Directories remain. sites/default/files/%gid/%galleryname

The Drupal content list does not show the image nodes, and if you delete a gallery, that node is gone too.

When going back to the gallery landing page:
The edit control links are there, but the manage images is not and the notice (There are no images in this gallery, click here to upload some images!). Try to upload images, warning, you can only upload x number of images.

Anyway:
The database(or where gallery gets the uploaded image information for the user) must be telling the gallery node that the images are there, even though the upload failed or the gallery was deleted from the content list.

dbeall’s picture

dbeall commented

This is issue not the same, but related to or similar to, #486750: File folder left behind upon gallery delete

dbeall’s picture

dbeall commented
Priority: Minor » Critical

Change status::I don't like critical issues, but this seems to be one of them.

This setup was on xamp, the last 2 tests were on wamp

I have duplicated the over image number failure issue
try to upload to many images.. upload fail, receive warning
log out, clear browser cache, login, try to upload within limit, upload fail receive warning

images are still in the file system even though the upload was failed.
Login as admin check node list(content list) the image nodes were not created.

dbeall’s picture

dbeall commented

Just getting going today..Happy birthday to me! A day off.
I tested this with the Oct 28th dev.
Posting this to bump it. Still an issue.
I think this is a release blocker.?

kmonty’s picture

kmonty
He/Him
Location Berlin, DE + SF, CA
commented

Yeah, it is a release blocker. I have not figured out how I want to handle this one.

justintime’s picture

justintime commented

So, I took some time looking at this tonight.

I committed a fix for the empty directory issue.

However, I'm about 95% sure that the directory being left over isn't the cause of all of dbeall's headaches. I'm trying to track it further, but there are cases where a node is being deleted, but its entry in the files table is not removed. There's also a huge oversight in the quota calculation feature. I'll file a separate issue for this.

justintime’s picture

justintime commented
Assigned: Unassigned » justintime
Status: Active » Fixed
dbeall’s picture

dbeall commented

Thank you Justintime.. You have made huge contributions to node_gallery and the Drupal community.

Just for the record, I(personally) am used to using ftp to clean up Drupal files. lol, I do a lot of it.

That said, this fix has things working better.. !
the gallery folder is flushed and the image in the gallery image folder is flushed as well.. This is a big improvement.
I am using ( files/user_name/gallery_name ) for my gallery path in node gallery configuration.

I am wondering if the code can be amended to hunt down any image(same file name) and/or folder(same folder name) for deletion inside the imagecache directory.
WHY.? this would then flush the ImageCache files that correspond to the main image file and gallery folder
( files/imagecache/[imagecache-preset-name]/[user_name -or- gallery_token]/[gallery_name -or- gallery_token]/[image-file] )

** This maybe should be a feature request to imagecache to allow directory path settings per preset, which would make this file and folder clean up easier and make our files directory much cleaner overall.

note: I am still confused with php or I would be submitting patches rather than dummy text.

justintime’s picture

justintime commented

Sweet, a one-line feature implementation ;-)

ImageCache has a function to handle cleanup, I just committed code that calls their function upon deletion of an image. Note that their function doesn't clean up empty dirs, but that's an imagecache issue. You can file a feature request with them if you like.

Also, to keep things separate, I created a new issue for the imagecache cleanup: #659624: Node Gallery image deletion should clean up the imagecache copies of itself

justintime’s picture

justintime commented
Status: Fixed » Active

Setting this bug to active, as the failed upload scenario is still broken. I'm marking #486750: File folder left behind upon gallery delete as fixed, and will continue working on this one.

justintime’s picture

justintime commented
Status: Active » Postponed (maintainer needs more info)

I remember reading that when you upload a file into Drupal, it's initially marked as "temporary". Only after the module developer makes a certain call does it become "permanent". However, temporary files are not cleaned up immediately, they're done via cron.

So, I'm wondering if you trigger the issue described above, and then run cron manually, is it resolved?

dbeall’s picture

dbeall commented
Status: Postponed (maintainer needs more info) » Active

I just did a new test with D6 with all updates, using node_gallery_alpha12..

If you try to upload more photos than your limit. You get red warning*over limit*, *failed upload*
With Limit set at 3, try to upload 4 images,, get warning,failed upload.....
ok
so, I will try one image because I have a 3 image limit. The warning comes up because I am over limit..
the module is thinking my 4 uploads were successful.

the files don't seem to be still in file system,, still looking at this. more to come

dbeall’s picture

dbeall commented

part of this was fixed with the imagecache clear files on delete function

dbeall’s picture

dbeall commented
Title: Image directorys and uploaded image information not being flushed on failed uploads &/or delete » Image information not being flushed on failed uploads

OK, it looks like that's the only thing still at issue..

Will redo tests with node_gallery_dev and report any thing different

the files are being removed on gallery delete..
the files don't seem to be in any tmp directory that I can see..
When an upload fails, the files never get to the upload(transfer) stage..

dbeall’s picture

dbeall commented
Title: Image information not being flushed on failed uploads » Image total count not being flushed on failed uploads

maybe that is better...

justintime’s picture

justintime commented

Can you hit cron.php and see if that lets you upload?

dbeall’s picture

dbeall commented

I tried it, still won't allow any more uploads.
it failed 4 images as I was over limit
run cron..
try upload 1 image, get warning*You can only upload a maximum of 3 images.*

justintime’s picture

justintime commented

Darn. Let me see if I can track it down then.

justintime’s picture

justintime commented
Status: Active » Needs review
StatusFileSize
new 601186.patch844 bytes

Well, that was a rare bug where the fix was easier than the test :)

Attached patch should fix the issue.

FWIW, the cron approach would eventually work. There's a constant in system.module: define('DRUPAL_MAXIMUM_TEMP_FILE_AGE', 21600) - once 21,600 seconds had passed, and cron was run, it would have worked. The attached patch should disregard failed uploads when determining the current count.

dbeall’s picture

dbeall commented

well, I ran cron, flush cache, clear browser. Tried in alpha and dev.. doesn't change behavior, unless I need to uninstall, re-install

justintime’s picture

justintime commented

If you don't apply the patch, you have to wait for 6 hours before cron will remove the temporary files from the db. If you apply the patch, then it should work immediately, because the failed files won't be included in the count. Did you apply the patch?

dbeall’s picture

dbeall commented

yes, the patch was added. tried dev and alpha on my wamp install(s)

justintime’s picture

justintime commented

Oh, yuck.

The patch fixes part of the problem. However, there's more badness here. If you have a file limit set to 3, and you attempt to upload 4, the first three are processed successfully and the last one fails. However, the code at line 148 on node_gallery.pages.inc says "Stop everything right now if anything didn't work right". The problem is that Drupal says you have 3 images that were successful, but we didn't do anything with them -- they're essentially ghosts in the system at that point.

I think I can still fix this, but your install is mucked up. You either will need to wipe your install and start over, or go through your files table and delete any rows that don't have files on your filesystem.

Give me a few minutes to come up with a patch.

dbeall’s picture

dbeall commented

I went into the database and deleted the old file references..
It works now but it allows any number, not respecting file number limit

justintime’s picture

justintime commented
StatusFileSize
new 601186-1.patch1.46 KB

Sorry Dave - I flipped a 1 to a 0 in that first patch. This one should hopefully work.

dbeall’s picture

dbeall commented

I think that's working..
I will dump everything and run it again

dbeall’s picture

dbeall commented

nope.. something is acting funny
logged in as authenticated user
on first upload, no limit is recognized
after that operation is done,, then the limit is recognized.

or/so
i go back and delete all but 1 image..
try to upload 1 more(so I would have 2 images and have a limit of 3) *warning, you can only upload 3 images*

might need more play time with it..

kmonty’s picture

kmonty
He/Him
Location Berlin, DE + SF, CA
commented

This patch is good but instead of being status = 1 it should be:
$current_number = db_result(db_query("SELECT COUNT(*) FROM {files} WHERE uid = %d and status = %d", $user->uid, FILE_STATUS_PERMANENT));

justintime’s picture

justintime commented

Good point, I've put that in locally so when it gets committed, it will use the constant.

dbeall’s picture

dbeall commented

I think that did the trick.. seems to be working
alpha12

using patch from #26 plus change from #29

dbeall’s picture

dbeall commented

nope,, something funny going on.

first upload no limits recognozed
delete files to be under limit,
upload more files within limit, act as uploading, are listed in files table, but not showing in gallery..

why do I think this is a one of those "can of worms" thingys
better leave this open for a bit

justintime’s picture

justintime commented

I know where the problem is. Once I get the kids to bed it will take ten minutes to code the fix. Yes, I'm a little OCD; -)

justintime’s picture

justintime commented
StatusFileSize
new 601186-2.patch2.67 KB

Okay, I haven't tested it because my 2.x install is kinda jacked up, but I think this will take care of things. Let me know.

dbeall’s picture

dbeall commented

#34 looks(works) better, acting as expected, seems solid, so far so good....
tested with 6.x-2.x-dev

will test in alpha12 and report any trouble..

dbeall’s picture

dbeall commented

OK, tested with alpha12,, it seems to be solid so far..

justintime’s picture

justintime commented
Status: Needs review » Fixed

committed fix to 2.x.

Status: Fixed » Closed (fixed)

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