File extensions "numbers" and "pages" in field.field.media.document.field_media_document.yml are missing from ExtensionMimeTypeGuesser

Created on 12 August 2024, 3 months ago
Updated 19 September 2024, about 2 months ago

Problem/Motivation

Some file extensions in field.field.media.document.field_media_document.yml are missing from ExtensionMimeTypeGuesser, which can cause some issues as reported in ๐Ÿ› Deprecated function: explode(): Passing null to parameter #2 ($string) of type string is deprecated in Drupal\file\Plugin\Field\FieldFormatter\FileMediaFormatterBase::mimeTypeApplies() (line 171 of Plugin/Field/FieldFormatter/FileMediaFormatterBase.php) Needs work .

The following file extensions are missing completely: fodt fods fodp fodg numbers pages

The .key extension is present, but is mapped to application/pgp-keys, to which it was historically mapped (along with .asc). Its official media type according to IANA is now application/vnd.apple.keynote, so perhaps it's time to update the mapping.

Steps to reproduce

  1. Manually configure the *.fodt, *.fods, *.fodp, *.fodg, *.numbers or *.pages file extension as an allowed upload file type, or have them configured automatically via the document media type recipe, the standard profile or the Umami demo profile.
  2. Upload one of these file types to a file field or as a media document.
  3. The resulting file entity will have the fallback "application/octet-stream" MIME type (which is visible in the database, and also sent as an HTTP header for private downloads) rather than an appropriate specific MIME type.

Proposed resolution

  • Re-map .key from application/pgp-keys to application/vnd.apple.keynote
  • Map .numbers application/vnd.apple.numbers
  • Map .pages to application/vnd.apple.pages
  • Map .fodg to application/vnd.oasis.opendocument.graphics-flat-xml
  • Map .fodp to application/vnd.oasis.opendocument.presentation-flat-xml
  • Map .fods to application/vnd.oasis.opendocument.spreadsheet-flat-xml
  • Map .fodt to application/vnd.oasis.opendocument.text-flat-xml
  • While we're here, fix alphabetical order of application/json

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

๐Ÿ› Bug report
Status

Needs review

Version

11.0 ๐Ÿ”ฅ

Component
File systemย  โ†’

Last updated 2 days ago

Created by

๐Ÿ‡บ๐Ÿ‡ธUnited States mfb San Francisco

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @mfb
  • First commit to issue fork.
  • Pipeline finished with Success
    3 months ago
    Total: 517s
    #251243
  • Pipeline finished with Success
    3 months ago
    Total: 516s
    #251417
  • Status changed to Needs work 3 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mfb San Francisco

    @annmarysruthy the array of file extensions should be in order by the numeric value.

    We're going to need to add the other extensions too, but idk what they are supposed to be - maybe same as their od* equivalent, but add a "+xml" on the end because it's XML? Just a wild guess. If we can't find the mapping specified/documented somewhere, we could copy from some other package that maintains a mapping.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mfb San Francisco

    Looks like various packages have the suffix as "-flat-xml" rather than "+xml", so I'd go with that as my best guess

  • Pipeline finished with Success
    3 months ago
    Total: 554s
    #252318
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mark_fullmer Tucson
  • First commit to issue fork.
  • Pipeline finished with Failed
    2 months ago
    #273151
  • Pipeline finished with Failed
    2 months ago
    Total: 76590s
    #273160
  • Pipeline finished with Success
    2 months ago
    #282569
  • Status changed to Needs review 2 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mfb San Francisco
  • Status changed to Needs work 2 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Thanks for reporting.

    Noticed that the issue summary appears incomplete so tagged for that.

    Also will need some kind of test case added to show the issue.

  • Status changed to Needs review 2 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mfb San Francisco

    @smustgrave There is already test coverage of the extension to MIME type mapping, so I don't think additional test coverage is needed here.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    If it already had tests though then why is the change needed? Shouldn't the tests of caught the bug?

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mfb San Francisco

    @smustgrave What did you have in mind? You wouldn't want to add a test for each of the ~369 mappings. It's just a couple big flat arrays.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mark_fullmer Tucson

    What did you have in mind? You wouldn't want to add a test for each of the ~369 mappings.

    I agree that this doesn't seem like a typical thing that Drupal core would add test coverage for, namely, a test that verifies that all of the extensions defined in the standard installation profile's media types are present in the MimeTypeGuesser array. I think automated test coverage *would* be more appropriate for ๐Ÿ› Deprecated function: explode(): Passing null to parameter #2 ($string) of type string is deprecated in Drupal\file\Plugin\Field\FieldFormatter\FileMediaFormatterBase::mimeTypeApplies() (line 171 of Plugin/Field/FieldFormatter/FileMediaFormatterBase.php) Needs work , to demonstrate that when unknown file types are declared, they do not trigger a PHP warning, and/or are handled through appropriate form validation.

    So, for this issue, I think it might be most appropriate to start by adding steps to reproduce, to show what "problem" there is when the MimeTypeGuesser does not include some of the file extensions defined in the standard installation profile's document media type configuration. Something like:

    1. Install the "standard" installation profile
    2. Enable the media_library module
    3. Go to Structure > Media Types > Document > Edit > Manage display
    4. [Describe resulting problem]
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mfb San Francisco

    Added steps to reproduce to the issue summary.

    I was thinking of this from the perspective of just missing data, like if a language was missing from the array of languages in LanguageManager.php, as Drupal maintains a mapping of known file extensions and MIME types, and file extensions can be configured manually. (Of course neither languages nor file extensions/MIME types are an exhaustive list, as they are only added when someone notices them missing..)

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    If we are re-mapping here are there others to be checked?

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mfb San Francisco

    We could create a followup issue to do something like.. create a script to audit/compare drupal's mapping vs. the MIME type databases typically found in operating system distributions? There are actually at least two such databases: The /etc/mime.types file and the /usr/share/mime directory.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mfb San Francisco

    ...and it looks like there is already a third-party tool pretty similar to what I mentioned - the fileeye-mimemap update utility retrieves MIME types from both Apache and freedesktop.org, merges them, and generates a map. This library could be incorporated as a runtime third-party dependency, or at least be leveraged to help maintain the existing mapping.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia kim.pepper ๐Ÿ„โ€โ™‚๏ธ๐Ÿ‡ฆ๐Ÿ‡บSydney, Australia

    #17 sounds like a great idea. Created ๐Ÿ“Œ Generate file extension mime type map with fileeye/mimemap update utility Active

  • The Needs Review Queue Bot โ†’ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide โ†’ to find step-by-step guides for working with issues.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    False positive

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    So since we don't have test coverage already and seems #18 mentions adding a way to auto do this I'm assuming coverage will be added there.

    That case I believe this change is fine, will go out on a limb and RTBC it.

Production build 0.71.5 2024