Add Apple mimetype files and extensions to accepted mimetypes

[argiepiano]

Currently, mimetypes for Apple files numbers, keynote and pages are not included in the list of available mimetypes in Backdrop. You can see the current list of available mimetypes by editing any File Type (e.g. admin/structure/file-types/manage/document) and opening the fieldset.

I feel Apple files have been around for a long time and are widely used, so I think it's a good idea to add them.

I'm proposing:

1. Adding those mimetypes and extensions to Backdrop in file_default_mimetype_mapping()
2. Adding those mimetypes to the mimetypes accepted by default by the document file types at admin/structure/file-types/manage/document

PR comming.

[argiepiano]

PR https://github.com/backdrop/backdrop/pull/4845 ready for review.

[klonos]

@argiepiano you beat me to it 👍🏼 ...I did some research, and came across https://stackoverflow.com/questions/1454777/apple-iwork-mime-types which has some comments indicating that we should also accept the preliminary/initial mimetypes that were assigned to Apple iWork documents before the proper IANA mimetypes were assigned. So should we add the following to the PR as well?:

• keynote: application/x-iwork-keynote-sffkey
• pages: application/x-iwork-pages-sffpages
• numbers: application/x-iwork-numbers-sffnumbers

[argiepiano]

@klonos, those mimetypes are very old. The new ones have been around since 2018. Do we really want to add mimetypes that are not in use anymore?

[klonos]

@argiepiano to answer that I would need to understand better how the detection of the mimetypes works. Is it up to the apache/nginx version, the PHP version, or the browser? If any of these factors base their detection on the older types, then I guess better to add them just to be safe. Right?

[argiepiano]

@klonos to answer your question: Backdrop determines the filemime by looking at the extension of the file. The server or the browser are not involved. This is determined in BackdropLocalStreamWrapper::getMimeType(), which is invoked by file_get_mimetype().

So, it doesn't make sense to keep using a deprecated mimetype, since files with extensions numbers, pages and keynote will be mapped by Backdrop to the current mimetype for the file extension.

[argiepiano]

Can someone review and test this PR? This would add these Apple mimetype extensions to known mimetypes in Backdrop, solving issues like https://github.com/backdrop/backdrop-issues/issues/6674

[herbdool]

Code looks good. I can't test unless I borrow an iPad. We'll see if someone beats me to it.

[argiepiano]

You can test using a Mac - all of them typically come with Numbers, Pages and Keynote.

To test:

1. Go to admin/content/files and click + Add file
2. Try to upload a Mac file. You'll get The selected file XXX cannot be uploaded. Only files with the following extensions are allowed: jpg, jpeg, gif, png, txt, doc, docx, xls, xlsx, pdf, ppt, pptx, pps, ppsx, odt, ods, odp, mp3, mov, mp4, m4a, m4v, mpeg, avi, ogg, oga, ogv, weba, webp, webm.
3. Apply the patch
4. Run update.php
5. Head to admin/structure/file-types/settings and add those files extensions to the Default allowed file extensions. (~~QUESTION: should the update hook add those to those default? I think so, in which case I need to add that~~. EDIT: I've added those three extensions to the Default allowed file extensions.)
6. Go to admin/content/files and click + Add file. Upload one of those files
7. The file should be included in the list of files, as Document file type

[argiepiano]

I've added those 3 extensions to the default allowed extensions, both in the update hook, and also in the config file.

[argiepiano]

Note: If you are testing and you have previously uploaded numbers, pages or keynote files, you will need to run admin/structure/file-types/classify to reclassify those files.

[argiepiano]

Just in case someone has added numbers, pages or keynote files in the past (through a file field, for example, which allows one to set the allowed extensions), they will need to reclassify those files. So, I set the flag to TRUE, which triggers the red dot alert to reclassify files. After reclassifying, they should see those files listed as Documents.

[herbdool]

I've tested with your sample numbers file. It works for me in Tugboat.

[argiepiano]

There is some concern expressed in Zulip about adding those file extensions to default_allowed_extensions (especially for existing site). Just to clarify that setting only affects the allowed extensions in file/add. It doesn't affect File reference fields at all. In fact the only default allowed extension for a File reference field is txt. You have to manually add any other extensions there, and the PR doesn't touch that.

However, I can see this is still concerning, and I'll remove that part of the PR. So, those testing the PR will need to head to admin/structure/file-types/settings and add those extensions there in order to be able to upload any of those 3 file types through file/add.

[argiepiano]

I have removed that part of the PR.

The PR sets the flag for "need file reclassification" which results in a red dot alert. Question: is this a good idea? Adding these mimetypes to Backdrop requires a file re-classification, for those who have in the past used File fields to upload these 3 mimetypes. Before this PR, those files will have been classified as application/octet-stream and file type undefined by Backdrop, which makes it impossible to use some of the File field widgets (they are not Documents, nor Images, etc). After the PR, any managed_file with these extensions will have its own mimetype, and will be re-classified as document after manually running the reclassification process through the red-dot alert.

However, it's unlikely that we have many Backdrop sites with this issue. So, forcing 2000+ sites out there to re-classify files may seem a bit extreme. Should the PR just display a warning after update? Something like "If you have in the past uploaded files with these 3 mimetypes, please be sure to re-classify your files".

BTW, the "re-classify files" link is not present in the UI, which makes it really hard for site builders to know this functionality exists.

[izmeez]

BTW, the "re-classify files" link is not present in the UI, which makes it really hard for site builders to know this functionality exists.

I had no idea there was such a functionality until you raised it in this issue and PR.

[laryn]

If we do a warning rather than a persistent red dot for everyone, the "re-classify your files" could be linked directly. Of course, to your point, if they don't do it immediately they may lose easy access to that link.

[argiepiano]

I've gotten feedback on Zulip. Someone said that "I can see not including the update hook, but not having them as default on a new site seems to restrictive for file/add form."

I look forward to more feedback here.

[argiepiano]

As for re-classifying files, perhaps the update hook could be smarter, looking for files with those extensions in the file_managed table, and turning on the red dot if it finds any? This will be a bit more time consuming for the user, especially for sites with thousands of files...

[argiepiano]

I've made changes to the install hook in the PR to check if there are any Apple files in file_managed. If so, turn on the alert.

[argiepiano]

Actually, the PR needs work - the table will show those files as octet-stream before this fix. So we need to check the file extension, not filemime column. Will get to this later today.

[argiepiano]

OK, switched to check file extensions now. Ready for new review and testing.

[quicksketch]

@argiepiano Could you check my code comment here: https://github.com/backdrop/backdrop/pull/4845#pullrequestreview-2447851252

Overall looks great to me. The update hook makes sense and only bothers users if there's something to actually update. Looks like a really good fix.

[argiepiano]

Thanks, @quicksketch. I've made the requested changes.

[quicksketch]

@argiepiano Thanks! I tested this both with and without any .keynote files in my files directory and it works well in both scenarios.

I think this could be slightly improved by providing a message if files need to be reclassified. Something like:

$message = format_plural('@count files with Apple file types were found.', '@count file with an Apple File type was found', $count); 
$message .= ' ' . t('Visit the <a href="@url">File reclassification form</a> next to classify these files properly.', array(
  '@url' => url('admin/structure/file-types/classify'),
));
return $message;

A returned message will be displayed on update.php when finished. That way users can at least be warned that there's an additional step instead of only getting the red dot in the admin bar. The message should only be returned if there are any files to update.

[argiepiano]

Added!

[quicksketch]

Tested and validated. Works great!

[quicksketch]

Yay, perfect, thank you @argiepiano! I merged https://github.com/backdrop/backdrop/pull/4845 into 1.x and 1.29.x.

I'm sorry this didn't get into today's release. But since it was a security release and this issue required an update hook, it may be for the better that this will wait until 1.29.3.

Thank you @herbdool, @klonos, @laryn, and @izmeez for your reviews and feedback.