Resolution: Not a bug
Affects Version/s: 2.3
Fix Version/s: None
Component/s: Files API
Sam looked at the mimetype changes for me and I never got a chance to record them anywhere So I am documenting here to at least review
- file_file_icon is not a great name for a function. file_get_icon, or something like that to make it easier. In fact given that function only deals with stored_files it should probably be a method of the stored_file class. If not (and I think it should) I really think it should be type-hinting $file. Actually having seeing file_fileinfo_icon as well I think think certainly this should be a method stored_file::get_icon OR called file_icon_for_stored_file and type hinted.
- Same goes with file_fileinfo_icon, I really think that should be a method of the file_info class file_info::get_icon.
- Looking at file_folder_icon I can't help but thing that function is making some very predictable checks and won't be great for performance. Really we should ensure we have a folder icon available in all of the size options there and not make the file_exists checks at all.
- Actually there is another obvious simplification to be made there, max accepts comma separated args, so in fact there is no need to build and array there, that is redundant.
- Oh max() has been used with arrays in mimeinfo as well.
- Todos on file_extension_icon need review I don't think they are any longer applicable.
- I wonder whether the groups should be constants so we can reduce conflicts and control them better.
- I seriously hope you have support tests for these functions to make sure they return what you expect.
- I expect you have tested various areas of Moodle to ensure that headers for files arn't changed during serving and that if they are things are still reasonable.