rails / marcel

Find the mime type of files, examining file, filename and declared type
Apache License 2.0
387 stars 67 forks source link

Breaking change with 1.0.3 - removal of Marcel::TYPES #98

Closed toby-brilliant closed 9 months ago

toby-brilliant commented 9 months ago

CarrierWave (at least version 3.0.5) relied on Marcel::TYPES (see here)

It looks like Marcel::Types changed ages ago but was just released with 1.0.3. Was this constant intended to be public (breaking change in a patch release) or potentially is CarrierWave doing something it shouldn't?

Getting stack traces that look like:

     NameError:
       uninitialized constant Marcel::TYPES
     # ./app/models/attachment.rb:44:in `block in set_remote_url'
     # ./app/models/attachment.rb:40:in `set_remote_url'
jeremy commented 9 months ago

I'm afraid that's a private internal constant, and it's documented as such. Sorry about this!

fwolfst commented 9 months ago

@jeremy maybe you could give a tiny pointer how to approach a fix (e.g. is the data exposed similarly through some public interface?) Thanks.

jeremy commented 9 months ago

Sure, @fwolfst. Here's how to do that:

First, check git history for the commit that made this internal change to remove TYPES: https://github.com/rails/marcel/commit/e413daeffaed05d1e34b31e0ff1de95182d2d418

Then observe the new constants (TYPE_EXTS and TYPE_PARENTS) and refer back to CarrierWave to examine how TYPES had been used: https://github.com/carrierwaveuploader/carrierwave/blob/ed8799191824a4d2762eb1028c01220102699377/lib/carrierwave/downloader/remote_file.rb#L36-L38

Looks like CarrierWave was checking whether the type exists and then grabbing its file extension.

So let's check Marcel::Magic for file extension APIs. And there it is! https://github.com/rails/marcel/blob/36a94f24c2ecedc910fc484172c219d431447601/lib/marcel/magic.rb#L62-L65

Now update CarrierWave to use Marcel::Magic.new(content_type).extensions instead.

fwolfst commented 9 months ago

Sure, @fwolfst. Here's how to do that:

First, check git history for the commit that made this internal change to remove TYPES: e413dae

Then observe the new constants (TYPE_EXTS and TYPE_PARENTS) and refer back to CarrierWave to examine how TYPES had been used: https://github.com/carrierwaveuploader/carrierwave/blob/ed8799191824a4d2762eb1028c01220102699377/lib/carrierwave/downloader/remote_file.rb#L36-L38

Looks like CarrierWave was checking whether the type exists and then grabbing its file extension.

So let's check Marcel::Magic for file extension APIs. And there it is!

https://github.com/rails/marcel/blob/36a94f24c2ecedc910fc484172c219d431447601/lib/marcel/magic.rb#L62-L65

Now update CarrierWave to use Marcel::Magic.new(content_type).extensions instead.

Very kind, @jeremy . Thanks a lot!