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
Allow ActiveStorage.default_url_options for DiskService #42847
Conversation
ab78bdd
to
58b84ae
Compare
58b84ae
to
79a28a3
Compare
4663668
to
9f13748
Compare
9f13748
to
1bb7ca6
Compare
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
@@ -160,7 +160,7 @@ def url_helpers | |||
end | |||
|
|||
def url_options | |||
ActiveStorage::Current.url_options | |||
ActiveStorage::Current.url_options || Rails.application.routes.default_url_options |
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.
Active Storage should not depend on the Rails application. Maybe we should inject it into the DiskService using the Engine in Active Storage?
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.
3b0556e, It is what you mean? Let me know what you think.
DiskService can use ActiveStorage::Current.url_options or Rails.application.routes.default_url_options (in that order) as url options.
fb1d2e4
to
2b9bdcc
Compare
….default_url_options
2b9bdcc
to
4299ae2
Compare
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Summary
DiskService
can useActiveStorage::Current.url_options
orActiveStorage.default_url_options
(in that order) as url options.Related to: #40855 #37841 #33549 #32590 #13372
Why
Setting
ActiveStorage::Current.host
orActiveStorage::Current.url_options
is useful when generate the URLs fromActionController
. But if you need to generate the URLs in a background job or rake task, it seems cumbersome to have to define it in another place. Also for my experience can be a bit confusing to other developers.I think that we can use
ActiveStorage::Current.url_options || ActiveStorage.default_url_options
as source forurl_options
. This approach allows override globalActiveStorage.default_url_options
withActiveStorage::SetCurrent
if needed.Other Information
If the PR receives positive feedback, I will update the guidelines in this or another PR.