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

Improve content type detector #74

Closed
wants to merge 2 commits into from
Closed

Improve content type detector #74

wants to merge 2 commits into from

Conversation

ngan
Copy link

@ngan ngan commented Dec 30, 2021

Improve content type detector by getting all types back from Marcel and cross-referencing them with the possible types (file extension). This is an improvement because we don't want to trust Marcel to order their magic detection appropriately.

For example, I have a PDF that contains the string wmv2 and Marcel is reporting that it's a video/x-ms-wmv file: https://github.com/rails/marcel/blob/main/lib/marcel/tables.rb#L2164. This is obviously wrong. If we were to use Marcel::Magic.all_by_magic then application/pdf is among the list of types returned. If Marcel considers it to be a possible PDF and the extension declares it as a pdf, let's just go with that.

ngan added 2 commits December 30, 2021 12:19
Improve content type detector by getting all types back from Marcel and cross-referencing them with the possible types (file extension). This is an improvement because we don't want to trust Marcel to order their magic detection appropriately.
@ssinghi ssinghi requested a review from sbhawsingka January 3, 2022 06:36
@ssinghi
Copy link

ssinghi commented Jan 8, 2022

Hi @ngan @sbhawsingka what do you think of #75
I think it will be better to delegate to Marcel for file content type detection completely.

@ssinghi
Copy link

ssinghi commented Jan 11, 2022

Closing in favor of #75

@ssinghi ssinghi closed this Jan 11, 2022
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.

3 participants