Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-32999 META: Files UI Stage 2 polishing in master
  3. MDL-33161

Notes from Sam H's review of mimetype changes

    XMLWordPrintable

    Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Not a bug
    • Affects Version/s: 2.3
    • Fix Version/s: None
    • Component/s: Files API
    • Labels:
      None
    • Affected Branches:
      MOODLE_23_STABLE

      Description

      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

      1. 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.
      1. Same goes with file_fileinfo_icon, I really think that should be a method of the file_info class file_info::get_icon.
      1. 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.
      1. 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.
      1. Oh max() has been used with arrays in mimeinfo as well.
      1. Todos on file_extension_icon need review I don't think they are any longer applicable.
      1. I wonder whether the groups should be constants so we can reduce conflicts and control them better.
      1. I seriously hope you have support tests for these functions to make sure they return what you expect.
      1. 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.

        Attachments

          Activity

            People

            Assignee:
            marina Marina Glancy
            Reporter:
            poltawski Dan Poltawski
            Participants:
            Component watchers:
            Matteo Scaramuccia, Andrew Nicols, Jun Pataleta, Michael Hawkins, Shamim Rezaie, Simey Lameze
            Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved: