Separate MIME type mapping from ExtensionMimeTypeGuesser

thanks for opening the issue

1. +++ b/core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php
   @@ -921,13 +58,13 @@ public function guess($path) {
   +  public function getMapper() {
   
   +++ b/core/modules/system/src/Tests/File/MimeTypeTest.php
   @@ -84,7 +84,7 @@ public function testFileMimeTypeDetection() {
   +    $extension_guesser->getMapper()->setMapping($mapping);
   

   I do not see the reason to have this public getter, just retrieve the mapper from the $this->container

2. +++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeMapper.php
   @@ -0,0 +1,921 @@
   +class MimeTypeMapper {
   

   Needs an interface, so that its swappable

1. +++ b/core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php
   @@ -912,22 +49,11 @@ public function guess($path) {
        //   - awesome.image.jpeg
        while ($additional_part = array_pop($file_parts)) {
          $extension = strtolower($additional_part . ($extension ? '.' . $extension : ''));
   -      if (isset($this->mapping['extensions'][$extension])) {
   -        return $this->mapping['mimetypes'][$this->mapping['extensions'][$extension]];
   +      if (isset($mapping['extensions'][$extension])) {
   +        return $mapping['mimetypes'][$mapping['extensions'][$extension]];
          }
        }
   

   One thing that we were discussing was if and how much of this logic should move to the mapper?

   Right now, there is just getMapping(). But that's a very arbitrary structure, wouldn't it be nice to have getMimeTypes() too, if you just need those?

   And if we go there, would it be useful if the mapper would possibly even have a method like getMimeTypeForExtension($extension)? Then this would be re-usable if you'd just have a extension and not an actual file. Not sure if we should consider that at all or in a separate issue...

2. +++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeMapperInterface.php
   @@ -0,0 +1,36 @@
   +  /**
   +   * Replaces the mimetypes/extension mapping.
   +   *
   +   * @param array|null $mapping
   +   *   Passing a NULL mapping will reset to the default mapping.
   +   */
   +  public function setMapping(array $mapping = NULL);
   

   Why do we need a setter? If you want to alter it, use the hook, this seems strange.

totally agreed with 1.

2. is strange indeed and was there pre-conversion, see also #1921558-17: Convert file_get_mimetype() to use Symfony MimeTypeGuessers.
In other words, the hook will only fire if the default mapping is used: The setter overrides the hook.

I very much support to make the hook fire *always* and get rid of the setter...or the other way around

#2388749: Register symfony's mime guessers if they are supported is related.

#2388749: Register symfony's mime guessers if they are supported has been committed, and it removed hook_file_mimetype_mapping_alter().

Rerolled patch in #8 (see do-not-test patch), and removed the alter call here too. Interdiff against the reroll.

Would love to see this in as there was no straightforward way in contrib to map extensions to mimetypes before #2388749: Register symfony's mime guessers if they are supported and also hacks like this

/**
   * {@inheritdoc}
   */
  public function getMimeType($extension) {
    return $this->mimeTypeGuesser->guess('dummy.' . $extension);
  }

now no longer (legitimately I'd say) work.

What is the reasoning for removing the alter hook? This hook has been *very* useful for contrib to easily provide support for new file types when adding them to core would take much longer.

@dave see #2388749-10: Register symfony's mime guessers if they are supported

Fixed PHPUnit test failure.

I dont think we should expose the mapping array, it is implementation details.
Instead lets provide methods.
Also added a method addMapping($mimetype, $extension) which adds/overwrites mappings.
If we agree on it i ll go ahead and write tests

#17

I dont think we should expose the mapping array

+1, but then I suggest also to have a method to return supported extensions given a MIME type, that would be possible to be done elsewhere with the mapping array exposed, but not otherwise. Done here.

Also added a method addMapping($mimetype, $extension) which adds/overwrites mappings.

Great! It's what I was suggesting in #2388749-39: Register symfony's mime guessers if they are supported.

If we agree on it i ll go ahead and write tests

+1 :)

#2388749: Register symfony's mime guessers if they are supported has been reverted

#21 new patch coming soon.

New patch - combining parts of #8 and parts of #18. Interdiff against #8.

This keeps new methods introduced by @ParisLiakos in #17 and by me in #18, and keeps (but changed to protected) the getMapping method in #8 which is needed to allow the alter call to hook_file_mimetype_mapping_alter(). I'd say it would be probably better to change the signature of the hook to pass the MimeTypeMapper object and use addMapping() in hook implementations instead of letting hooks manipulate the array by reference, as pointed out in #17. But I guess that needs discussion.

Also added some tests for MimeTypeMapper::getExtensionsForMimeType() and MimeTypeMapper::addMapping().

+++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeMapper.php
@@ -0,0 +1,944 @@
+    if (!in_array($mimetype, $this->getMapping()['mimetypes'])) {
...
+    $key = array_search($mimetype, $this->getMapping()['mimetypes']);
...
+    $extensions = $this->getMapping()['extensions'];
+    return isset($extensions[$extension]) ? $this->getMapping()['mimetypes'][$extensions[$extension]] : NULL;
...
+    if (!in_array($mimetype, $this->getMapping()['mimetypes'])) {
...
+    $key = array_search($mimetype, $this->getMapping()['mimetypes']);
+    $extensions = array_keys($this->getMapping()['extensions'], $key, TRUE);

lets only call getMapping() once and store it in a local variable instead of doing unnecessary function calls in the same method

I'd say it would be probably better to change the signature of the hook to pass the MimeTypeMapper object and use addMapping() in hook implementations instead of letting hooks manipulate the array by reference

Good idea!

#24 done

1. +++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeMapper.php
   @@ -0,0 +1,946 @@
   +    $this->getMapping();
   

   can we add a comment here? why this is needed

2. +++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeMapperInterface.php
   @@ -0,0 +1,57 @@
   +  public function getMimeTypes();
   

   and maybe add a small test case for this method

3. +++ b/core/modules/system/file.api.php
   @@ -102,25 +102,30 @@ function hook_file_url_alter(&$uri) {
   + * Invoked by \Drupal\Core\File\MimeType\MimeTypeMapper::getMapping(). It
   ...
   + * should use the \Drupal\Core\File\MimeType\MimeTypeMapper::addMapping()
   ...
   + * \Drupal\Core\File\MimeType\MimeTypeMapper::getMimeTypeForExtension() and
   + * \Drupal\Core\File\MimeType\MimeTypeMapper::getExtensionsForMimeType()
   ...
   + * @see \Drupal\Core\File\MimeType\MimeTypeMapper::addMapping()
   + * @see \Drupal\Core\File\MimeType\MimeTypeMapper::getMimeTypeForExtension()
   + * @see \Drupal\Core\File\MimeType\MimeTypeMapper::getExtensionsForMimeType()
   

   Maybe those docs should refer to the interface methods ;)

#26 done.

#26.1 - this is needed to make sure that alter hook is invoked before adding any mapping. Maybe I am overcomplicating, but it could be a case that code outside of the hook implementation add a mapping, then the first time any other method is called the hook would fire and may override that mapping.

Also fixed a couple of doc issues.

Nice, thanks..so i guess all we need now is a beta evaluation template and updating https://www.drupal.org/node/2258015 with alter hook change.
other than that, this is rtbc

#28 updated change record draft

https://www.drupal.org/node/2258015/revisions/view/8067869/8077401

can anybody take care of the beta eval?

thank you
I think this is a bug since its a regression

1. +++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeMapper.php
   @@ -0,0 +1,961 @@
   +    if (!$this->isMappingAltered) {
   ...
   +    $this->getMapping();
   

   There's a weird recursion going on here when the alter hook is fired.

2. +++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeMapper.php
   @@ -0,0 +1,961 @@
   +
   +    return TRUE;
   

   This seems pointless - just return $this for a fluent interface.

3. I really like the new way of adding mappings in the alter - much nicer.

4. +++ b/core/modules/system/file.api.php
   @@ -102,25 +102,31 @@ function hook_file_url_alter(&$uri) {
     * Alter MIME type mappings used to determine MIME type from a file extension.
   

   This is not quite correct anymore.

Thanks @alexpott.

#31.1 - different approach here, alter hook is invoked at construction time before any method is called. Not sure it is OK.
#31.2 - done
#31.3 - :)
#31.4 - changed

also changed a small doc issue 'Contains \Drupal...' instead of 'Contains Drupal'

no, we should avoid doing stuff on the constructor other than injecting dependencis, especially invoking a hook.

There's a weird recursion going on here when the alter hook is fired.

which is fine since

$this->isMappingAltered = TRUE;

is called before the hook is called, so the function will just return $this->mapping, it wont do anything else.

I agree that doing the alter in the constructor is wrong. The alter should only be fired when necessary but also the recursive call during an alter is also weird. Plus we've lost the ability to remove mappings.

We are indeed facing recursion here, because if we call e.g. getMimeTypeForExtension, this will call getMapping which, on first run, will invoke the hook. Hook implementations then will call e.g. addMapping who in turn will call again getMapping. So here we need measures to avoid endless loops. I made a change here to make things I hope more explicit, starting back from #27. If any idea how to circumvent this, feel free :)

Next - adding removeMapping and removeMimeType, will post a patch soon.

Added ::removeMapping() and ::removeMimeType() methods.

I found an alternative, i.e. using the 'calls' parameter in service definition to execute the alter hook separately from other methods, but not in the constructor (kind of 'setter injection' according to Symfony docs). Sorry, I'm learning here...

@mondrake that looks like a really nice solution.

#2260061: Responsive image module does not support sizes/picture polyfill 2.2 just introduced the below

+++ b/core/modules/responsive_image/responsive_image.module
@@ -328,17 +445,38 @@ function responsive_image_get_image_dimensions($variables) {
+  // The MIME type guesser needs a full path, not just an extension, but the
+  // file doesn't have to exist.
+  $fake_path = 'responsive_image.' . ImageStyle::load($image_style_name)->getDerivativeExtension($extension);
+  return Drupal::service('file.mime_type.guesser.extension')->guess($fake_path);
+}

which is OKish, but a workaround. With the patch here this could become

  $extension = ImageStyle::load($image_style_name)->getDerivativeExtension($extension);
  return Drupal::service('file.mime_type.mapper')->getMimeTypeForExtension($extension);

is it something to be added to this issue?

Based on @yched's comment in #2426757-5: responsive_image_build_source_attributes() calls mime type guesser over and over, added #39. Also added a strtolower($extension) in MimeTypeMapper::addMapping to keep consistency with other methods.

Rerolled.

Any reviewers?

We were at RTBC back in #30 and @alexpott's feedback in #31 was addressed, see #38 also.

BTW we also have a published change record which is giving this for done, so it would have to be at least set back to draft while this is sorted out.

• Added lazy: true tag to the file.mime_type.mapper service, to match mime type guessing services definition
• fixed a glitch in the responsive_image module
• Fixed and added more dcoumentation to hook_file_mimetype_mapping_alter
• Changed String::format to SafeMarkup::format in MimeTypeTest

Reroll.

Added/updated the proxy classes.

IMO, this needs decision before RC:

• the changes to hook_file_mimetype_mapping_alter() signature (passing a MimeTypeMapperInterface object instead of the internal mapping array) will be hard to get in during 8.0 because of BC
• the published change record give the changes in this issue for done, so if this gets postponed, the CR will have to be reverted in some parts

Rerolled, conficts due to #2534066: Allow selecting the original image when creating a responsive image style, then made the logic in responsive_image_get_mime_type simpler to follow, and added comments.

This is a soft blocker for ImageMagick, see #2612590: Allow configuring in the UI the image formats supported by the toolkit. It would allow the ImageMagick toolkit to be configurable in terms of image formats accepted. Now the supported file extensions are hardcoded in ImagemagickToolkit::getSupportedExtensions() similarly to GD core toolkit, but the difference with ImageMagick is that if you want to add support for an image format you do not need to write specific code for that format as in GD - you just have to have a version of the executable that supports the format you need.

Just a reroll. However this is now no longer acceptable I think, it breaks BC. This issue is still blocking a stable release for the ImageMagick module.

An attempt to move on without BC break. This restores the hook_file_mimetype_mapping_alter to its original signature, still keeping the new service file.mime_type.mapper.

+++ b/core/core.services.yml
@@ -1554,6 +1554,11 @@ services:
+    lazy: true

Are we sure this needs to be lazy? Given this is already injected into a lazy service that always needs it I'm not sure.

@alexpott thanks, patch here removes lazy flag and proxy class.

Whoops..

Let's see this.

Removed no longer needed file docblocks. Anyone up for a review?

Reroll after #2796329: \Drupal\Core\File\MimeType\ExtensionMimeTypeGuesser maps the 'js' extension to the 'application/x-javascript' MIME type instead of 'application/javascript'.

A problem with #79 was that, even if existing API were not changed, still any class that was extending from ExtensionMimeTypeGuesser and trying to access its protected $mapping and $defaultMapping properties would fail, since the mapping is moved to the new class.

Here I am using a magic __get method in ExtensionMimeTypeGuesser, so that any get attempt from ExtensionMimeTypeGuesser::$mapping or ExtensionMimeTypeGuesser::defaultMapping will be redirected to the mapper service, and fetched through two public methods there, introduced for BC and marked @internal.

This issue is pretty much stalled :(

I wonder whether it would make sense to take a different approach in the short/medium term, i.e. just add the methods that would be needed in contrib to the ExtensionMimeTypeGuesser service:

getMapping()
addMapping($mimetype, $extension)
removeMapping($extension)
removeMimeType($mimetype)
getMimeTypes()
getMimeTypeForExtension($extension)
getExtensionsForMimeType($mimetype)

and postpone introducing the separate service MimeTypeMapper to D9.

Any feedback?

Two years later... I would still like this to move on. Getting the list of available MIME types or the list of file extensions mapped to a MIME type is not possible, and workarounds are not good.

In the ImageMagick module, at one point I was extending a class from the guesser, but this was forcing the alter hook to be called again. Lately I am using reflection to get the value of the $mapping array after it has been processed by the alter hook, but again this smells bad.

Relaunching here. This patch (sorry no interdiff):

1. Defines a new file.mime_type.mapper service, implemented through a Drupal\Core\File\MimeType\MimeTypeMapper class (with a supporting Interface), that advocates the execution of the hook_file_mimetype_mapping_alter hook and provides OO methods to retrieve and manipulate the MIME type to extension mapping array.
2. Provides a Kernel test for that.
3. Changes the ExtensionMimeTypeGuesser class to use the new mapper; the ::setMapping method is deprecated, and deprecation tests are added.
4. Changes the signature of the hook_file_mimetype_mapping_alter hook to pass the mapper object in addition to the $mapping array. This for BC, in D9 the $mapping array should be removed. Hook implementations can, as of the release after which the changes are made, either manipulate the array directly or use the OO methods to alter the mapping.

+++ b/core/modules/file/tests/file_test/file_test.module
@@ -291,16 +292,13 @@ function file_test_file_url_alter(&$uri) {
-function file_test_file_mimetype_mapping_alter(&$mapping) {
+function file_test_file_mimetype_mapping_alter(&$mapping, MimeTypeMapperInterface $mime_type_mapper) {

this means no BC, so not sure it is possible in minor versions

OK, I agree it is confusing. Let me do sth else.

Instead of changing the hook signature, this is now deprecating hook_file_mimetype_mapping_alter and replacing it with a hook_mimetype_alter, where the mapper is passed as an argument.

If we agree this is the sustainable way forward, I will add a test for the hook deprecation.

Updated IS.

Added deprecation tests for hook_file_mimetype_mapping_alter.

FTR, if anyone interested, I released today a beta1 version of the Sophron module:

Features

• Enhances Drupal's MIME type detection based on file extension to recognise 1200+ MIME types from 1600+ file extensions (vs Drupal's 360 MIME types and 475 file extensions).
• Provides an extensive MIME type management API through FileEye/MimeMap.
• Optionally replaces Drupal's core MIME type extension-based guesser.

Re-roll of #93.

Seems like these test fails are unrelated so setting back to Needs Review & Triggering bots.

Re-rolled as patch was not applying on core.services.yml
Patch is smaller due to the similarity from ExtensionMimeTypeGuesser with MimeTypeMapper

diff --git a/core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php b/core/lib/Drupal/Core/File/MimeType/MimeTypeMapper.php
similarity index 86%
copy from core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php
copy to core/lib/Drupal/Core/File/MimeType/MimeTypeMapper.php

This was a bugsmash triage target. kim.pepper agreed it was a bug. So let's get a reroll here.

One last test failing.

ExtensionMimeTypeGuesserDeprecationTest::testConstructorDeprecation test is failing due to ExtensionMimeTypeGuesser::$mapping or ExtensionMimeTypeGuesser::$defaultMapping are not being redirected to the mapper service.

I am not sure if we should add setters or getters as added in #80 to maintain backwards compatibility.

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.

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 #3477946: Generate file extension mime type map with fileeye/mimemap update utility

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 to work without using the deprecated hook_file_mimetype_mapping_alter(). It looks like the new alter hook doesn't allow a 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 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!

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

#3477346: ExtensionMimeTypeGuesser::guessMimeType returns less accurate MIME type when file extensions have multiple parts went in, and this needs a reroll now.

working on this

rerolled

lgtm

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.

I've merged 11.x into this MR. Looks like the Hook Attributes work that got merged is triggering some deprecations that will need looking at.

This was just a rebase on 11.x so putting back to RTBC

Spoke to @xjm at DrupalCon Singapore contribution day, and she recommended tagging as Needs framework manager review.

Discussed with @larowlan and this should be a task.

I squashed all the commits in the MR to make rebasing on 11.x easier.

Looks like we are triggering deprecation errors in Drupal\Core\File\MimeType\DefaultMimeTypeMap::getMapping() and Drupal\Core\File\MimeType\DefaultMimeTypeMap::setMapping()

Removing 'Needs framework manager review' tag as @larowlan has reviewed.

A couple comments.

Thanks for the review. Addressed feedback.

Apologies, seeing a couple new things with fresh eyes that I missed last pass:

• One more use of in_array and array_search can be reduced to array_search once
• Discovered in #3492453: Memory leak in DrupalKernel when installing modules that #2066993: Use magic methods to sync container property to \Drupal::getContainer in functional tests should be done, so can get ahead of it here with using \Drupal::service() instead of $this->container->get in test classes

Applied suggestions.

Just noticed the CR links redirect to this issue and not to a CR. A CR for this is actually missing.

Created a draft CR. Will look at changing the links in the morning unless someone else gets there first.

Updated links to CR.

LGTM. Nice work, @kim.pepper. Thanks for sticking with this.

Working on BC fixes.

Now Sophron is passing tests with this MR applied (deprecation errors are thrown but that is OK). https://github.com/mondrake/d8-unit/actions/runs/12844774697/job/3581814...

We need to keep the protected properties as they are and cannot rely on the new ones (the map and the file system) to be initialised when the methods are called, as the classes that today extend ExtensionMimeTypeGuesser overriding the constructor will not have done that.

I think we have resolved all threads except one question for @alexpott about changes to MimeTypeMapFactory

Looks like all the latest feedback has been addressed. Back to RTBC,

I've updated the MR to leverage \Drupal\Core\DependencyInjection\DeprecatedServicePropertyTrait - it'll allow us to remove the module handler property and have less complexity in the constructor.

There are test failures in Drupal\Tests\system\FunctionalJavascript\Form\TriggeringElementTest. I'm guessing unrelated, and the tests need re-running.

I re-ran the pipeline a couple of times to see if it was a random failure, but still getting the fails.

Head was broken, might need a rebase.

Maybe the MR branch needs a rebase? Looks like there was a commit to HEAD that broke TriggeringElementTest, but it was reverted. See #3461309: Refactor FormTestClickedButtonForm::buildForm #16.

Rebased on 11.x

Tests back to green.

Committed 37737af and pushed to 11.x. Thanks!

Small issue found in #3519804: DummyMimeTypeMapLoadedSubscriber does not get registered.