-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ActiveStorage adapter #3501
Add ActiveStorage adapter #3501
Conversation
core/app/models/concerns/spree/active_storage_adapter/attachment.rb
Outdated
Show resolved
Hide resolved
1dc97f6
to
c993687
Compare
c993687
to
1b99555
Compare
1b99555
to
a51eb58
Compare
a51eb58
to
4ee0f54
Compare
4ee0f54
to
3f84628
Compare
a693500
to
b1dfa37
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! ✨
So good to see someone pick up the slack! 😄
I commented with a few nits and a proposal for getting rid of the methods added on Spree::Core.
Also would like to propose to reorder the commits so that they're easier to follow while reviewing (e.g. putting all the preparatory code at the begining, new specs, helper methods, etc. and leaving the core of the PR for the last commits), just an idea to save some skipping back and forth for those that review commit-by-commit.
33b5c23
to
bb31261
Compare
bb31261
to
7bef4d6
Compare
7bef4d6
to
b458a0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing, thanks @filippoliverani. Just a couple of comments!
core/app/models/concerns/spree/active_storage_adapter/attachment.rb
Outdated
Show resolved
Hide resolved
b458a0e
to
3b6129b
Compare
To work seamlessly with the current code an adapter is needed to make ActiveStorage attachments behave exactly like Paperclip ones.
Allow to enable ActiveStorage attachments adapter only if Rails version is greater than 6.1.0.alpha. ActiveStorage Attachment adapter needs public blob URLs which has been introduced in rails/rails@94584c2.
ActiveStorage host is always required to make Disk service work correctly.
Skips a breaking model reload before NestedSet update depth logic. This issue has already been addressed in: collectiveidea/awesome_nested_set#413 The patch can be removed when a new version of awesome_nested_set will be released.
Paperclip is still the default storage adapter. To explicitly switch tests to ActiveStorage, ENABLE_ACTIVE_STORAGE env variable must be set.
3b6129b
to
0ac3904
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super awesome work @filippoliverani, thank you!
The adapter intends to make it possible to work with ActiveStorage as it was Paperclip. We had been using Paperclip until Rails 6.1 introduced ActiveStorage's public links. We moved to ActiveStorage as the default file attachment backend from that point on, but we tried to make its API compatible with Paperclip's one. See solidusio#3501 for details. When trying to translate [Paperclip's style definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494) (based on [ImageMagick](see https://legacy.imagemagick.org/Usage/thumbnails/)) to what ActiveStorage expects (which is [image_processing syntax](https://github.com/janko/image_processing), common for ImageMagick and libvips), we were transforming everything to a [`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit) option. However, that corresponds to the `>` symbol in ImageMagick, but not to other options that can be given. Whit this commit, we add support for defining other kinds of transformations in ActiveStorage. However, we're still missing more advanced options that can be given when using [standalone Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{ geometry: '100x100', format: :jpg }`) or [standalone ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods) (example `rotate: 90`). In the long run, we should aim to completely differentiate how definitions are given for each library.
The adapter intends to make it possible to work with ActiveStorage as it was Paperclip. We had been using Paperclip until Rails 6.1 introduced ActiveStorage's public links. We moved to ActiveStorage as the default file attachment backend from that point on, but we tried to make its API compatible with Paperclip's one. See #3501 for details. When trying to translate [Paperclip's style definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494) (based on [ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what ActiveStorage expects (which is [image_processing syntax](https://github.com/janko/image_processing), common for ImageMagick and libvips), we were transforming everything to a [`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit) option. However, that corresponds to the `>` symbol in ImageMagick, but not to other options that can be given. Whit this commit, we add support for defining other kinds of transformations in ActiveStorage. However, we're still missing more advanced options that can be given when using [standalone Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{ geometry: '100x100', format: :jpg }`) or [standalone ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods) (example `rotate: 90`). In the long run, we should aim to completely differentiate how definitions are given for each library.
The adapter intends to make it possible to work with ActiveStorage as it was Paperclip. We had been using Paperclip until Rails 6.1 introduced ActiveStorage's public links. We moved to ActiveStorage as the default file attachment backend from that point on, but we tried to make its API compatible with Paperclip's one. See #3501 for details. When trying to translate [Paperclip's style definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494) (based on [ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what ActiveStorage expects (which is [image_processing syntax](https://github.com/janko/image_processing), common for ImageMagick and libvips), we were transforming everything to a [`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit) option. However, that corresponds to the `>` symbol in ImageMagick, but not to other options that can be given. Whit this commit, we add support for defining other kinds of transformations in ActiveStorage. However, we're still missing more advanced options that can be given when using [standalone Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{ geometry: '100x100', format: :jpg }`) or [standalone ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods) (example `rotate: 90`). In the long run, we should aim to completely differentiate how definitions are given for each library.
The adapter intends to make it possible to work with ActiveStorage as it was Paperclip. We had been using Paperclip until Rails 6.1 introduced ActiveStorage's public links. We moved to ActiveStorage as the default file attachment backend from that point on, but we tried to make its API compatible with Paperclip's one. See #3501 for details. When trying to translate [Paperclip's style definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494) (based on [ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what ActiveStorage expects (which is [image_processing syntax](https://github.com/janko/image_processing), common for ImageMagick and libvips), we were transforming everything to a [`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit) option. However, that corresponds to the `>` symbol in ImageMagick, but not to other options that can be given. Whit this commit, we add support for defining other kinds of transformations in ActiveStorage. However, we're still missing more advanced options that can be given when using [standalone Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{ geometry: '100x100', format: :jpg }`) or [standalone ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods) (example `rotate: 90`). In the long run, we should aim to completely differentiate how definitions are given for each library.
The adapter intends to make it possible to work with ActiveStorage as it was Paperclip. We had been using Paperclip until Rails 6.1 introduced ActiveStorage's public links. We moved to ActiveStorage as the default file attachment backend from that point on, but we tried to make its API compatible with Paperclip's one. See #3501 for details. When trying to translate [Paperclip's style definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494) (based on [ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what ActiveStorage expects (which is [image_processing syntax](https://github.com/janko/image_processing), common for ImageMagick and libvips), we were transforming everything to a [`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit) option. However, that corresponds to the `>` symbol in ImageMagick, but not to other options that can be given. Whit this commit, we add support for defining other kinds of transformations in ActiveStorage. However, we're still missing more advanced options that can be given when using [standalone Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{ geometry: '100x100', format: :jpg }`) or [standalone ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods) (example `rotate: 90`). In the long run, we should aim to completely differentiate how definitions are given for each library.
The adapter intends to make it possible to work with ActiveStorage as it was Paperclip. We had been using Paperclip until Rails 6.1 introduced ActiveStorage's public links. We moved to ActiveStorage as the default file attachment backend from that point on, but we tried to make its API compatible with Paperclip's one. See #3501 for details. When trying to translate [Paperclip's style definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494) (based on [ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what ActiveStorage expects (which is [image_processing syntax](https://github.com/janko/image_processing), common for ImageMagick and libvips), we were transforming everything to a [`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit) option. However, that corresponds to the `>` symbol in ImageMagick, but not to other options that can be given. Whit this commit, we add support for defining other kinds of transformations in ActiveStorage. However, we're still missing more advanced options that can be given when using [standalone Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{ geometry: '100x100', format: :jpg }`) or [standalone ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods) (example `rotate: 90`). In the long run, we should aim to completely differentiate how definitions are given for each library.
The adapter intends to make it possible to work with ActiveStorage as it was Paperclip. We had been using Paperclip until Rails 6.1 introduced ActiveStorage's public links. We moved to ActiveStorage as the default file attachment backend from that point on, but we tried to make its API compatible with Paperclip's one. See solidusio#3501 for details. When trying to translate [Paperclip's style definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494) (based on [ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what ActiveStorage expects (which is [image_processing syntax](https://github.com/janko/image_processing), common for ImageMagick and libvips), we were transforming everything to a [`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit) option. However, that corresponds to the `>` symbol in ImageMagick, but not to other options that can be given. Whit this commit, we add support for defining other kinds of transformations in ActiveStorage. However, we're still missing more advanced options that can be given when using [standalone Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{ geometry: '100x100', format: :jpg }`) or [standalone ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods) (example `rotate: 90`). In the long run, we should aim to completely differentiate how definitions are given for each library.
The adapter intends to make it possible to work with ActiveStorage as it was Paperclip. We had been using Paperclip until Rails 6.1 introduced ActiveStorage's public links. We moved to ActiveStorage as the default file attachment backend from that point on, but we tried to make its API compatible with Paperclip's one. See solidusio#3501 for details. When trying to translate [Paperclip's style definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494) (based on [ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what ActiveStorage expects (which is [image_processing syntax](https://github.com/janko/image_processing), common for ImageMagick and libvips), we were transforming everything to a [`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit) option. However, that corresponds to the `>` symbol in ImageMagick, but not to other options that can be given. Whit this commit, we add support for defining other kinds of transformations in ActiveStorage. However, we're still missing more advanced options that can be given when using [standalone Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{ geometry: '100x100', format: :jpg }`) or [standalone ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods) (example `rotate: 90`). In the long run, we should aim to completely differentiate how definitions are given for each library.
The adapter intends to make it possible to work with ActiveStorage as it was Paperclip. We had been using Paperclip until Rails 6.1 introduced ActiveStorage's public links. We moved to ActiveStorage as the default file attachment backend from that point on, but we tried to make its API compatible with Paperclip's one. See solidusio#3501 for details. When trying to translate [Paperclip's style definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494) (based on [ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what ActiveStorage expects (which is [image_processing syntax](https://github.com/janko/image_processing), common for ImageMagick and libvips), we were transforming everything to a [`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit) option. However, that corresponds to the `>` symbol in ImageMagick, but not to other options that can be given. Whit this commit, we add support for defining other kinds of transformations in ActiveStorage. However, we're still missing more advanced options that can be given when using [standalone Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{ geometry: '100x100', format: :jpg }`) or [standalone ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods) (example `rotate: 90`). In the long run, we should aim to completely differentiate how definitions are given for each library.
The adapter intends to make it possible to work with ActiveStorage as it was Paperclip. We had been using Paperclip until Rails 6.1 introduced ActiveStorage's public links. We moved to ActiveStorage as the default file attachment backend from that point on, but we tried to make its API compatible with Paperclip's one. See solidusio#3501 for details. When trying to translate [Paperclip's style definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494) (based on [ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what ActiveStorage expects (which is [image_processing syntax](https://github.com/janko/image_processing), common for ImageMagick and libvips), we were transforming everything to a [`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit) option. However, that corresponds to the `>` symbol in ImageMagick, but not to other options that can be given. Whit this commit, we add support for defining other kinds of transformations in ActiveStorage. However, we're still missing more advanced options that can be given when using [standalone Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{ geometry: '100x100', format: :jpg }`) or [standalone ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods) (example `rotate: 90`). In the long run, we should aim to completely differentiate how definitions are given for each library.
Description
This PR stems from the discussion here #2725 and builds upon @elia's work here #2974 and here #3237 🙏 .
From Rails 6.1 ActiveStorage will support public blob URLs (rails/rails@94584c2) and Solidus should be ready to offer an ActiveStorage adapter at least for new stores.
This PR adds an experimental ActiveStorage support leveraging
config.image_attachment_module
andconfig.taxon_attachment_module
extension points. Solidus attachments heavily rely on Paperclip API, to avoid any change in existing code I tried to adapt ActiveStorage API to Paperclip attachment public API.To run tests using ActiveStorage,
ENABLE_ACTIVE_STORAGE
env var must be set and Rails version should be >=6.1.0.alpha
/master
.Checklist: