Skip to content

[Validator] Use Mime component to determine mime type for file validator #36868

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 30, 2020
Merged

[Validator] Use Mime component to determine mime type for file validator #36868

merged 1 commit into from
May 30, 2020

Conversation

pierredup
Copy link
Contributor

@pierredup pierredup commented May 19, 2020 •
edited
Loading

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets N/A
License MIT
Doc PR N/A

When validating the mime type for a file, the Validator component relies on the Symfony\Component\HttpFoundation\File\File class, but if the HttpFoundation component is not installed, then you just get the error

PHP Fatal error:  Uncaught Error: Class 'Symfony\Component\HttpFoundation\File\File' not found

This PR uses the Mime component to get the mime type for a file and throws an exception if the Mime component is not installed.

@pierredup pierredup changed the title Throw exception when validating file mime types and the HttpFoundation component is not installed [Validator] Throw exception when validating file mime types and the HttpFoundation component is not installed May 19, 2020
@nicolas-grekas
Copy link
Member

nicolas-grekas commented May 19, 2020 •
edited
Loading

Now that the Mime component is standalone, can't we skip the dependency on HttpFoundation instead?

@pierredup pierredup force-pushed the validator-error-message branch from cdc74bf to 4c06e60 Compare May 19, 2020 09:55
@pierredup pierredup changed the title [Validator] Throw exception when validating file mime types and the HttpFoundation component is not installed [Validator] Use Mime component to determine mime type for file validator May 19, 2020
@pierredup pierredup force-pushed the validator-error-message branch from 4c06e60 to df554cd Compare May 19, 2020 09:58
@pierredup
Copy link
Contributor Author

@nicolas-grekas Done

@nicolas-grekas
Copy link
Member

What about considering it as a bugfix?

@pierredup pierredup force-pushed the validator-error-message branch from cf33702 to 3577b28 Compare May 19, 2020 10:35
@pierredup
Copy link
Contributor Author

What about considering it as a bugfix?

I wasn't sure if it counted as a bugfix or new feature. When in doubt, I just consider something a new feature :)

But I'm not sure if a bugfix against 3.4 can use the Mime component, so should I create a new PR for 3.4 and have the original exception message for HttpFoundation, then just rebase and retarget 4.4 here?

@nicolas-grekas
Copy link
Member

I would suggest targeting 4.4 and forget about 3.4.

@pierredup pierredup force-pushed the validator-error-message branch from 3577b28 to 62ade6a Compare May 19, 2020 10:41
@pierredup pierredup changed the base branch from master to 4.4 May 19, 2020 10:42
@nicolas-grekas nicolas-grekas added this to the 4.4 milestone May 22, 2020
@pierredup pierredup force-pushed the validator-error-message branch 4 times, most recently from 84357d4 to 05416a2 Compare May 29, 2020 10:57
@pierredup
Copy link
Contributor Author

Status: Ready for review

@nicolas-grekas nicolas-grekas force-pushed the validator-error-message branch from 05416a2 to 2a720eb Compare May 29, 2020 11:34
@nicolas-grekas
Copy link
Member

I pushed a CS change. The code looks good, but the tests need some love, see failures on appveyor when the finfo extension is not enabled.

@pierredup pierredup force-pushed the validator-error-message branch 2 times, most recently from 35fc85e to 09dc302 Compare May 29, 2020 12:09
@pierredup pierredup force-pushed the validator-error-message branch from 09dc302 to 4728833 Compare May 29, 2020 12:11
@pierredup
Copy link
Contributor Author

pierredup commented May 29, 2020 •
edited
Loading

I reverted the changes to the tests to use mocks again, and just changed the ordering of the checks

@fabpot
Copy link
Member

fabpot commented May 30, 2020

Thank you @pierredup.

@fabpot fabpot merged commit 5e0e9a8 into symfony:4.4 May 30, 2020
This was referenced May 31, 2020
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@fabpot fabpot fabpot approved these changes

@xabbuh xabbuh xabbuh approved these changes

Assignees
No one assigned
Labels
Bug Status: Reviewed Validator
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
5 participants
@pierredup @nicolas-grekas @fabpot @xabbuh @carsonbot