Skip to content

Check mime types in custom asset validation rules#3290

Merged
jasonvarga merged 13 commits intostatamic:masterfrom
arthurperton:pr/issue-3277-asset-mimetypes
Mar 10, 2021
Merged

Check mime types in custom asset validation rules#3290
jasonvarga merged 13 commits intostatamic:masterfrom
arthurperton:pr/issue-3277-asset-mimetypes

Conversation

@arthurperton
Copy link
Copy Markdown
Contributor

@arthurperton arthurperton commented Feb 22, 2021

Fixes #3277.

This PR is written on top of #3280 which provides a way to see if a key is missing, and regenerate the meta when needed.

Mimicking Laravel validation rules
The new asset validation rules in this PR mimic the behaviour of the original Laravel rules. The Laravel code for all this magic can be found in Illuminate\Validation\Concerns\ValidatesAttributes.

Laravel actually has two validation rules to check a file's mime type. One is called mimes and, contrary to what you might expect, you should provide a list of extensions rather than mime types, like so: mimes:jpg,png. It doesn't use the actual file extension, instead it guesses the extension based on the mime type of the file.

People found it confusing that you had to provide a list of extensions to this rule. So later a second rule was added called mimetypes. This rule actually takes a list of mime types, e.g. mimetypes:image/jpg,image/png.

Using the guessed extension elsewhere too
I wonder, should the Asset class use the guessed extension too now for all the extension-based tests like isImage() and isVideo()?

Let's say you rename your image.jpg to image.rad and you upload it to an asset field that expects an image. Meaning it has an image validator or something like mimes:jpg,png. The new validators will be perfectly fine with your renamed file, because the guessed extension will still be jpg. But Asset.isImage() would currently return false, because it looks at the actual file extension, which is now rad. This is inconsistent behaviour.

@arthurperton
Copy link
Copy Markdown
Contributor Author

Hmmm getting some unit test weirdness.. They run fine locally 😕

@arthurperton arthurperton changed the title Custom asset validation rules check mime types Check mime types in custom asset validation rules Feb 23, 2021
@arthurperton arthurperton requested a review from jasonvarga March 5, 2021 09:48
@jasonvarga
Copy link
Copy Markdown
Member

I know sometimes you see me merge PRs and you pounce on a new one. Just letting you know I'm working on this one next, so don't worry about fixing anything here yet 😄

@arthurperton
Copy link
Copy Markdown
Contributor Author

Haha sure, I will leave this one alone 🐅 😉
Very happy to hear you're working on this one!

@jasonvarga jasonvarga marked this pull request as ready for review March 10, 2021 16:24
@jasonvarga jasonvarga changed the base branch from 3.0 to master March 10, 2021 16:24
@jasonvarga
Copy link
Copy Markdown
Member

I've merged master into this and made a few tweaks. Thanks for this.

I wonder, should the Asset class use the guessed extension too now for all the extension-based tests like isImage() and isVideo()?

I agree that those methods could be updated to use mimetypes, but I'd to an actual mimetype check instead of comparing the guessed extensions. e.g. isImage could check that the mime type starts with image/. Separate PR though.

@jasonvarga jasonvarga merged commit 15e6d5d into statamic:master Mar 10, 2021
@arthurperton
Copy link
Copy Markdown
Contributor Author

Nice! 🎉

those methods could be updated to use mimetypes, but I'd to an actual mimetype check instead of comparing the guessed extensions.

Yeah I agree, that's better, more direct.

@arthurperton arthurperton deleted the pr/issue-3277-asset-mimetypes branch September 9, 2022 12:20
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.

Custom asset validation rules should check mime types

2 participants