Merge branch 'master' into feature/reselect-method

6 files changed, 50 insertions, 129 deletions

diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb
index e7f2615b0f..53aa9f0237 100644
--- a/activestorage/app/models/active_storage/blob.rb
+++ b/activestorage/app/models/active_storage/blob.rb
@@ -35,6 +35,10 @@ class ActiveStorage::Blob < ActiveRecord::Base
 
   scope :unattached, -> { left_joins(:attachments).where(ActiveStorage::Attachment.table_name => { blob_id: nil }) }
 
+  before_destroy(prepend: true) do
+    raise ActiveRecord::InvalidForeignKey if attachments.exists?
+  end
+
   class << self
     # You can used the signed ID of a blob to refer to it on the client side without fear of tampering.
     # This is particularly helpful for direct uploads where the client-side needs to refer to the blob
diff --git a/activestorage/app/models/active_storage/blob/identifiable.rb b/activestorage/app/models/active_storage/blob/identifiable.rb
index 049e45dc3e..2c17ddc25f 100644
--- a/activestorage/app/models/active_storage/blob/identifiable.rb
+++ b/activestorage/app/models/active_storage/blob/identifiable.rb
@@ -15,6 +15,10 @@ module ActiveStorage::Blob::Identifiable
     end
 
     def download_identifiable_chunk
-      service.download_chunk key, 0...4.kilobytes
+      if byte_size.positive?
+        service.download_chunk key, 0...4.kilobytes
+      else
+        ""
+      end
     end
 end
diff --git a/activestorage/app/models/active_storage/filename.rb b/activestorage/app/models/active_storage/filename.rb
index bebb5e61b3..2a03e0173d 100644
--- a/activestorage/app/models/active_storage/filename.rb
+++ b/activestorage/app/models/active_storage/filename.rb
@@ -3,8 +3,6 @@
 # Encapsulates a string representing a filename to provide convenient access to parts of it and sanitization.
 # A Filename instance is returned by ActiveStorage::Blob#filename, and is comparable so it can be used for sorting.
 class ActiveStorage::Filename
-  require_dependency "active_storage/filename/parameters"
-
   include Comparable
 
   class << self
@@ -60,10 +58,6 @@ class ActiveStorage::Filename
     @filename.encode(Encoding::UTF_8, invalid: :replace, undef: :replace, replace: "�").strip.tr("\u{202E}%$|:;/\t\r\n\\", "-")
   end
 
-  def parameters #:nodoc:
-    Parameters.new self
-  end
-
   # Returns the sanitized version of the filename.
   def to_s
     sanitized.to_s
diff --git a/activestorage/app/models/active_storage/filename/parameters.rb b/activestorage/app/models/active_storage/filename/parameters.rb
deleted file mode 100644
index fb9ea10e49..0000000000
--- a/activestorage/app/models/active_storage/filename/parameters.rb
+++ /dev/null
@@ -1,36 +0,0 @@
-# frozen_string_literal: true
-
-class ActiveStorage::Filename::Parameters #:nodoc:
-  attr_reader :filename
-
-  def initialize(filename)
-    @filename = filename
-  end
-
-  def combined
-    "#{ascii}; #{utf8}"
-  end
-
-  TRADITIONAL_ESCAPED_CHAR = /[^ A-Za-z0-9!#$+.^_`|~-]/
-
-  def ascii
-    'filename="' + percent_escape(I18n.transliterate(filename.sanitized), TRADITIONAL_ESCAPED_CHAR) + '"'
-  end
-
-  RFC_5987_ESCAPED_CHAR = /[^A-Za-z0-9!#$&+.^_`|~-]/
-
-  def utf8
-    "filename*=UTF-8''" + percent_escape(filename.sanitized, RFC_5987_ESCAPED_CHAR)
-  end
-
-  def to_s
-    combined
-  end
-
-  private
-    def percent_escape(string, pattern)
-      string.gsub(pattern) do |char|
-        char.bytes.map { |byte| "%%%02X" % byte }.join
-      end
-    end
-end
diff --git a/activestorage/app/models/active_storage/variant.rb b/activestorage/app/models/active_storage/variant.rb
index 056fd2be99..ea57fa5f78 100644
--- a/activestorage/app/models/active_storage/variant.rb
+++ b/activestorage/app/models/active_storage/variant.rb
@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require "ostruct"
+
 # Image blobs can have variants that are the result of a set of transformations applied to the original.
 # These variants are used to create thumbnails, fixed-size avatars, or any other derivative image from the
 # original.
@@ -51,7 +53,7 @@
 # * {ImageProcessing::Vips}[https://github.com/janko-m/image_processing/blob/master/doc/vips.md#methods]
 # * {ruby-vips reference}[http://www.rubydoc.info/gems/ruby-vips/Vips/Image]
 class ActiveStorage::Variant
-  WEB_IMAGE_CONTENT_TYPES = %w( image/png image/jpeg image/jpg image/gif )
+  WEB_IMAGE_CONTENT_TYPES = %w[ image/png image/jpeg image/jpg image/gif ]
 
   attr_reader :blob, :variation
   delegate :service, to: :blob
@@ -95,37 +97,35 @@ class ActiveStorage::Variant
 
     def process
       blob.open do |image|
-        transform image do |output|
-          upload output
-        end
+        transform(image) { |output| upload(output) }
       end
     end
 
-
-    def filename
-      if WEB_IMAGE_CONTENT_TYPES.include?(blob.content_type)
-        blob.filename
-      else
-        ActiveStorage::Filename.new("#{blob.filename.base}.png")
-      end
+    def transform(image, &block)
+      variation.transform(image, format: format, &block)
     end
 
-    def content_type
-      blob.content_type.presence_in(WEB_IMAGE_CONTENT_TYPES) || "image/png"
+    def upload(file)
+      service.upload(key, file)
     end
 
-    def transform(image)
-      format = "png" unless WEB_IMAGE_CONTENT_TYPES.include?(blob.content_type)
-      result = variation.transform(image, format: format)
 
-      begin
-        yield result
-      ensure
-        result.close!
-      end
+    def specification
+      @specification ||=
+        if WEB_IMAGE_CONTENT_TYPES.include?(blob.content_type)
+          Specification.new \
+            filename: blob.filename,
+            content_type: blob.content_type,
+            format: nil
+        else
+          Specification.new \
+            filename: ActiveStorage::Filename.new("#{blob.filename.base}.png"),
+            content_type: "image/png",
+            format: "png"
+        end
     end
 
-    def upload(file)
-      service.upload(key, file)
-    end
+    delegate :filename, :content_type, :format, to: :specification
+
+    class Specification < OpenStruct; end
 end
diff --git a/activestorage/app/models/active_storage/variation.rb b/activestorage/app/models/active_storage/variation.rb
index ae1376a6cf..3adc2407e5 100644
--- a/activestorage/app/models/active_storage/variation.rb
+++ b/activestorage/app/models/active_storage/variation.rb
@@ -47,13 +47,9 @@ class ActiveStorage::Variation
   # saves the transformed image into a temporary file. If +format+ is specified
   # it will be the format of the result image, otherwise the result image
   # retains the source format.
-  def transform(file, format: nil)
+  def transform(file, format: nil, &block)
     ActiveSupport::Notifications.instrument("transform.active_storage") do
-      if processor
-        image_processing_transform(file, format)
-      else
-        mini_magick_transform(file, format)
-      end
+      transformer.transform(file, format: format, &block)
     end
   end
 
@@ -63,63 +59,22 @@ class ActiveStorage::Variation
   end
 
   private
-    # Applies image transformations using the ImageProcessing gem.
-    def image_processing_transform(file, format)
-      operations = transformations.each_with_object([]) do |(name, argument), list|
-        if name.to_s == "combine_options"
-          ActiveSupport::Deprecation.warn("The ImageProcessing ActiveStorage variant backend doesn't need :combine_options, as it already generates a single MiniMagick command. In Rails 6.1 :combine_options will not be supported anymore.")
-          list.concat argument.keep_if { |key, value| value.present? }.to_a
-        elsif argument.present?
-          list << [name, argument]
+    def transformer
+      if ActiveStorage.variant_processor
+        begin
+          require "image_processing"
+        rescue LoadError
+          ActiveSupport::Deprecation.warn <<~WARNING
+            Generating image variants will require the image_processing gem in Rails 6.1.
+            Please add `gem 'image_processing', '~> 1.2'` to your Gemfile.
+          WARNING
+
+          ActiveStorage::Transformers::MiniMagickTransformer.new(transformations)
+        else
+          ActiveStorage::Transformers::ImageProcessingTransformer.new(transformations)
         end
-      end
-
-      processor
-        .source(file)
-        .loader(page: 0)
-        .convert(format)
-        .apply(operations)
-        .call
-    end
-
-    # Applies image transformations using the MiniMagick gem.
-    def mini_magick_transform(file, format)
-      image = MiniMagick::Image.new(file.path, file)
-
-      transformations.each do |name, argument_or_subtransformations|
-        image.mogrify do |command|
-          if name.to_s == "combine_options"
-            argument_or_subtransformations.each do |subtransformation_name, subtransformation_argument|
-              pass_transform_argument(command, subtransformation_name, subtransformation_argument)
-            end
-          else
-            pass_transform_argument(command, name, argument_or_subtransformations)
-          end
-        end
-      end
-
-      image.format(format) if format
-
-      image.tempfile.tap(&:open)
-    end
-
-    # Returns the ImageProcessing processor class specified by `ActiveStorage.variant_processor`.
-    def processor
-      begin
-        require "image_processing"
-      rescue LoadError
-        ActiveSupport::Deprecation.warn("Using mini_magick gem directly is deprecated and will be removed in Rails 6.1. Please add `gem 'image_processing', '~> 1.2'` to your Gemfile.")
-        return nil
-      end
-
-      ImageProcessing.const_get(ActiveStorage.variant_processor.to_s.camelize) if ActiveStorage.variant_processor
-    end
-
-    def pass_transform_argument(command, method, argument)
-      if argument == true
-        command.public_send(method)
-      elsif argument.present?
-        command.public_send(method, argument)
+      else
+        ActiveStorage::Transformers::MiniMagickTransformer.new(transformations)
       end
     end
 end