From 536bdaa4db6e950cb4f382697964651b8eab63cd Mon Sep 17 00:00:00 2001 From: shadlaws Date: Tue, 29 Jan 2013 18:35:10 +0100 Subject: [PATCH] #1967 - Improve how graphics::generate handles missing/bad images. - Made missing_photo match the image format (jpg, png, etc.). - Swapped missing_photo.png for missing_photo.jpg since it's likely to require less conversion to match. - Improved error messages to user when things go wrong. - Ensured that missing image placeholders are always copied when there's an error. - Ensured we don't mistake no file output for a correct file output (delete target before attempt). - Restructured graphics::generate a bit to work better with above changes. - Added unit tests for graphics::generate. --- modules/gallery/helpers/graphics.php | 78 ++++++++++++--- modules/gallery/images/missing_photo.jpg | Bin 0 -> 2034 bytes modules/gallery/images/missing_photo.png | Bin 1570 -> 0 bytes .../gallery/tests/Graphics_Helper_Test.php | 89 ++++++++++++++++++ 4 files changed, 153 insertions(+), 14 deletions(-) create mode 100644 modules/gallery/images/missing_photo.jpg delete mode 100644 modules/gallery/images/missing_photo.png create mode 100644 modules/gallery/tests/Graphics_Helper_Test.php diff --git a/modules/gallery/helpers/graphics.php b/modules/gallery/helpers/graphics.php index 0c5f8366d3..3f5e2d5671 100644 --- a/modules/gallery/helpers/graphics.php +++ b/modules/gallery/helpers/graphics.php @@ -154,10 +154,9 @@ static function generate($item) { try { foreach ($ops as $target => $output_file) { + // Delete anything that might already be there + @unlink($output_file); if ($input_item->is_movie()) { - // Convert the movie filename to a JPG first, delete anything that might already be there - $output_file = legal_file::change_extension($output_file, "jpg"); - @unlink($output_file); // Run movie_extract_frame events, which can either: // - generate an output file, bypassing the ffmpeg-based movie::extract_frame // - add to the options sent to movie::extract_frame (e.g. change frame extract time, @@ -173,8 +172,8 @@ static function generate($item) { try { movie::extract_frame($input_file, $output_file, $movie_options_wrapper->movie_options); } catch (Exception $e) { - // Didn't work, likely because of MISSING_FFMPEG - copy missing_movie instead - copy(MODPATH . "gallery/images/missing_movie.jpg", $output_file); + // Didn't work, likely because of MISSING_FFMPEG - use placeholder + graphics::_replace_image_with_placeholder($item, $target); } } $working_file = $output_file; @@ -193,31 +192,82 @@ static function generate($item) { if (file_exists($item->thumb_path())) { $item->thumb_dirty = 0; } else { - copy(MODPATH . "gallery/images/missing_photo.png", $item->thumb_path()); + Kohana_Log::add("error", "Failed to rebuild thumb image: $item->title"); + graphics::_replace_image_with_placeholder($item, "thumb"); } - list ($item->thumb_width, $item->thumb_height) = - photo::get_file_metadata($item->thumb_path()); } if (!empty($ops["resize"])) { if (file_exists($item->resize_path())) { $item->resize_dirty = 0; } else { - copy(MODPATH . "gallery/images/missing_photo.png", $item->resize_path()); + Kohana_Log::add("error", "Failed to rebuild resize image: $item->title"); + graphics::_replace_image_with_placeholder($item, "resize"); } - list ($item->resize_width, $item->resize_height) = - photo::get_file_metadata($item->resize_path()); } + graphics::_update_item_dimensions($item); $item->save(); } catch (Exception $e) { - // Something went wrong rebuilding the image. Leave it dirty and move on. - // @todo we should handle this better. - Kohana_Log::add("error", "Caught exception rebuilding image: {$item->title}\n" . + // Something went wrong rebuilding the image. Replace with the placeholder images, + // leave it dirty and move on. + Kohana_Log::add("error", "Caught exception rebuilding images: {$item->title}\n" . $e->getMessage() . "\n" . $e->getTraceAsString()); + if ($item->is_photo()) { + graphics::_replace_image_with_placeholder($item, "resize"); + } + graphics::_replace_image_with_placeholder($item, "thumb"); + graphics::_update_item_dimensions($item); + $item->save(); throw $e; } } + private static function _update_item_dimensions($item) { + if ($item->is_photo()) { + list ($item->resize_width, $item->resize_height) = + photo::get_file_metadata($item->resize_path()); + } + list ($item->thumb_width, $item->thumb_height) = + photo::get_file_metadata($item->thumb_path()); + } + + private static function _replace_image_with_placeholder($item, $target) { + if ($item->is_movie() || ($item->is_album() && $item->album_cover()->is_movie())) { + $input_path = MODPATH . "gallery/images/missing_movie.jpg"; + } else { + $input_path = MODPATH . "gallery/images/missing_photo.jpg"; + } + + if ($target == "thumb") { + $output_path = $item->thumb_path(); + $size = module::get_var("gallery", "thumb_size", 200); + } else { + $output_path = $item->resize_path(); + $size = module::get_var("gallery", "resize_size", 640); + } + $options = array("width" => $size, "height" => $size, "master" => Image::AUTO); + + try { + // Copy/convert/resize placeholder as needed. + gallery_graphics::resize($input_path, $output_path, $options, null); + } catch (Exception $e) { + // Copy/convert/resize didn't work. Add to the log and copy the jpg version (which could have + // a non-jpg extension). This is less than ideal, but it's better than putting nothing + // there and causing theme views to act strangely because a file is missing. + // @todo we should handle this better. + Kohana_Log::add("error", "Caught exception converting placeholder for missing image: " . + $item->title . "\n" . $e->getMessage() . "\n" . $e->getTraceAsString()); + copy($input_path, $output_path); + } + + if (!file_exists($output_path)) { + // Copy/convert/resize didn't throw an exception, but still didn't work - do the same as above. + // @todo we should handle this better. + Kohana_Log::add("error", "Failed to convert placeholder for missing image: $item->title"); + copy($input_path, $output_path); + } + } + private static function _get_rules($target) { if (empty(self::$_rules_cache[$target])) { $rules = array(); diff --git a/modules/gallery/images/missing_photo.jpg b/modules/gallery/images/missing_photo.jpg new file mode 100644 index 0000000000000000000000000000000000000000..a9d176d8a337fa9a9d5a2dc4b8369df59998b68f GIT binary patch literal 2034 zcmb_bdo?GUERuO8 zA+Kvrb0MX~HVkno(W+X9RWzZPYNTP2(Y@#FIPW=o|Ja`Ad_U)Vp6B^~p6~N{zF*}d zn&}aBf3WVCdNkk zhK8mVFl$pYJ99%r8&_Mqw;kYcxQX={FE>X|m=oMl*$ZewfI(mo1cCy}KEMC~0V=8> z5b(!PQ3I)h)iw4uy^aDZAdu=Iuo^^N_4flH6;(B82w3~r2?sdgR+;X(u-H2~sW-kg za}52gjy3U%y1BCtb-_0+SNRx#?9-kFc3%C#{U;uj%(fklv>Lle35=(Pgap~*n$e9E zld$JKba-|!YrAwp>MdCI4Vc=oZ#?{Z`4Us_gQ!vLY*!55mx-c_Od86FGJ}7fpL2*Cc@6&xo)#&V>7@99_;#QZi zCh*|XN+5tKyYA?jx0F`EAlw^ub1%nqjrA7`niN?knSE=a3{uxt&FfI$zyk|J<6`OV z`cgoV(kU_T+?aE2_CY=6J5yyNSXYZIP#6~_E0zYcD60N|yo7W8$b6T3_&5ACw` zZEMWSOq-z1_xFSJSJtF%xH@0!tB&(>S^{Re?37||q~rt%K|8EjsCvbwc|Efop8lMh z(iJ<$?vhtN>;=rv43M$(wZMCg^yh7yO9QPh2%}xjt1v54cd{d(-nr8qq$j>GG&{oc zPMWPeCWo;WejR*KY|@p_ljo=#p66Bb{9V}z)5lxU_^I@k^hxAUedB6OBEG2UbG%I! zCQ~{`zB$xT{w{nA9tW*h0ACXw-+9t+5EN0LSROJ$8Cwm9_HslRZiemXsrHkF!gw;{ zQZ9k`jFG*Y*uoB$I|QM-GiNvLtl|zzT_g{|l{YboO`gIAL~rg5Jr$U_i>OA?N3C$? zG6&1K@8SzdF^fJF%r_71J#akfL-)1_jO$!ZT(%~%qBROdfpmwB-SwD^Bx@$1o1-|4 zQ(QVZWRwy$I%j>oH&4`T-1Jo>eB3v_ZLw{deZ;9*Bh%b{gW-l?T%!y>egB{|+=^pc z`n5+creGm%M^nrsrRjq_)*EVRQTYRsh79q05A>0)22fyNK$d%ixd;txDB6D5u{KS>X|otUoC~P{8kXe3apB{GX@TLN{oN;zz{yvQ&-WeG~j*CfQvx z-M0lIaXyx@bguViN!2`}?`A=vkbrl%*&S2)ofFf#(WR=~ga3V}*hR`uQb2{n1}(2F z#c6gfg^xpu56R?|rqD5*PHbDxdRX4JuH(NpVMj=(XFJ^Y6iDrLNE!9plL4AyD!G+D z=pJVX;q6KCb1W5R{qJ34^o8qNXZ_My#geekV!l)vvZfGkiCb2mY}g$rU#bF+Z?!!) zm)6@zt1Fb({VCA&U5<~F9H|!;podN&QS2)0cBNp*jdl;b#tt8aP zQ_HxzFH0p)!y0qGfFJY3OYQFu+^8>#so&O1e)ISv39nzU;`)-5-S^)FDXeVbnwMow zVjt^3rx#c6cN_qK-=TG0oUf{TmRuXXQ-VyG*|4i7r`6eX7hzLZd?w;KsC2x)1JR;= zo7(b%+h&00e~uPk7FOO?0(PBda1pJq%Fmz7`M6A+FRU!lj)v+JCVmd2ng?M#k?DSy z1OWxG{Gu~S88tn1) z5c5MqmhnhP$f7((8c5Rjj{N(85Ve#;DY`AyZ#SqC}(~H90sAj literal 0 HcmV?d00001 diff --git a/modules/gallery/images/missing_photo.png b/modules/gallery/images/missing_photo.png deleted file mode 100644 index 677862751f7d1708d534687e4c8010242793106a..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 1570 zcmZ`(dpOe#6rM{ptNDgSqOW1@L?sOqlgoVWmt>=K*A2PkQu(U6C%05$BauG2R4P$I zwy~dFxxFZHGZ15V$km-W!a- zFN4Z}z99K%6&R!g&+h*x1z6)ZECd4x3{maufM;4U|@jF zW)BVy4h;R$)6>&4Gc&WZ zvvYHE^Yily3k!>jiz1O|X=!PBd08wLudJ*{B$CzD)wQ*?pFe-DudmmWV@p8ra+mOg zNC;%J(l3*O6i~K-#Z6JpZVsD)-p}PfLw;>SOWoLVwZ|uFDCE(PU033TUIO?ISC1wQICeQ6#A5@7#PS zu(lSjdvZ|{kCSyyE{4)(ueO~vCpRAb4g%MP;baL(BV6v$prGJ)^kb&wz6QDNr_9OL z*4A>GK-kSs6ALJ4)rLQ_82&~+vI=}Hgn?@S{4pmqrh$`^k}?(b!}*+MYH)Vg^U`HY zVzu8>UxPemEivO2UI|<$XMw?Buvo06rGaufcqqRR(z8`QKk*;rmuwu6!(ccS@YI*S zk*-;SliD|GT>3t>+HFb4PQh^zc3551bn8(4L?ZsZ%l!MVRG0P?FENU8{ zQWSywys{F9#mOq5;e4THbg3ya{{+llZPBOxz!E27j7%d;rcb)M3r_=hJ~0x$5ayy2=G_8fE>rt7Hq4z z^ZIH3dOHV--_}+Z64ywNs7BY{kE>D98X2Z{NTEaHsanYc@X*H4Hm0 z4z6r!FTvcRrg*Wpr{i7W``okerjIkeo;(nXoJRDagIeqEzuYW zdlQ&2X>rT#Faqp?pR4iblk^?@Zi?lnVzD}IJ+^v*m3{T32A%I*p-8NLwsAi}c5aLJ zQ{VDCwTbeltA!Q7lJgN#U|^sbxdR2ol=V)beG`U~kK=c%Q||vG))UO$LObveA}n2kK^yBA#n3vP60m zz;lkSZ%iUKT5P;Q^O98bX4@6-dt&-r*EN6IXn)X~fUI)0uq&4@Cw&zqCxIjr)BSZL z(sD;I3zjOQV~e8b-08N{s^qNMaMi@f4BJdo#A((%vbDrog}yg+4OzD%NvX_L{lYCb zx=-f(08wDZ+~WjNK0i39nxz^*tiCY|yMA)|EjT&$+SXPl`}T|l%extf`b;(No=}EH z{n<_x8$$%$29H(r6gj-NV2xn%2wMrqi{8=G4ptG`PSp9p#;nj8dhuguBPQF}(S`6z zcGUPCXV*DRz#MnqKHOKw>j~I2sX9`H?gNacjIZB&P({xg*nPzikEr7s0?M}Ys{em}k=-`xZmaUIR^R>};y-2ZY!DNMG43;rKLg+}SmX z=&JOz(}Gj|6L7mulfSHiSMrlC5qR%Q1>7K)aY+$5oe;_4_$H(0DG2WhaHX zOA+iTe;~OoA&I5K)lE)t3fUyS@>sy{CFz$|tzO6u%6TWD5alU{Kvje|JGk3FvpYum E56}$^7XSbN diff --git a/modules/gallery/tests/Graphics_Helper_Test.php b/modules/gallery/tests/Graphics_Helper_Test.php new file mode 100644 index 0000000000..ddcb9dfd5e --- /dev/null +++ b/modules/gallery/tests/Graphics_Helper_Test.php @@ -0,0 +1,89 @@ +assert_equal(array(640, 480, "image/jpeg", "jpg"), + photo::get_file_metadata($photo->resize_path())); + $this->assert_equal(array(200, 150, "image/jpeg", "jpg"), + photo::get_file_metadata($photo->thumb_path())); + // Check that the items table got updated + $this->assert_equal(array(640, 480), array($photo->resize_width, $photo->resize_height)); + $this->assert_equal(array(200, 150), array($photo->thumb_width, $photo->thumb_height)); + // Check that the images are not marked dirty + $this->assert_equal(0, $photo->resize_dirty); + $this->assert_equal(0, $photo->thumb_dirty); + } + + public function generate_movie_test() { + $movie = test::random_movie(); + // Check that the image was correctly resized + $this->assert_equal(array(200, 160, "image/jpeg", "jpg"), + photo::get_file_metadata($movie->thumb_path())); + // Check that the items table got updated + $this->assert_equal(array(200, 160), array($movie->thumb_width, $movie->thumb_height)); + // Check that the image is not marked dirty + $this->assert_equal(0, $movie->thumb_dirty); + } + + public function generate_bad_photo_test() { + $photo = test::random_photo(); + // At this point, the photo is valid and has a valid resize and thumb. Make it garble. + file_put_contents($photo->file_path(), test::lorem_ipsum(200)); + // Regenerate + $photo->resize_dirty = 1; + $photo->thumb_dirty = 1; + try { + graphics::generate($photo); + $this->assert_true(false, "Shouldn't get here"); + } catch (Exception $e) { + // Exception expected + } + // Check that the images got replaced with missing image placeholders + $this->assert_same(file_get_contents(MODPATH . "gallery/images/missing_photo.jpg"), + file_get_contents($photo->resize_path())); + $this->assert_same(file_get_contents(MODPATH . "gallery/images/missing_photo.jpg"), + file_get_contents($photo->thumb_path())); + // Check that the items table got updated with new metadata + $this->assert_equal(array(200, 200), array($photo->resize_width, $photo->resize_height)); + $this->assert_equal(array(200, 200), array($photo->thumb_width, $photo->thumb_height)); + // Check that the images are marked as dirty + $this->assert_equal(1, $photo->resize_dirty); + $this->assert_equal(1, $photo->thumb_dirty); + } + + public function generate_bad_movie_test() { + // Unlike photos, its ok to have missing movies - no thrown exceptions, thumb_dirty can be reset. + $movie = test::random_movie(); + // At this point, the movie is valid and has a valid thumb. Make it garble. + file_put_contents($movie->file_path(), test::lorem_ipsum(200)); + // Regenerate + $movie->thumb_dirty = 1; + graphics::generate($movie); + // Check that the image got replaced with a missing image placeholder + $this->assert_same(file_get_contents(MODPATH . "gallery/images/missing_movie.jpg"), + file_get_contents($movie->thumb_path())); + // Check that the items table got updated with new metadata + $this->assert_equal(array(200, 200), array($movie->thumb_width, $movie->thumb_height)); + // Check that the image is *not* marked as dirty + $this->assert_equal(0, $movie->thumb_dirty); + } +} \ No newline at end of file