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

Add basename and fix extension value for fog file #2587

Conversation

leductienttkt
Copy link

About this PR

Make basename and extension of CarrierWave::SanitizedFile and CarrierWave::Storage::Fog::File the same

My code

  • carrierwave.rb
CarrierWave.configure do |config|
  case Rails.env
  when 'test', 'development'
    config.storage :file
    ...

  when 'production', 'staging'
    config.storage :fog
    ...
  end
end
  • models/document.rb
class Document < ApplicationRecord
  mount_uploader :file, DocumentFileUploader
  ...
end
  • What I want to show in view:
    document.file.file.extension
    document.file.file.basename

It works fine in development environment, but I have problem in production.
After checking, I got the result as below.
I think we shouldn't return different values in different environments for the same function.

Actual results

when file has complicated extensions , ex: complex.filename.tar.gz

  • config.storage :file

Ảnh chụp Màn hình 2021-09-11 lúc 19 48 09

Ảnh chụp Màn hình 2021-09-11 lúc 19 48 20

  • config.storage :fog

Ảnh chụp Màn hình 2021-09-11 lúc 19 42 32

Ảnh chụp Màn hình 2021-09-11 lúc 19 45 49

when file has simple extension, ex: simple.extension

Ảnh chụp Màn hình 2021-09-11 lúc 19 49 06

Ảnh chụp Màn hình 2021-09-11 lúc 19 49 14

  • config.storage :fog

Ảnh chụp Màn hình 2021-09-11 lúc 19 43 59

when file has no extension, ex: filename

  • config.storage :file

Ảnh chụp Màn hình 2021-09-11 lúc 19 49 33

Ảnh chụp Màn hình 2021-09-11 lúc 19 50 02

  • config.storage :fog

Ảnh chụp Màn hình 2021-09-11 lúc 19 44 32

Expected results

when file has complicated extensions , ex: complex.filename.tar.gz

  • basename: complex.filename
  • extension: tar.gz

when file has simple extension, ex: simple.extension

  • basename: simple
  • extension: extension

when file has no extension, ex: filename

  • basename: filename
  • extension: ""

@leductienttkt leductienttkt force-pushed the add_fog_file_basename_and_fix_extension_value branch from 93f8af4 to 59619c2 Compare October 11, 2021 09:11
Comment on lines 546 to 559
def split_extension(filename)
# regular expressions to try for identifying extensions
extension_matchers = [
/\A(.+)\.(tar\.([glx]?z|bz2))\z/, # matches "something.tar.gz"
/\A(.+)\.([^\.]+)\z/ # matches "something.jpg"
]

extension_matchers.each do |regexp|
if filename =~ regexp
return $1, $2
end
end
return filename, "" # In case we weren't able to split the extension
end
Copy link
Member

Choose a reason for hiding this comment

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

Can this logic extracted into a separate module like CarrierWave::Utilities::Filename and shared with CarrierWave::SanitizedFile 's, instead of just copying the implementation?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mshibuya
Thanks for review.
I fixed it!

@leductienttkt leductienttkt force-pushed the add_fog_file_basename_and_fix_extension_value branch from 59619c2 to 08296f2 Compare October 13, 2021 03:17

Verified

This commit was signed with the committer’s verified signature.
lukasmalkmus Lukas Malkmus
@leductienttkt leductienttkt force-pushed the add_fog_file_basename_and_fix_extension_value branch from 08296f2 to dff8f6f Compare October 13, 2021 03:26
@mshibuya mshibuya merged commit d6eccb4 into carrierwaveuploader:master Oct 13, 2021
@mshibuya
Copy link
Member

Awesome, thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants