From fb34bb1ef89b94c3180e91e65dbc2058403c396a Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Thu, 31 Aug 2017 21:14:06 +0200 Subject: [PATCH 1/2] Use the second line of callstack in log_warning When using log_warning to log a warning to Rails logger we pass the line of the caller. As the instance method delegates to the class method we need to use the second line of the callstack. --- lib/alchemy/logger.rb | 2 +- spec/libraries/logger_spec.rb | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) create mode 100644 spec/libraries/logger_spec.rb diff --git a/lib/alchemy/logger.rb b/lib/alchemy/logger.rb index 60f1008448..2e83191665 100644 --- a/lib/alchemy/logger.rb +++ b/lib/alchemy/logger.rb @@ -7,7 +7,7 @@ def self.warn(message, caller_string) end def log_warning(message) - Alchemy::Logger.warn(message, caller(0..0)) + Alchemy::Logger.warn(message, caller(1..1)) end end end diff --git a/spec/libraries/logger_spec.rb b/spec/libraries/logger_spec.rb new file mode 100644 index 0000000000..73e85fd244 --- /dev/null +++ b/spec/libraries/logger_spec.rb @@ -0,0 +1,35 @@ +require 'spec_helper' + +RSpec.describe Alchemy::Logger do + let(:message) { "Something bad happened" } + + describe '.warn' do + let(:caller_string) { "file.rb:14" } + + subject { Alchemy::Logger.warn(message, caller_string) } + + it { is_expected.to be_nil } + + it 'uses Rails debug logger' do + expect(Rails.logger).to receive(:debug) { message } + subject + end + end + + describe '#log_warning' do + class Something + include Alchemy::Logger + end + + subject { Something.new.log_warning(message) } + + before do + expect_any_instance_of(Something).to receive(:caller).with(1..1) { ["second"] } + end + + it 'delegates to Alchemy::Logger.warn class method with second line of callstack' do + expect(Alchemy::Logger).to receive(:warn).with(message, ["second"]) + subject + end + end +end From 4dd249615ac6306d7ade9698a743b6ba870ffd8e Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Thu, 31 Aug 2017 21:16:17 +0200 Subject: [PATCH 2/2] 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