From 4dd249615ac6306d7ade9698a743b6ba870ffd8e Mon Sep 17 00:00:00 2001
From: Thomas von Deyen <thomas@vondeyen.com>
Date: Thu, 31 Aug 2017 21:16:17 +0200
Subject: [PATCH] Handle custom errors in Alchemy::Picture#url

Currently we raise custom errors in the Alchemy::Picture#url
method without rescueing them later. This causes pages and the image library
to fail completely if one the images could not be rendered because the image file
is missing or the image format could not be used.

This is bad user experience. We should not display the broken image and not break the whole page.
---
 CHANGELOG.md                            |  4 ++++
 app/models/alchemy/picture/url.rb       | 10 +++++++++-
 lib/alchemy/errors.rb                   |  7 ++++++-
 spec/models/alchemy/picture_url_spec.rb | 18 ++++++++++++++----
 4 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index bb3a8863e1..5de353a883 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,9 @@
 # Change Log
 
+## 3.6.2 (unreleased)
+
+* Handle custom errors in `Alchemy::Picture#url` [#1305](https://github.com/AlchemyCMS/alchemy_cms/pull/1305) by [tvdeyen](https://github.com/tvdeyen)
+
 ## 3.6.1 (2017-08-16)
 
 * Do not ask `systempage?` everytime we load the page definition [#1239](https://github.com/AlchemyCMS/alchemy_cms/pull/1283) by [tvdeyen](https://github.com/tvdeyen)
diff --git a/app/models/alchemy/picture/url.rb b/app/models/alchemy/picture/url.rb
index fb6465449e..c85941bc7d 100644
--- a/app/models/alchemy/picture/url.rb
+++ b/app/models/alchemy/picture/url.rb
@@ -1,5 +1,7 @@
 module Alchemy
   module Picture::Url
+    include Alchemy::Logger
+
     TRANSFORMATION_OPTIONS = [
       :crop,
       :crop_from,
@@ -28,6 +30,9 @@ def url(options = {})
       image = encoded_image(image, options)
 
       image.url(options.except(*TRANSFORMATION_OPTIONS).merge(name: name))
+    rescue MissingImageFileError, WrongImageFormatError => e
+      log_warning e.message
+      nil
     end
 
     private
@@ -53,7 +58,10 @@ def processed_image(image, options = {})
     #
     def encoded_image(image, options = {})
       target_format = options[:format] || default_render_format
-      raise WrongImageFormatError if !target_format.in?(Alchemy::Picture.allowed_filetypes)
+
+      unless target_format.in?(Alchemy::Picture.allowed_filetypes)
+        raise WrongImageFormatError.new(self, target_format)
+      end
 
       options = {
         flatten: target_format != 'gif' && image_file_format == 'gif'
diff --git a/lib/alchemy/errors.rb b/lib/alchemy/errors.rb
index a66162ddb8..c557820d76 100644
--- a/lib/alchemy/errors.rb
+++ b/lib/alchemy/errors.rb
@@ -55,9 +55,14 @@ class MissingImageFileError < StandardError; end
 
   # Raised if calling +image_file+ on a Picture object returns nil.
   class WrongImageFormatError < StandardError
+    def initialize(image, requested_format)
+      @image = image
+      @requested_format = requested_format
+    end
+
     def message
       allowed_filetypes = Alchemy::Picture.allowed_filetypes.map(&:upcase).to_sentence
-      "Requested image format is not one of allowed filetypes (#{allowed_filetypes})."
+      "Requested image format (#{@requested_format.inspect}) for #{@image.inspect} is not one of allowed filetypes (#{allowed_filetypes})."
     end
   end
 
diff --git a/spec/models/alchemy/picture_url_spec.rb b/spec/models/alchemy/picture_url_spec.rb
index d05ac960c7..537b15a31f 100644
--- a/spec/models/alchemy/picture_url_spec.rb
+++ b/spec/models/alchemy/picture_url_spec.rb
@@ -35,8 +35,13 @@ def decode_dragon_fly_job(url)
           expect(picture).to receive(:image_file) { nil }
         end
 
-        it 'raises error' do
-          expect { url }.to raise_error(Alchemy::MissingImageFileError)
+        it 'returns nil' do
+          expect(url).to be_nil
+        end
+
+        it "logs warning" do
+          expect(Alchemy::Logger).to receive(:warn)
+          url
         end
       end
 
@@ -171,8 +176,13 @@ def decode_dragon_fly_job(url)
           {format: 'zip'}
         end
 
-        it "raises error" do
-          expect { url }.to raise_error(Alchemy::WrongImageFormatError)
+        it "returns nil" do
+          expect(url).to be_nil
+        end
+
+        it "logs warning" do
+          expect(Alchemy::Logger).to receive(:warn)
+          url
         end
       end