Closed swissspidy closed 2 years ago
I wonder if we should have one object with all this data. Something like this
const fileTypes = {
images: {
jpg: 'image/jpeg',
jpeg: 'image/jpeg',
png: 'image/png',
},
videos: {
mp4: 'video/mp4',
},
audio: { ...}
}
So to get allowedImageFileTypes
or allowedImageMimeTypes
. You would do this.
const allowedImageFileTypes = Object.keys( fileTypes.images );
const allowedImageMimeTypes = Object.values( fileTypes.images );
There is also a difference between allow images types and list images that can be uploaded. One is used for allow images for posters. How would be best denote that?
Including file types in there is redundant and makes it harder to maintain for us and everyone trying to build an editor. We can operate with just mime types and then use getExtensionFromMimeType
on the client-side.
There is also a difference between allow images types and list images that can be uploaded. One is used for allow images for posters. How would be best denote that?
The suggestion above is to rename the existing fields to make that more clear.
We can operate with just mime types and then use getExtensionFromMimeType on the client-side.
Do we even need allowedFileTypes
, allowedAudioFileTypes
and allowedImageFileTypes
then? Don't we just need the mime types? Meaning this could become.
const fileTypes = {
images: ['image/jpeg', 'image/png',],
videos: ['video/mp4', 'video/webm'],
audio: [ ... ]
}
So to get allowedImageFileTypes
or allowedImageMimeTypes
. You would do this.
const allowedImageMimeTypes = fileTypes.images;
const allowedImageFileTypes = allowedImageMimeTypes.map( ( mimeType ) => getExtensionFromMimeType( mimeType )) ;
Precisely. That's what I meant in the ticket description by saying we can make it obsolete.
There might be a problem with passing down mime types and getting an extension array in the case of image/jpeg
since both .jpeg
and .jpg
have the mime-type image/jpeg
.
if allowedImageMimeTypes
is ['image/jpeg']
allowedImageFileTypes
would be ['jpeg']
but MediaUpload
would accept both jpeg
and jpg
files.
Thanks for flagging that. Agreed that we need to keep that in mind.
As far as I know we only use the file extensions to display user-facing messages like You can upload jpeg, jpg, png, mp4
. For MediaUpload
and others, the checks are based on the mime type.
If the user-facing message now says You can upload jpeg, png, mp4
(omitting jpg
and FWIW also jpe
), that is OK.
There is a similar argument for mp4/m4v files as well. Also what if developers add a custom file type, like videopress or a new file format. It may not be part of the mine type package.
I'm not concerned about invalid mime types, we don't really support that.
My orignal suggestion https://github.com/googleforcreators/web-stories-wp/issues/10592#issuecomment-1046843486 would handle this issue.
As you can see in my sudo code shows, that structure can support multiple file extensions with the same mime types.
Not catering for mime types with multiple corresponding file extensions is fine, because file extension info is only used for error messages.
Again, I want us to avoid having to pass file extensions via config. It's redundant. So let's try it first without. If it doesn't work for some reason we can always revisit.
And something like video/videopress
is not supported anyway because it's an invalid mime type.
I have a POC PR up here https://github.com/GoogleForCreators/web-stories-wp/pull/10709.
I noticed a problem. The getExtensionFromMimeType
function doesn't work as expected. Some functions return odd file extensions or nothing at all.
Take this example. audio/mpeg, returns mpga, not mp3 as expected. audio/acc returns null and not acc file.
This is an issue with the mimetype library being used. This will have to be fixed or replaces. Can you advice @swissspidy
It seems to work as expected, although it's definitely confusing at first.
This is related to @ayushnirwal's comment above.
audio/mpeg
maps to multiple file extensions: mpga,mp2,mp2a,mp3,m2a,m3a. You're getting the first one.
The mime package doesn't offer a way to get all of them, see https://github.com/broofa/mime/issues/254
audio/aac
returns nothing because it has no extension defined in mime-db but audio/x-aac
does, see https://github.com/jshttp/mime-db/blob/ebb6bf92ea39d3168a50942295d5edfdcdce641a/db.json#L6367-L6369 and https://github.com/jshttp/mime-db/issues/81
It's not a huge deal because again, this is only used for error messages.
To make it less confusing it should be fine to have a hardcoded list of mime types for these cases
See #10718 for my suggestion.
Task Description
We currently pass this list of mime/file types to the editor:
https://github.com/GoogleForCreators/web-stories-wp/blob/f130e0cfa2c69ddd07201db7c1b74d58b24bf341/includes/Admin/Editor.php#L363-L369
As we are working on some documentation on using the decoupled story editor, it became more obvious how confusing this config is.
allowedMimeTypes
: map of allowed mime types for actual story elements. For example,allowedMimeTypes.image
might containimage/svg+xml
or something to say that you can drop SVGs on the canvasallowedImageMimeTypes
: this is used for places in the design/document tab where you can upload images (video poster, story poster, publisher logo). For example, here we would never want to allow uploading SVGs. Hence this separate config.allowedAudioMimeTypes
: similarly, this is used for when you want to add background audio to a page.In addition to that,
allowedFileTypes
is just a simple array of file types so that in the editor we can say “You are only allowed to upload jpeg, png, gif” etc. We can just get rid of this and usegetExtensionFromMimeType
from themedia
package instead.allowedTranscodableMimeTypes
is really pointless too, which is why I opened #10591 for that already.To-do
allowedMimeTypes
to somehting more obvious, like for exampleallowedElementMimeTypes
allowedImageMimeTypes
andallowedAudioMimeTypes
to something more obvious tooallowedFileTypes
obsolete by assembling that on-the-fly in the editor instead, usinggetExtensionFromMimeType