Separate MIME type mapping from ExtensionMimeTypeGuesser

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

kim.pepper → made their first commit to this issue’s fork.

kim.pepper → changed the visibility of the branch 11.x to hidden.

Did a fair amount of cleanup on this issue.

• ExtensionMimeTypeGuesser is now using the MimeTypeMapper
• Removed the getMapping() method which was temporary anyway and refactored tests to not call it
• Split up tests and moved them to the Drupal\KernelTests\Core\File\MimeType namespace to make the namespace under lib/
• Handle NULL returned from ExtensionMimeTypeGuesser::guessMimeType()
• Updated the changes to the default map that went in since this was last worked on a year ago
• Added typehints
• Added more dependency injection
• Fixed linting errors
• Updated the deprecation message to refer to 11.1.0 and 12.0.0

Also created a follow-up 📌 Generate file extension mime type map with fileeye/mimemap update utility Active

Why not allow contrib modules to call getMapping() so they can arbitrarily alter the mapping?

Can't already alter the mapping with the alter hook?

Also different mapping implementations could use different data structures, so what gets returned from getMapping() could be different.

Well I'm looking at how to still allow File MIME module → work without using the deprecated hook_file_mimetype_mapping_alter(). It looks like the new alter hook doesn't allow contrib module to get the mapping so it can make various changes, and then apply it via setMapping().

And different data structures? Well that would make it even more difficult for a contrib module to alter the mapping, lol sigh. I confess to not actually following this issue.

function hook_mimetype_alter(?MimeTypeMapperInterface $mime_type_mapper = NULL) gives you the ability to add/remove/update mappings by using the API interface rather than the raw data structure, which is better IMO.

Yes I see know it's possible to call addMapping(). But I'd have to iterate through and make hundreds of method calls (if someone configures File MIME module to parse their /etc/mime.types file), with an array_search() in each one, so it just seems slower, that's all. And I'd have to double check there is no behavior change from how it worked previously.

Realized I was being lazy and I should benchmark it. So, I found that implementing the new hook in File MIME module (configured to add all the MIME types from the /etc/mime.types file) adds about 10 milliseconds of execution time. This is probably fine for those cases where the hook is invoked during file uploads - such requests are going to be extremely slow regardless - but not ideal for other pages where the hook might be invoked (e.g. managing display of an entity with a file field). Admittedly, File MIME module doesn't have very many installs (and more installs of the Drupal 7 branch than the current branch).

#128 Thinking about this further, we should probably remove setMapping() to hide the underlying data structure. Projects like https://www.drupal.org/project/sophron → that use the https://github.com/FileEye/MimeMap library have a different data structure.

#129 hmm. Shouldn't that hook only get fired on container build? It should be cached after that.

@kim.pepper no, there is nothing caching the map, whether in the container or otherwise. Both the old and new alter hooks run whenever the map is used.

Projects like https://www.drupal.org/project/sophron → that use the https://github.com/FileEye/MimeMap library have a different data structure.

I looked at Sophron module, and it has a totally different mechanism for allowing other modules to alter its mapping - it dispatches an event. I guess you are saying that a future version of Sophron module could start invoking this alter hook?

#131 I don't think that's right.

  file.mime_type.mapper:
    class: Drupal\Core\File\MimeType\MimeTypeMapper
    calls:
      - [alterMapping, ['@module_handler']]

The hook gets fired when this service is created.

alterMapping is called whenever the file.mime_type.mapper service is created (for example when ExtensionMimeTypeGuesser is constructed). So the alter hook fires on any request that needs to create that service.

E.g. if a page renders an entity that displays an audio file field, the service is needed and the alter hook fires. But once that entity is cached in the render cache, it will no longer render, and therefore the service won't be created, so the alter hook won't fire. Maybe that is the caching you were referring to, I'm not sure? There are also admin pages where the service is created, and the alter hook fires, without any sort of cache being involved (e.g. managing display of an entity with a file field).

#134 Yeah I guess I was talking about the container cache. I misunderstood what you meant by 'Both the old and new alter hooks run whenever the map is used.' thinking you meant when we call guessMimeType() but it's when the service is created.

I did some more work removing setMapping(). I don't think that's going to cause too many issues.

I was timing the alter hook on requests where the service is created (e.g. because guessMimeType() was called at least once). Since it's when the service is created, the 10ms slowdown only happens at most once per request.

I've done a fair bit of rework to replace the new hook with tagged services. I think triggering hooks during container build is a bit of an antipattern, and this approach uses a more symfony native approach.

@kim.pepper I don't understand why you say "triggering hooks during container build" - the hook was triggered when the service was created, i.e. when a request needs to use it for the first time, not when the container is built.

the hook was triggered when the service was created

Fair enough.

As for the test fail, I have no idea why \Drupal\Tests\block\Functional\BlockCacheTest::testCachePerPage() is failing. It's passing locally for me. Could it be a random fail?

Found the source of the test fails was a deprecation warning on the legacy hook_file_mimetype_mapping_alter() in file_test.module. We already test this in file_deprecated_test.module so I removed it.

Ready for reviews again!

mondrake → changed the visibility of the branch 2311679-separate-mime-type to hidden.

I did a review. I am definitely biased by having developed Sophron, feel free to push back.

Also, adding a related issue that should be committed before this IMHO

Thanks for the detailed review @mondrake. Much appreciated. I've responded to all your comments and made changes where I agree. I resolved a few where I did not agree, but feel free to re-open those threads if you disagree.

I created a POC MR with a FileEye MIME map implementation to check if our interfaces and factory approach works with 3rd party libs.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. 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.

Rebased.

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.

Tests are passing.

A bunch of nitpickeries from me. I think this is almost ready, cannot RTBC it though.

Addressed all feedback. Thanks again.

One small deprecation docblock issue with version numbers.

Thanks. Feedback addressed.

Added tests and answered question.

lgtm

godotislate → changed the visibility of the branch 2311679-separate-mime-type-11.x to hidden.

godotislate → changed the visibility of the branch 2311679-separate-mime-type-11.x to active.

godotislate → changed the visibility of the branch 2311679-separate-mime-type-fileeye to hidden.

🐛 ExtensionMimeTypeGuesser::guessMimeType return less accurate MIME type when file extensions have multiple parts Active went in, and this needs a reroll now.

working on this

rerolled

lgtm