From 6faca022db3f518c7e0d70433fc4c8a17dc8745a Mon Sep 17 00:00:00 2001 From: Earlopain <14981592+Earlopain@users.noreply.github.com> Date: Sat, 20 Sep 2025 15:37:07 +0200 Subject: [PATCH] Restore support to analyze with `image_magick` Since https://github.com/rails/rails/commit/93c00a8c74c24a3da40d30b6d0c3c667b8ebe9ba, `config.active_storage.analyzers` contains both vips and image magick analyzers. However, they both inherit from `Analyzer::ImageAnalyzer`, which means they just check for `blob.image?` to check if they should accept the image. That means that even if the user explicitly `variant_processor = :mini_magick`, vips gets called first (because it comes first in the array), and image magick never gets a chance to do anything. Before that commit it worked because the analyzers were conditionally added to the array, so there was only ever one or the other present. This fixes this by once again implementing custom `accept?`, which was previously removed in https://github.com/rails/rails/commit/e978ef011f11102771c5b302f25d9ab7895316cb --- .../analyzer/image_analyzer/image_magick.rb | 4 ++++ .../analyzer/image_analyzer/vips.rb | 4 ++++ .../analyzer/image_analyzer/image_magick_test.rb | 16 ++++++++++++++-- .../test/analyzer/image_analyzer/vips_test.rb | 4 ++-- 4 files changed, 24 insertions(+), 4 deletions(-) diff --git a/activestorage/lib/active_storage/analyzer/image_analyzer/image_magick.rb b/activestorage/lib/active_storage/analyzer/image_analyzer/image_magick.rb index 65c843415f1e9..e15629a504f6d 100644 --- a/activestorage/lib/active_storage/analyzer/image_analyzer/image_magick.rb +++ b/activestorage/lib/active_storage/analyzer/image_analyzer/image_magick.rb @@ -11,6 +11,10 @@ module ActiveStorage # This analyzer relies on the third-party {MiniMagick}[https://github.com/minimagick/minimagick] gem. MiniMagick requires # the {ImageMagick}[http://www.imagemagick.org] system library. class Analyzer::ImageAnalyzer::ImageMagick < Analyzer::ImageAnalyzer + def self.accept?(blob) + super && ActiveStorage.variant_processor == :mini_magick + end + private def read_image download_blob_to_tempfile do |file| diff --git a/activestorage/lib/active_storage/analyzer/image_analyzer/vips.rb b/activestorage/lib/active_storage/analyzer/image_analyzer/vips.rb index ca2ead77ea076..8f86c26e25f3e 100644 --- a/activestorage/lib/active_storage/analyzer/image_analyzer/vips.rb +++ b/activestorage/lib/active_storage/analyzer/image_analyzer/vips.rb @@ -19,6 +19,10 @@ module ActiveStorage # This analyzer relies on the third-party {ruby-vips}[https://github.com/libvips/ruby-vips] gem. Ruby-vips requires # the {libvips}[https://libvips.github.io/libvips/] system library. class Analyzer::ImageAnalyzer::Vips < Analyzer::ImageAnalyzer + def self.accept?(blob) + super && ActiveStorage.variant_processor == :vips + end + private def read_image download_blob_to_tempfile do |file| diff --git a/activestorage/test/analyzer/image_analyzer/image_magick_test.rb b/activestorage/test/analyzer/image_analyzer/image_magick_test.rb index 5b78a8e7a375f..bc80288f91bd3 100644 --- a/activestorage/test/analyzer/image_analyzer/image_magick_test.rb +++ b/activestorage/test/analyzer/image_analyzer/image_magick_test.rb @@ -46,6 +46,18 @@ class ActiveStorage::Analyzer::ImageAnalyzer::ImageMagickTest < ActiveSupport::T end end + test "analyzing with ruby-vips unavailable" do + stub_const(Object, :Vips, Module.new) do + analyze_with_image_magick do + blob = create_file_blob(filename: "racecar.jpg", content_type: "image/jpeg") + metadata = extract_metadata_from(blob) + + assert_equal 4104, metadata[:width] + assert_equal 2736, metadata[:height] + end + end + end + test "instrumenting analysis" do blob = create_file_blob(filename: "racecar.jpg", content_type: "image/jpeg") @@ -60,12 +72,12 @@ class ActiveStorage::Analyzer::ImageAnalyzer::ImageMagickTest < ActiveSupport::T private def analyze_with_image_magick - previous_analyzers, ActiveStorage.analyzers = ActiveStorage.analyzers, [ActiveStorage::Analyzer::ImageAnalyzer::ImageMagick] + previous_processor, ActiveStorage.variant_processor = ActiveStorage.variant_processor, :mini_magick yield rescue LoadError ENV["BUILDKITE"] ? raise : skip("Variant processor image_magick is not installed") ensure - ActiveStorage.analyzers = previous_analyzers + ActiveStorage.variant_processor = previous_processor end end diff --git a/activestorage/test/analyzer/image_analyzer/vips_test.rb b/activestorage/test/analyzer/image_analyzer/vips_test.rb index 5c4387d2d11bb..446986bd6929f 100644 --- a/activestorage/test/analyzer/image_analyzer/vips_test.rb +++ b/activestorage/test/analyzer/image_analyzer/vips_test.rb @@ -60,12 +60,12 @@ class ActiveStorage::Analyzer::ImageAnalyzer::VipsTest < ActiveSupport::TestCase private def analyze_with_vips - previous_analyzers, ActiveStorage.analyzers = ActiveStorage.analyzers, [ActiveStorage::Analyzer::ImageAnalyzer::Vips] + previous_processor, ActiveStorage.variant_processor = ActiveStorage.variant_processor, :vips yield rescue LoadError ENV["BUILDKITE"] ? raise : skip("Variant processor vips is not installed") ensure - ActiveStorage.analyzers = previous_analyzers + ActiveStorage.variant_processor = previous_processor end end