Skip to content
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

Use Active Storage for Picture and File attachments #2544

Closed
wants to merge 29 commits into from

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented Aug 2, 2023

What is this pull request for?

Reworks #2288

Use ActiveStorage instead of Dragonfly for picture and file attachments.

Important changes

  • Cropping animated images (gif and webp) does not work with vips. It works with mini_magick though
  • Upfront processing of image variants after upload is not supported in Rails 7.0 (Need Rails 7.1 to re-implement support)
  • Passing additional params to the picture url is not supported anymore

TODO

  • Use ActiveStorage for Alchemy::Attachment as well
  • Remove Dragonfly dependency and related code
  • Remove our custom picture variants and accompany files
  • Make displaying animated GIFs work again
  • Write upgrade task to create ActiveStorage variants from our variants
  • Fix image cropping with mini_magick
  • Figure out how to persist meta data. Active Storage uses a background job to analyze and offers no callbacks
  • Add custom pictures controller to handle authentication and use proxy instead of redirect
  • Make CDN env vars work so that S3 images get delivered by cloudfront.
  • Figure out how to configure alchemy active storage service for different envs

Checklist

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have added tests to cover this change

@tvdeyen tvdeyen self-assigned this Aug 2, 2023
@tvdeyen tvdeyen added this to the 7.1 milestone Aug 2, 2023
@tvdeyen tvdeyen force-pushed the active-storage branch 2 times, most recently from 7fc3de3 to 9e46ec4 Compare August 2, 2023 15:19
Copy link

This pull request has not seen any activiy in a long time.
Probably because of missing tests or a necessary rebase.
This PR will be closed in 7 days if no further activity happens.

@github-actions github-actions bot added the Stale label Nov 29, 2023
@tvdeyen tvdeyen removed this from the 7.1 milestone Dec 28, 2023
@github-actions github-actions bot removed the Stale label Dec 29, 2023
@tvdeyen tvdeyen added this to the 7.2 milestone Jan 24, 2024
Necessary for active storage image transforms
ActiveStorage uses the image_processing gem to process images. This uses a hash to describe the conversion instead of a string
Instead of using the Dragonfly Model validations we write our own. That way we can use another image attachment library more easily
The vips image_processing processor sharpens images by default. This disables this behavior.
Active storage already knows if a file is convertible
This avoids N+1 in the image library
Using activestorage methods to read the values.
Ideally we would cache these values, but active
storage asynchronously sets these values and provides
no callback we could use.
Now that we use ActiveStorage we can remove our custom picture variant handling
This method is the only one left after the PictureVariant and PictureThumb removal.
Active storage has the mime type stored not the extension.
ruby-vips and mini_magick need to be told to ignore
the page in order to resize animated gifs.
Before we where not telling active storage that we want
a certain format explicitely.
We need that for displaying animated PNGs properly.
Proxy downloads the file.
Rails only adds it in Rails 7.1
@tvdeyen tvdeyen removed this from the 7.2 milestone Mar 11, 2024
Copy link

This pull request has not seen any activiy in a long time.
Probably because of missing tests or a necessary rebase.
This PR will be closed in 7 days if no further activity happens.

@github-actions github-actions bot added the Stale label May 11, 2024
Copy link

This pull request has not seen any activiy in a long time.
Probably because of missing tests or a necessary rebase.
Please open a new PR to latest main if you want to continue working on this.
Thanks for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant