#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

@@ -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();

modules/gallery/tests/Graphics_Helper_Test.php

@@ -0,0 +1,89 @@
+<?php defined("SYSPATH") or die("No direct script access.");
+/**
+ * Gallery - a web based photo album viewer and editor
+ * Copyright (C) 2000-2013 Bharat Mediratta
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or (at
+ * your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+class Graphics_Helper_Test extends Gallery_Unit_Test_Case {
+  public function generate_photo_test() {
+    $photo = test::random_photo();
+    // Check that the images were correctly resized
+    $this->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);
+  }
+}