-
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
Added errors on file size #1026
Added errors on file size #1026
Conversation
@jnicklas does this have any chance to be merged? If does, i can work to close this issue |
@bensie, what do you think about this one? On the one hand, file size validators are a lot more effective on the client side, in javascript, where they can actually prevent the upload from happening. But on the other hand it could be nice to easily guarantee nobody's dumping huge files on you. |
I definitely think there is a place for it on the server-side, particularly when file uploads are permitted via an API. I'm 👍 on this. |
# are allowed to be uploaded. | ||
# === Returns | ||
# | ||
# [NilClass, Range] a size range which are permitted to be uploaded |
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.
In kilobytes? Note units.
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 think that here it should be in kilobytes, but it should be converted when showing the errors messages
ps: maybe i misunderstood your question
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 agree with the conversion for error messages also. The more I think about it the more I'm into this pull, btw.
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.
👍
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 realize this is 10 years old but this PR is linked from the changelog, and it's not clearly documented in https://github.com/carrierwaveuploader/carrierwave/blob/master/lib/carrierwave/uploader/file_size.rb so I felt like it would be helpful to comment here.
It feels to me like if it's comparing to new_file.size
isn't it comparing bytes? When I set a size_range
the validation messages convert to human friendly units and it does seem like it's using bytes. I set the limit to 1024 and it allowed a file smaller than 1KB and when I tried a 4KB file it rejected it, with the error message "Validation failed: File File size should be less than 1 KB". (It does seem to go for the closest human-friendly unit, so if the limit is in MB or GB it reflects that.) So I think that the value here is in fact interpreted as bytes.
Submitted a tiny PR to update the doc comment here: #2662
ah yeah this is implicitly documented in #2199, but it would be nice to make it explicit.
Hello, any chance this gets merged? Are there any blockers? |
Admittedly, it's been ages since I've looked into the internals in CarrierWave, but it used to be possible to use |
Easier, but not safe, as it would happen after processing images. |
👍 just needs the modifications that @taavo suggested |
Similar to extension_white_list and extension_black_list, have added size_range.