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

Only default to activestorage adapter if Rails version is supported #4563

Merged
merged 1 commit into from
Sep 5, 2022

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented Sep 5, 2022

Summary

We only support the activestorage adapter with Rails 6.1 and above,
but we default to it, even if the Rails version is 6.0 and below.
Since we still support Rails 5.2 and 6.0, we cannot default to
the active storage adapter. The app won't boot unless we also
change the adapter in the spree initializer.

If the initializer, though, is nested in a config.to_prepare hook
(as recommended from Rails new autoloader zeitwerk [1]) the app
refuses to start anyhow, because the config is then taken from
the app_configuration defaults.

1

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

  • I have written a thorough PR description.
  • I have kept my commits small and atomic.
  • I have used clear, explanatory commit messages.

@kennyadsl
Copy link
Member

Hey @tvdeyen. Can you help me understand from where this is coming? New stores won't start on Rails 6.0 and if I really correctly there were a deprecation message telling people to explicitly set that preference to Paperclip for existing stores.

Just to understand if there's something I am missing, but of course the changes make sense!

@waiting-for-dev
Copy link
Contributor

It makes sense, although I neither understand how it was a problem in new stores with the latest Rails. BTW, what about using https://github.com/solidusio/solidus/blob/master/core/lib/spree/rails_compatibility.rb to return the value? Having all the stuff that depends on Rails versions encapsulated will help to deprecate things while we move forward.

@tvdeyen
Copy link
Member Author

tvdeyen commented Sep 5, 2022

@kennyadsl sure.

This is a Rails 6.0 application, running Solidus 3.0.

The initializer is set up like this:

Rails.application.config.to_prepare do
  Spree.config do |config|
    [..]
    config.image_attachment_module = "Spree::Image::PaperclipAttachment"
    config.taxon_attachment_module = "Spree::Taxon::PaperclipAttachment"
  end
end

Trying to install the latest migrations via:

bin/rake railties:install:migrations

blows up with

Configuration Error: Solidus ActiveStorage attachment adpater requires Rails >= 6.1.0.

Spree::Config.image_attachment_module preference is set to Spree::Image::ActiveStorageAttachment
Spree::Config.taxon_attachment_module preference is set to Spree::Taxon::ActiveStorageAttachment
Rails version is 6.0.5.1

To solve the problem you can upgrade to a Rails version greater than or equal to 6.1.0
or use legacy Paperclip attachment adapter by editing `config/initialiers/spree/rb`:
config.image_attachment_module = 'Spree::Image::PaperclipAttachment'
config.taxon_attachment_module = 'Spree::Taxon::PaperclipAttachment'

New stores won't start on Rails 6.0

What do you mean by that? We still support rails 5.2 and 6.0 in our Gemspec.

@tvdeyen
Copy link
Member Author

tvdeyen commented Sep 5, 2022

It makes sense, although I neither understand how it was a problem in new stores with the latest Rails. BTW, what about using https://github.com/solidusio/solidus/blob/master/core/lib/spree/rails_compatibility.rb to return the value? Having all the stuff that depends on Rails versions encapsulated will help to deprecate things while we move forward.

@waiting-for-dev what do you mean by this comment? Do you agree with the change and I should try using that module instead?

@waiting-for-dev
Copy link
Contributor

@waiting-for-dev what do you mean by this comment? Do you agree with the change and I should try using that module instead?

That's it 🙂

@tvdeyen
Copy link
Member Author

tvdeyen commented Sep 5, 2022

@waiting-for-dev thanks. Unfortunately the module is not available in Solidus 3.0 and this bug exists in Solidus 3.0 and above. Any ideas?

@waiting-for-dev
Copy link
Contributor

@waiting-for-dev thanks. Unfortunately the module is not available in Solidus 3.0 and this bug exists in Solidus 3.0 and above. Any ideas?

Good point. What about using it only where it's available?

@tvdeyen tvdeyen force-pushed the active-storage-rails-6.1 branch from 2a93666 to 6e2fa1d Compare September 5, 2022 09:50
@kennyadsl
Copy link
Member

I didn't get that the issue was there when the migration runs, no matter what preference are you using. That's definitely a bug that we should address soon. Thanks!

@tvdeyen tvdeyen force-pushed the active-storage-rails-6.1 branch from 6e2fa1d to 3caad8a Compare September 5, 2022 09:52
@tvdeyen
Copy link
Member Author

tvdeyen commented Sep 5, 2022

@waiting-for-dev thanks. Unfortunately the module is not available in Solidus 3.0 and this bug exists in Solidus 3.0 and above. Any ideas?

Good point. What about using it only where it's available?

@waiting-for-dev done for master and v3.2

@tvdeyen tvdeyen force-pushed the active-storage-rails-6.1 branch from 3caad8a to 1d86872 Compare September 5, 2022 10:02
We only support the activestorage adapter with Rails 6.1 and above,
but we default to it, even if the Rails version is 6.0 and below.
Since we still support Rails 5.2 and 6.0, we cannot default to
the active storage adapter. The app won't boot unless we also
change the adapter in the spree initializer.

If the initializer, though, is nested in a `config.to_prepare` hook
(as recommended from Rails new autoloader zeitwerk [1]) the app
refuses to start anyhow, because the config is then taken from
the `app_configuration` defaults.

[1](https://guides.rubyonrails.org/autoloading_and_reloading_constants.html#use-case-1-during-boot-load-reloadable-code)
@tvdeyen tvdeyen force-pushed the active-storage-rails-6.1 branch from 1d86872 to 8820577 Compare September 5, 2022 10:32
@waiting-for-dev waiting-for-dev added the type:bug Error, flaw or fault label Sep 5, 2022
Copy link
Contributor

@waiting-for-dev waiting-for-dev left a comment

Choose a reason for hiding this comment

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

Thanks!

@tvdeyen tvdeyen merged commit 5ece9ac into solidusio:master Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_core Changes to the solidus_core gem type:bug Error, flaw or fault
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants