Skip to content

Conversation

@Earlopain
Copy link
Contributor

Motivation / Background

Since 93c00a8, 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 does 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 e978ef0

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

Since rails@93c00a8,
`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 rails@e978ef0
Copy link
Member

@zzak zzak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, and sorry for missing this.

Just to confirm, the issue was simply:

  • Image analyzers were made optional, based on the variant processor in:
  • When we added them all back, we forgot the add accept? in:

@rafaelfranca rafaelfranca merged commit 8106838 into rails:main Sep 22, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants