-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Make cache storage pluggable #1312
Make cache storage pluggable #1312
Conversation
Anyone here? |
Thanks for this. I'll review it as soon as I have some time, but it's a lot of changes so it may take awhile. In the meantime, please remove the change to the gemspec -- that has been addressed separately. |
Thanks for reply, I've rebased it to the master. |
@@ -17,6 +18,10 @@ def configure(&block) | |||
def clean_cached_files!(seconds=60*60*24) | |||
CarrierWave::Uploader::Base.clean_cached_files!(seconds) | |||
end | |||
|
|||
def tmp_path | |||
@tmp_path ||= File.expand_path(File.join('..', 'tmp'), root) |
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.
What assumptions are being made here? What if there isn't a tmp
directory in the working dir?
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.
All of supported web application frameworks(Merb, Rails, Sinatra, Padrino) has /tmp
directory at the same level with /public
, so we can safely assume that ../tmp
of CarrierWave.root(=/public
) as default location of tmp directory.
If that's not the case or someone wants to use tmp directory other than the framework's default, he or she can specifically set tmp_path such as CarrierWave.tmp_path = '/path/to/tmp'
in app's initializer or somewhere around.
…url contains '/' in query string
Wow, when this issue could be merged to master? we really need it. |
@bensie Any other questions? :) |
So CI Passed, then will be merged then? |
@bensie |
# | ||
def cache_storage(storage = nil) | ||
if storage | ||
self._cache_storage = storage.is_a?(Symbol) ? eval(storage_engines[storage]) : 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.
What is a sample value of storage_engines[storage]
here and why must it be eval'd?
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.
I've just made it the same manner as storage configuration, and the value of storage_engines[storage]
can be either "CarrierWave::Storage::File"
or "CarrierWave::Storage::Fog"
by default.
Tracing back this line revealed that this change was introduced by d1c4156, which states that the purpose is to prevent unnecessary autoloading of classes.
Awesome, thanks for your work on this! |
Thank you for merging this in! |
See carrierwaveuploader/carrierwave#1312 for more information.
See carrierwaveuploader/carrierwave#1312 for more information.
See carrierwaveuploader/carrierwave#1312 for more information.
Refs #461
This pull request introduces abstraction to cache storage strategy and makes it selectable via configuration, thus making it safe to use CarrierWave in a multi-application-server environment(e.g. multiple dyno configuration in Heroku).
You can enable this feature either globally:
or per-uploader basis:
Any suggestions will be greatly appreciated.
Thanks for publishing and maintaining this awesome gem!!