Load configured Active Storage plugins during boot #45100

skipkayhil · 2022-05-14T22:59:57Z

Summary

Previously, the parts of Active Storage using ruby-vips, mini_magick, or
image_processing would try to load gems only when used. This strategy works
well because it allows apps that don't need these features to easily
ignore them and not have to depend on gems they don't need.

However, the downsides to this strategy are that it requires loading
code during requests and that it moves potential error messages into request
logs instead of those errors being immediately visible on boot.

This commit addresses these issues by restructing how the Vips and Image
Magick transformers/analyzers are organized so that they will be loaded
(if configured) on boot along with whatever dependencies they need.

For now, if :vips or :mini_magick is specified as the Active Storage
:variant_processor, not having the required gem in the Gemfile will print a
deprecation warning instead of erroring because it is likely many apps
are currently configured this way.

Other Information

Looking for feedback on the approach, I will add tests/docs/changelog etc. after

Closes #44991
cc @byroot because of our discussion in the other issue

byroot

A bunch of smaller comments, but looks mostly good to me.

byroot · 2022-05-15T13:18:54Z

activestorage/lib/active_storage.rb

@@ -46,6 +46,8 @@ module ActiveStorage
  mattr_accessor :verifier
  mattr_accessor :variant_processor, default: :mini_magick

+  mattr_accessor :variant_transformer


Note to self: we should probably convert all these mattr_accessor to attr_accessor.

byroot · 2022-05-15T13:20:30Z

activestorage/lib/active_storage/analyzer/image_analyzer/image_magick.rb

+    Setting config.active_storage.variant_processor to :mini_magick without adding
+    mini_magick to the Gemfile is deprecated and will raise in Rails 7.2.
+
+    To fix this warning, set config.active_storage.variant_processor = :nil


:nil is weird, did you mean nil?

byroot · 2022-05-15T13:21:44Z

activestorage/lib/active_storage/analyzer/image_analyzer/vips.rb

+    Setting config.active_storage.variant_processor to :vips without adding
+    ruby-vips to the Gemfile is deprecated and will raise in Rails 7.2.
+
+    To fix this warning, set config.active_storage.variant_processor = :nil


byroot · 2022-05-15T13:25:56Z

activestorage/lib/active_storage/transformers/image_magick.rb

+        def validate_transformation(name, argument)
+          method_name = name.to_s.tr("-", "_")
+
+          unless ActiveStorage.supported_image_processing_methods.any? { |method| method_name == method }


I know you just moved it over, but I wonder why this isn't just:

Suggested change

unless ActiveStorage.supported_image_processing_methods.any? { |method| method_name == method }

unless ActiveStorage.supported_image_processing_methods.include?(method_name)

byroot · 2022-05-15T13:27:08Z

activestorage/lib/active_storage/transformers/image_magick.rb

+
+          unless ActiveStorage.supported_image_processing_methods.any? { |method| method_name == method }
+            raise UnsupportedImageProcessingMethod, <<~ERROR.squish
+              One or more of the provided transformation methods is not supported.


When writing error message, it's best to include the faulty value.

byroot · 2022-05-15T13:29:55Z

activestorage/test/models/variant_test.rb

+          ActiveStorage::Transformers::Vips
+        when :mini_magick
+          ActiveStorage::Transformers::ImageMagick
+        end


Might want to have an else that raises if the argument wasn't any of the two, this way if someone typo :mini_magick into say :minimagick, they get an early error.

byroot · 2024-11-16T11:08:26Z

Hum, this has fallen through the cracks because I don't have push notifications. I think we should do it though. I'll try to find time to rebase this (but I don't even think we need a deprecation here)

Previously, the parts of Active Storage using ruby-vips, mini_magick, or image_processing would try to load gems only when used. This strategy works well because it allows apps that don't need these features to easily ignore them and not have to depend on gems they don't need. However, the downsides to this strategy are that it requires loading code during requests and that it moves potential error messages into request logs instead of those errors being immediately visible on boot. This commit addresses these issues by restructing how the Vips and Image Magick transformers/analyzers are organized so that they will be loaded (if configured) on boot along with whatever dependencies they need. For now, if :vips or :mini_magick is specified as the Active Storage :variant_processor, not having the required gem in the Gemfile will print a deprecation warning instead of erroring because it is likely many apps are currently configured this way.

zzak · 2025-01-07T11:49:46Z

I think this is ready for another look, gave it a rebase and addressed some of the comments.

Earlopain · 2025-01-07T16:50:44Z

activestorage/lib/active_storage/analyzer/image_analyzer/image_magick.rb

+begin
+  gem "mini_magick"
+rescue LoadError => error
+  raise error unless error.message.include?("ruby-vips")


Copy-paste error? I think this will cause it to not show the deprecation warning.

Suggested change

raise error unless error.message.include?("ruby-vips")

raise error unless error.message.include?("mini_magick")

🤦 Thanks, was fixing a rubocop failure

Fixes rails#54152 In rails#45100 we decided that when the variant_processor is :vips or :mini_magick, we should try to eagerly load the library and throw a warning if the library was not available. However, because `:vips` is the default, this will always show the warning unless the developer adds `image_processing` gem to their dependencies. If they do not want to do this, they can opt-out by setting `config.active_storage.variant_processor = nil`.

ghiculescu · 2025-01-30T05:21:13Z

activestorage/lib/active_storage/analyzer/image_analyzer/image_magick.rb

@@ -1,22 +1,17 @@
 # frozen_string_literal: true

+begin
+  gem "mini_magick"


@skipkayhil how come you don't require the gem here? You do in the vips anlayzer, and I think that's causing issues, but I can't work if that's an intentional oversight.

Tanda @ Rails 8.1.0.alpha (test) :008 > gem "mini_magick" true Tanda @ Rails 8.1.0.alpha (test) :009 > MiniMagick (web):9:in `<main>': uninitialized constant MiniMagick (NameError) MiniMagick ^^^^^^^^^^ Did you mean? MiniMime Tanda @ Rails 8.1.0.alpha (test) :010 > ::MiniMagick (web):10:in `<main>': uninitialized constant MiniMagick (NameError) ::MiniMagick ^^^^^^^^^^^^ Did you mean? MiniMime Tanda @ Rails 8.1.0.alpha (test) :011 > require "mini_magick" true Tanda @ Rails 8.1.0.alpha (test) :012 > MiniMagick MiniMagick

I think the missing require was not intentional

ref: rails#45100 (comment) Compare: https://github.com/rails/rails/blob/885734a85ef4f4578400bda72a7507dd09a85e64/activestorage/lib/active_storage/analyzer/image_analyzer/vips.rb#L5

rails-bot bot added the activestorage label May 14, 2022

skipkayhil force-pushed the load-as-gems-in-initializer branch from 7efa39d to 1859dc9 Compare May 14, 2022 23:02

byroot reviewed May 15, 2022

View reviewed changes

skipkayhil force-pushed the load-as-gems-in-initializer branch from 1859dc9 to 6e554d3 Compare June 22, 2022 01:40

skipkayhil mentioned this pull request Jun 22, 2022

require mini_magick at the level it is being used #45420

Merged

byroot mentioned this pull request Nov 16, 2024

Show better error messages when libvips is missing #44991

Closed

zzak force-pushed the load-as-gems-in-initializer branch from 6e554d3 to 7c8e351 Compare January 7, 2025 11:30

zzak force-pushed the load-as-gems-in-initializer branch from 7c8e351 to 684e760 Compare January 7, 2025 11:40

rails-bot bot added the railties label Jan 7, 2025

zzak requested a review from byroot January 7, 2025 11:49

rafaelfranca merged commit e978ef0 into rails:main Jan 7, 2025
3 checks passed

Earlopain reviewed Jan 7, 2025

View reviewed changes

skipkayhil deleted the load-as-gems-in-initializer branch January 7, 2025 22:55

Earlopain mentioned this pull request Jan 8, 2025

[head] ActiveStorage shows deprecation warning about image_processing even without using it #54152

Open

zzak mentioned this pull request Jan 8, 2025

Warn to set config.as.variant_processor = nil #54155

Closed

Earlopain mentioned this pull request Jan 8, 2025

Don't fail to set ActiveStorage analyser because the transformer failed to load #54156

Closed

4 tasks

djmb mentioned this pull request Jan 29, 2025

In Active Storage config.active_storage.analyzers = [] is not obeyed #54391

Open

ghiculescu reviewed Jan 30, 2025

View reviewed changes

ghiculescu mentioned this pull request Jan 30, 2025

Add missing require in ImageMagick analyzer #54411

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load configured Active Storage plugins during boot #45100

Load configured Active Storage plugins during boot #45100

skipkayhil commented May 14, 2022

byroot left a comment

byroot May 15, 2022

byroot May 15, 2022

byroot May 15, 2022

byroot May 15, 2022

byroot May 15, 2022

byroot May 15, 2022

byroot commented Nov 16, 2024

zzak commented Jan 7, 2025

Earlopain Jan 7, 2025

Earlopain Jan 7, 2025

zzak Jan 7, 2025

ghiculescu Jan 30, 2025

zzak Jan 30, 2025

ghiculescu Jan 30, 2025

	unless ActiveStorage.supported_image_processing_methods.any? { \|method\| method_name == method }
	unless ActiveStorage.supported_image_processing_methods.include?(method_name)

	raise error unless error.message.include?("ruby-vips")
	raise error unless error.message.include?("mini_magick")

Load configured Active Storage plugins during boot #45100

Load configured Active Storage plugins during boot #45100

Conversation

skipkayhil commented May 14, 2022

Summary

Other Information

byroot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byroot commented Nov 16, 2024

zzak commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment