Hi, Jenny... I've added some feedback if a non-image file upload is attempted.
Thanks for the feedback! Rick -----Original Message----- From: Jenny Gavin-Wear [mailto:[email protected]] Sent: Monday, August 22, 2011 1:04 AM To: cf-talk Subject: RE: Anyone care to review this image handling process? Hi Rick, The only thing I couldn't see was feedback to the user if there is any problem with the image upload. ie, non-image file type, corrupt upload, etc Jenny Gavin-Wear Fast Track Online Tel: 01262 602013 http://www.fasttrackonline.co.uk/ >>-----Original Message----- >>From: Rick Faircloth [mailto:[email protected]] >>Sent: 22 August 2011 03:50 >>To: cf-talk >>Subject: RE: Anyone care to review this image handling process? >> >> >> >>Hi, Kym, and thanks for the feedback. >> >>I will implement the suggestion to test "isImage". That's >>good advice. >> >>Also, I like using a lot of variables, also. It makes my >>code easier to read and remember what it's doing over time. >>It could be more concise with fewer variables, but ultimately >>that would make it harder to work with for me. >> >>Thanks, again! >> >>Rick >> >>-----Original Message----- >>From: Kym Kovan [mailto:[email protected]] >>Sent: Sunday, August 21, 2011 9:35 PM >>To: cf-talk >>Subject: Re: Anyone care to review this image handling process? >> >> >>Hi Rick, >> >>not knowing what is in the rest of the CFC have you considered a >>malicious person uploading something other than an image beyond doing >>the upload to an web-unreachable temp folder? >> >>It might be worthwhile to either test the image for goodness before you >>copy it and/or put a try/catch round the image resize code so you pick >>up the bad image and can give the user some feedback or log the user as >>a nasty. >> >><cfimage action="read" name="theImageObject"... >><cfif isImage(theImageObject)> >> <cfset ImageResize("#theImageObject#",... >> <cfimage source="#theImageObject#" action="write"... >> ... >><cfelse> >> bad person code >></cfif> >> >>Regarding point 6 I like lots of variables if it makes the code easier >>to maintain rather than having one which changes what it is keeping >>depending on where you are. And verbose names as well. >> >> >>Kym K >> >> >>On 22/08/2011 10:44, Rick Faircloth wrote: >>> >>> Thanks, Peter! >>> >>> 1) Yes, code is indeed inside a larger function "add staff", etc. >>> >>> 2) Good catch! Duh! Now the counter increments as it loops. >>> >>> 3) I debugged my brain about the hashes. :o) >>> >>> 4) e:\tempImages now has a delete cffile.serverFile after processing is >>> complete >>> >>> 5) I usually do this... just got lazy. Now e:\tempImages is >>> application.tempImagesDirectory >>> >>> 6) I'll have to work on this. Thanks for the suggestion! >>> >>> 7) Same as No. 6... I assume you're talking about "extending" methods >>within >>> a component. >>> Haven't done that, yet, but it would be good to work on. On the >>"to-do" >>> list! >>> >>> I appreciate your feedback and that of others! >>> >>> Thanks, all! >>> >>> Rick >>> >>> >>> -----Original Message----- >>> From: Peter Boughton [mailto:[email protected]] >>> Sent: Sunday, August 21, 2011 9:49 AM >>> To: cf-talk >>> Subject: Re: Anyone care to review this image handling process? >>> >>> >>> I'd raise four must-fix issues with that code. >>> >>> 1: >>> You haven't var/local scoped any of these variables, despite >>being inside >>a >>> function which is probably going to end up in a shared scope, >>so this code >>> isn't thread-safe and thus can cause incorrect behaviour if two people >>> upload images at the same time. >>> (Or, possibly, you've not copied that part of code, since there's no >>> cffunction/cfargument lines either, but still safer to highlight the >>> problem.) >>> >>> 2: >>> The variable "loopCounter" never changes, which is probably not what you >>> want. >>> >>> The whole imageList looping is broken, because the list >>attribute accepts >>a >>> string (which is actually what you're giving it, so no errors), but the >>> imageList variable is a query. Either do >>list=#ValueList(imageList.name)# >>or >>> use query="imageList" instead. >>> >>> >>> (Also, don't forget the filter attribute. You can use it to do >>"file*" as >>> well as "*.ext" to reduce the number of files to check against) >>> >>> 3: >>> You've also got incorrect hash usage, e.g: >>> <cfset newImageFileName = reReplace(#originalImageFileName#, >>> '[^a-zA-Z0-9_\-.]', '', 'all') /> >>> <cfif image is '#newFileNameComplete#'> >>> <cfset renamedNewFileNameComplete = #newFileNameComplete#> >>> >>> (Yes, over-use of hashes is a must-fix issue, because it >>indicates a lack >>of >>> understanding of what's going on, so it's a bug in the programmers brain >>> that needs fixing.) >>> >>> 4: >>> As Justin points out, tempImages is going to keep growing. >>> Depending on where the images are uploaded from, you may simply >>be able to >>> FileDelete the uploaded version after the cfimage/resize has been done, >>and >>> that's that. >>> Or you could schedule a task that runs outside working hours to >>clear the >>> folder, or to remove anything over X days old (if there's a >>reason to keep >>> recent ones). >>> >>> >>> I wouldn't pass any code without those things being corrected, but a few >>> others which aren't specific bugs but still worth being mentioned/fixed >>are: >>> >>> 5: >>> You haven't use a variable for e:\tempImages, which means you'd need to >>> search/replace if/when the drive or path changes. >>> Better to have it defined in one place, so you can simply >>change a single >>> line if it does, and using a variable means any typo will be caught >>(whereas >>> a typo in a directory name might be silently accepted). >>> >>> 6: >>> I'd also point out that you've got an awful lot of single-use variables >>> there. Variables are variable. You can change what they contain >>and don't >>> need to keep creating new ones - you only need one or two >>"ImageFileName" >>> variables there, which you mutate as you go through the code. >>> >>> 7: >>> Based on all this being inside if/len/trim, I'm thinking this is just a >>> small part of a larger function, dealing with profile changing (or >>something >>> like that). >>> If so, I definitely suggest breaking down the function into smaller >>> self-contained sub-functions. >>> i.e. This logic should in be a processStaffPhoto function, with relevant >>> arguments being passed in and the final filename being returned at the >>end, >>> called by whatever the larger function is. Makes things significantly >>easier >>> to follow and maintain. >>> >>> >>> There we go... happy to expand on anything here if you want >>more details. >>> >>> >>> >>> >> >> >> >> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~| Order the Adobe Coldfusion Anthology now! http://www.amazon.com/Adobe-Coldfusion-Anthology/dp/1430272155/?tag=houseoffusion Archive: http://www.houseoffusion.com/groups/cf-talk/message.cfm/messageid:346964 Subscription: http://www.houseoffusion.com/groups/cf-talk/subscribe.cfm Unsubscribe: http://www.houseoffusion.com/groups/cf-talk/unsubscribe.cfm
