[Symfony 5] The "Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesser" class is deprecated since Symfony 4.3, use "Symfony\Component\Mime\MimeTypes" instead.

Just looking for ways to break this down into digestible chunks

The bottom two notices no longer appear in the drupal codebase, for the upper two that is a different story

In short I am testing that theory with a patch that should come back green.. and we can then separate them out into a separate quick fix issue.

'The "Symfony\Component\HttpFoundation\File\MimeType\FileBinaryMimeTypeGuesser" class is deprecated since Symfony 4.3, use "Symfony\Component\Mime\FileBinaryMimeTypeGuesser" instead.'

'The "Symfony\Component\HttpFoundation\File\MimeType\FileinfoMimeTypeGuesser" class is deprecated since Symfony 4.3, use "Symfony\Component\Mime\FileinfoMimeTypeGuesser" instead.'

Haha so my theory was incorrect.

So my second plan to break this into small steps is just to work on clearing the failures associated with

./vendor/bin/phpunit -c core/ -v ./core/tests/Drupal/KernelTests/KernelTestBaseTest.php 

as that would seem to be a good focal point.

I am posting early, I don't have the complete solution. I just was to see the wider effect of a controversial change.

a) I did a global replace of

Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesser for Symfony\Component\Mime\MimeTypes

and then came across a subtle fixup

b) Of relevance we have two script generated files :-

core/lib/Drupal/Core/ProxyClass/File/MimeType/ExtensionMimeTypeGuesser.php
core/lib/Drupal/Core/ProxyClass/File/MimeType/MimeTypeGuesser.php

After the global replace I started getting new warnings which, in short, were fixed after running a modified generate-proxy-class script

 php core/scripts/generate-proxy-class.php 'Drupal\Core\File\MimeType\MimeTypeGuesser' "core/lib/Drupal/Core"

the script ( see core/lib/Drupal/Component/ProxyBuilder/ProxyBuilder.php )
needs to be modified to keep up symfony's use of php7's returnTypes and parameter types

for a single example in core/lib/Drupal/Core/ProxyClass/File/MimeType/MimeTypeGuesser.php

+         * {@inheritdoc}
+         */
+        public function getMimeTypes(string $ext) : array
+        {
+            return $this->lazyLoadItself()->getMimeTypes($ext);
+        }

we needed to auto add the string parameter type, and the array return type otherwise interface type violations

So hey Symfony's use of modern php is once again dragging Drupal into the modern age.

It is something I am happy with but it is bound to be controversial - maybe I need to split that off into a side issue so other people who care about auto-script generation will see it.

PS

Just making my todo list public

Next :-

a) I think some files ending in guesser ( see MimeTypeGuesser ) need to be reduced MimeType to be consistent with symfony.
b) MimeTypeGuesser is a singleton. I need to think careful about what has changed.
c) ProxyBuilderTest need to be augmented in light of my changes.
d) Fix more tests.

Just quick skim - as classes abstract now how it affects contrib?
Probably it will need change record

1. +++ b/core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php
   @@ -3,12 +3,12 @@
     * Makes possible to guess the MIME type of a file using its extension.
   ...
   -class ExtensionMimeTypeGuesser implements MimeTypeGuesserInterface {
   +abstract class ExtensionMimeTypeGuesser implements MimeTypesInterface {
   
   +++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeGuesser.php
   @@ -4,13 +4,13 @@
     * Defines a MIME type guesser that also supports stream wrapper paths.
   ...
   -class MimeTypeGuesser implements MimeTypeGuesserInterface {
   +abstract class MimeTypeGuesser implements MimeTypesInterface {
   

   docs needs change as class is abstract now

2. +++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeGuesser.php
   @@ -48,6 +48,12 @@ public function __construct(StreamWrapperManagerInterface $stream_wrapper_manage
   +  public function reset() {
   +  }
   

   Would be great to add comment why it's no-op

Thanks for the quick scan .. abstract is going to be reverted in the next iteration.
Lots is going change - I probably posted too early yesterday ... I am aiming for the next post last Sunday night.

I am still fleshing things out... so still too early for review. I just want to see testbots response to an intricate set of changes.

A consequence of converting

-use Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface;
+use Symfony\Component\Mime\MimeTypesInterface;

means that we no longer have to register stuff with a symfony ( think singleton) ... classes are picked up by virtue of implementing the "MimeTypesInterface"so I have deleted core/tests/Drupal/Tests/Core/File/MimeTypeGuesserTest.php and the registration logic.

@andypost - this means #11.2 "the reset issue" goes away.

And there are going to be plenty of API changes I need to provide a change record for.

Regarding #10 a ( the todo list )
files ending in 'guess' have been renamed

for example

-class ExtensionMimeTypeGuesser implements MimeTypeGuesserInterface {
+class ExtensionMimeType implements MimeTypesInterface {

as part of converting things ending in guesser I have deprecated two core services and just renamed them. leaving the legacy guess() method in place.

Also I have ensured a smattering of tests are ok

./vendor/bin/phpunit -c core/ ./core/modules/media/tests/src/Functional/MediaBulkFormTest.php
./vendor/bin/phpunit -c core/ ./core/modules/image/tests/src/Kernel/ImageItemTest.php
./vendor/bin/phpunit -c core/ ./core/tests/Drupal/KernelTests/KernelTestBaseTest.php

My next steps

The properties.

ExtensionMimeType::defaultMapping and Symfony\Component\Mime::map

now appear to provide the same function ... that is providing golden default for which drupal can override.

I am currently thinking if drupal really should allow contrib to override the golden values.

I guess we do, so "how I can remove defaultMapping whilst still preserving the functionality."

Also testing ..

A ) Add service depecation tests -- to verify contrib is given time to change
B) ProxyGenerator tests
C) Test the new getExtensions() and getMimeTypes() methods

The motivation of this issue

"Resolve the following SF4 Deprecations if possible:"

We will have to add before we subtract.

I have spent some time tonight thinking more carefully about the how I dperecate

a) We need to keep the MimeTypeGuesser::registerWithSymfonyGuesser logic in DrupalKernel at least until Drupal10 for contribs sake.

b) My previous attempt was to move the functionalilty of MimeTypeGuesser into MimeType while provide sane legacy methods and deprecation notices -- Well that was overly complex and so I have now restored MimeTypeGuesser, added a trigger warning to that class and made the new MimeType class and its new service definition standalone.

Given these concerns the deprecation notices have to stay until D10 plus I have added two more. That makes me grumpy .. but I think it is the best solution.

+      'The Drupal\Core\File\MimeType\MimeTypeGuesser is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Instead, use Drupal\Core\File\MimeType\MimeType. See https://www.drupal.org/node/3055193',
+      'The Drupal\Core\File\MimeType\ExtensionMimeTypeGuesser is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Instead, use Drupal\Core\File\MimeType\ExtensionMimeType. See https://www.drupal.org/node/3055193',

in terms of review - DrupalKernel this is a bit hacky I think it is important at least that we added a public deprecation notice here
as many companies modify this file.

    // Override of Symfony's MIME type guesser singleton.
    // Deprecated: registering Guesser will no longer be needed in Drupal 10.
    if (class_exists('\Drupal\Core\File\MimeType\MimeTypeGuesser')) {
      // @codingStandardsIgnoreLine
      \Drupal\Core\File\MimeType\MimeTypeGuesser::registerWithSymfonyGuesser($this->container);
    }

A high level description of these changes :-

a) Added the appropriate @legacy and @expectedDeprecation to some test files.
b) Added tests for the new ::getMimeTypes() and ::getExtensions() methods.

This is getting closer to "Need review" by a human now.

I am getting into the polishing phase but I am not there yet.

PS

To make this more review-able I am trying to separate out the Proxy Builder modifications.

For now though that code needs to be in both issues.

a) Fixed tests.
b) MimeType::getMimeTypes() When I measured the code coverage I saw that method was missing despite having tests...fixed.
c) Lots of little polishes.

I think this is ready for review.

PS added 'overrrides' testing

It might be easier to review if I gave a brief overview, in the form of a change record.

https://www.drupal.org/node/3126004

This is looking pretty good. At first I wondered why we can't just reuse the existing services and add a BC layer inside but I see that is not possible with the interface changes. I also agree that #15.b around merging the services might be possible but is out of scope, let's keep these services 1:1 with the originals.

1. +++ b/core/lib/Drupal/Core/File/MimeType/ExtensionMimeType.php
   @@ -0,0 +1,987 @@
   +  public function guessMimeType($path) : ?string {
   ...
   +  public function isGuesserSupported(): bool {
   

   Not sure we have return types in the coding standards yet, unsure if there should be a space or not before the colon?

2. +++ b/core/lib/Drupal/Core/File/MimeType/ExtensionMimeType.php
   @@ -0,0 +1,987 @@
   +  private function cacheMapping() {
   

   Does this need to be private? Should it be protected in case a subclass wants to use it?

3. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -156,13 +156,13 @@ class FileUploadResource extends ResourceBase {
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, $serializer_formats, LoggerInterface $logger, FileSystemInterface $file_system, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, AccountInterface $current_user, MimeTypeGuesserInterface $mime_type_guesser, Token $token, LockBackendInterface $lock, Config $system_file_config) {
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, $serializer_formats, LoggerInterface $logger, FileSystemInterface $file_system, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, AccountInterface $current_user, MimeTypesInterface $mime_type, Token $token, LockBackendInterface $lock, Config $system_file_config) {
   

   I think this and other similar constructor changes may need a BC layer?

thanks man,

All good points, I will work on 1, 2, 3 tonight

3, in particular is a really good insight.

#22.1) I have changed my position on this styling ...

I was going to for two spaces ( albeit inconsistently )

method() : array 

but now I have gone for the one space solution

method(): array 

I have updated the ProxyBuilder issued and I have given a more complete reasoning there.

https://www.drupal.org/project/drupal/issues/3125234#comment-13544797

For now I have just folded that patch back into this one.

#22.2 Yep I agree .. fixed.

#22.3 Good catch .. the added BC layer is correspondingly enforced with FileUploadResouceLegacyTest

Just to close up a thought I expressed in #13

The properties.

ExtensionMimeType::defaultMapping and Symfony\Component\Mime::map

now appear to provide the same function ... that is providing golden default for which drupal can override.

I am not acting upon this thought as the two list -- while trying to both provide a golden standard are different.

So I would create a mountain of conflict if I swapped one for the other.

https://github.com/symfony/mime/blob/master/MimeTypes.php

1. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -156,13 +157,16 @@ class FileUploadResource extends ResourceBase {
   +    if ($mime_type instanceof MimeTypeGuesserInterface) {
   +      @trigger_error(sprintf(__METHOD__ . '() The parameter $mime_type of type %s is deprecated in 9.1.x and the parameter type of %s will be enforced from Drupal 10.0.0. See https://www.drupal.org/node/3126004', MimeTypeGuesserInterface::class, MimeTypeInterface::class), E_USER_DEPRECATED);
   +    }
   +    $this->mimeType = $mime_type;
   
   @@ -254,7 +258,7 @@ public function post(Request $request, $entity_type_id, $bundle, $field_name) {
   -    $file->setMimeType($this->mimeTypeGuesser->guess($prepared_filename));
   +    $file->setMimeType($this->mimeType->guessMimeType($prepared_filename));
   

   The method name is different between the two interfaces, we now need a BC shim here that checks the interface before calling.

2. +++ b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php
   @@ -114,10 +114,10 @@ class TemporaryJsonapiFileFieldUploader {
   -  public function __construct(LoggerInterface $logger, FileSystemInterface $file_system, MimeTypeGuesserInterface $mime_type_guesser, Token $token, LockBackendInterface $lock, ConfigFactoryInterface $config_factory) {
   +  public function __construct(LoggerInterface $logger, FileSystemInterface $file_system, MimeTypesInterface $mime_type, Token $token, LockBackendInterface $lock, ConfigFactoryInterface $config_factory) {
   

   This needs BC as well in case someone overrides/extends this service - it is in the Controller namespace but it still has a service definition.

3. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
   @@ -75,19 +75,19 @@ class ThemeSettingsForm extends ConfigFormBase {
   -  public function __construct(ConfigFactoryInterface $config_factory, ModuleHandlerInterface $module_handler, ThemeHandlerInterface $theme_handler, MimeTypeGuesserInterface $mime_type_guesser, ThemeManagerInterface $theme_manager, FileSystemInterface $file_system) {
   +  public function __construct(ConfigFactoryInterface $config_factory, ModuleHandlerInterface $module_handler, ThemeHandlerInterface $theme_handler, MimeTypesInterface $mime_type, ThemeManagerInterface $theme_manager, FileSystemInterface $file_system) {
   

   I think form constructors also need BC?

On it, but not tonight.

a) Fixed tests

b) For those, at home, playing Drupal scrabble we have a new potential high score.

TemporaryJasonapiFileFieldUploaderLegacyTest

#26.3 was a question, and I too think we need it.

#26: 1, 2, 3 fixed

Thanks, the ProxyBuilder issue is RTBC, but I have left the code in here. I just want to see if the tests pass first.

Minor fixes.

[ If this return green, I will start posting two patch, one for commit and one with the ProxyBuilder patch included. ]

Locally I was testing with for example

./vendor/bin/phpunit -c core/ ./core/modules/jsonapi/tests/src/Unit/Controller/TemporaryJsonapiFileFieldUploaderLegacyTest.php

but testbot uses

php ./core/scripts/run-tests.sh --url http://nginx --sqlite test.sql --file ./core/modules/jsonapi/tests/src/Unit/Controller/TemporaryJsonapiFileFieldUploaderLegacyTest.php

which shows up all my goofy spelling mistakes in directory names and namespaces.

Its is not easy being green.

Congrats to catch it! Hope it green

Ok, I think now it is appropriate to carry 2 patches.

One with the ProxyBuilder stuff included, and one in the fullness of time to be committed.

Marking this RTBC but I think #3125234: ProxyBuilder compatibility with Symfony 5 should go in first.

I wish there was a cleaner way to do this but I can't see one, because Symfony has moved this around and renamed methods etc we have to catch up with them and this seems to be the only sane way.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -15,7 +15,6 @@
   -use Drupal\Core\File\MimeType\MimeTypeGuesser;
   
   @@ -595,7 +594,11 @@ public function preHandle(Request $request) {
   -    MimeTypeGuesser::registerWithSymfonyGuesser($this->container);
   +    // Deprecated: registering Guesser will no longer be needed in Drupal 10.
   +    if (class_exists('\Drupal\Core\File\MimeType\MimeTypeGuesser')) {
   +      // @codingStandardsIgnoreLine
   +      \Drupal\Core\File\MimeType\MimeTypeGuesser::registerWithSymfonyGuesser($this->container);
   +    }
   

   This change is unnecessary both DrupalKernel and MimeTypeGuesser are in the Drupal namespace. If we remove the MimeTypeGuesser we change this code.

2. +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
   @@ -154,6 +154,8 @@ public static function getSkippedDeprecations() {
   +      'The Drupal\Core\File\MimeType\MimeTypeGuesser is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Instead, use Drupal\Core\File\MimeType\MimeType. See https://www.drupal.org/node/3055193',
   +      'The Drupal\Core\File\MimeType\ExtensionMimeTypeGuesser is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Instead, use Drupal\Core\File\MimeType\ExtensionMimeType. See https://www.drupal.org/node/3055193',
   

   This doesn't look right. Adding stuff here shouldn't really happen for our own code.

3. I need to spend more time reviewing and thinking about this issue - but I'd like to search for a cleaner way before resorting to the solution in #33.

-    MimeTypeGuesser::registerWithSymfonyGuesser($this->container);
+    // Deprecated: registering Guesser will no longer be needed in Drupal 10.
+    if (class_exists('\Drupal\Core\File\MimeType\MimeTypeGuesser')) {
+      // @codingStandardsIgnoreLine
+      \Drupal\Core\File\MimeType\MimeTypeGuesser::registerWithSymfonyGuesser($this->container);
+    }

This change is unnecessary both DrupalKernel and MimeTypeGuesser are in the Drupal namespace. If we remove the MimeTypeGuesser we change this code.

Firstly, this is not my favourite of code.

The issue summary talks about doing this if possible .. we may have found the answer to be NO

Resolve the following SF4 Deprecations if possible:

Maybe another sees a more elegant way of proceeding, so I am just going to layout the problem under review.

I see the need to two distinct paths.

PATH A) Through the appropriate composer require command we start using Symfony5 stuff.
Then it is no longer possible or needed to register things. ( registerWithSymfonyGuesser )

PATH B) The Symfony4 legacy route ( call registerWithSymfonyGuesser() )

the if class_exists() is my hacky way choosing A or B.

When I got to #15 I asked in slack if anyone had a solution to the general problem

if( SYMFONY5 ) { // DO A } else {// Symfony 4 DO B }

and my patch reflects the response.

If the fallback position is to take out the IF statement that I will update the change record with a step by step guide including the appropriate composer require commands.

There is a final aspect I want to talk about.

Do I understand correctly .. At large scale companies like platform.sh and Acquia --- regard the DrupalKernel.php files as only a template -
a jumping off point which from which things will change.

If that is the case then we still need to add to DrupalKernel with a deprecation notice saying the template is changing - that registerWithSymfonyGuesser() is deprecated in D9 to be removed in D10.

When I looked online at the standardised deprecation templates -- this case was outside the scope considered. So Invented my own pattern

If anyone has a better "template" deprecation suggestion -- rather than just leaving a notice ... a trigger or an assert maybe please let me know.

Bumping this to critical since it blocks Drupal 10's release.

Do I understand correctly .. At large scale companies like platform.sh and Acquia --- regard the DrupalKernel.php files as only a template -
a jumping off point which from which things will change.

If that is the case then we still need to add to DrupalKernel with a deprecation notice saying the template is changing - that registerWithSymfonyGuesser() is deprecated in D9 to be removed in D10.

I'm not sure exactly what this is about, but we don't need to second guess what the Drupal hosting companies are doing. If something is covered by the backwards compatibility/deprecation policy we should try to stick to it, if not, then worst case when we break something, people can open an issue to discuss.

When I got to #15 I asked in slack if anyone had a solution to the general problem

if( SYMFONY5 ) { // DO A } else {// Symfony 4 DO B }

At the moment at least, we don't need to account for this. Drupal 9 is stuck on Symfony 4, and Drupal 10 will update to Symfony 5 (or even Symfony 6) as soon as it opens. It's OK if the patch to update to Symfony 5 and/or 6 needs some additional changes for things we've had to leave in for bc.

Making it possible/easier to run Symfony 5 with Drupal 9 is in #3020303: Consider using Symfony flex to allow switching between major Symfony versions but we're quite a long way off being able to do that.

I'm not sure exactly what this is about,

It is just the problems of having a remote conversion spread out over a period of months..

The verbiage on my part was just to justify this breadcrumb in Kernel.php

 // Override of Symfony's MIME type guesser singleton.

As usual from me too much talking ... less clarity.

--------------------------------------------------------------------------

At the moment at least, we don't need to account for this. Drupal 9 is stuck on Symfony 4, and Drupal 10 will update to Symfony 5 (or even Symfony 6) as soon as it opens. It's OK if the patch to update to Symfony 5 and/or 6 needs some additional changes for things we've had to leave in for bc.

Sorry If I am parsing this paragraph too closely

but the first statement "we don't need to account for this" is immediately contradicted by the second statement
saying it is OK to add stuff for backwards compatibility.

That is the point to the class_exists() check we must call registerWithSymfonyGuesser() only in a Symfony4 environment and explicitly not in a symfony5/6 situation.

a )Is the outcome of this that this patch should be postponed until another D9 patch upgrades to Symfony 5 or 6 ?

b) I appreciate that Alex Pott has asked for other solutions to the problem - and so the current patch is not viable. I have been thinking about other ways to d this .. but I think (a) or the bc fix are the only 2 ways forward

Anyway maybe I am too close to this to see another way forward, and hope my words are received only as a discussion not a argument.

but the first statement "we don't need to account for this" is immediately contradicted by the second statement
saying it is OK to add stuff for backwards compatibility.

Not quite :)

We need to provide backwards compatibility for contrib/custom code in Drupal 9 minor releases (for things that are covered by our bc policy).

However, it's OK if we need to make small changes to core when we actually update to Symfony 5.

Trying to get this going again:

#35.1 reverted this change.

#35.2 this is because the deprecation error was triggered when the class is loaded. I moved the deprecation triggering to the two public methods on the class that consumers would actually call. We'll need to adjust the phpdoc for those methods (and the class as a whole probably) if this approach works, but see what DrupalCI says.

#35.3 on the overall approach to adding a completely new service and deprecating the old one, the tricky thing here seems to be the change in signature of the ::addGuesser() method. However, this method is on neither the new or old interfaces.

So... I wonder if we're able to have the existing MimeTypeGuesser class implement both the old and new interfaces, but renaming some non-interface methods. This seems possible, but want to see if there's any fall-out from the above two changes before attempting that since it'd be a larger change to the patch.

Same change needed for ExtensionMimeTypeGuesser.

Fixing #43 and 2 coding standard violations.

Looks the patch size does not match and Something was wrong. Checking

Rebuild the patch in a new environment. Must be something wrong with my git config.

The culprit is the following in ~/.gitconfig

[diff]
  renames = copies

Can not remember when and why I added it. Sorry for the noises here.

Trying the second part of #41 - i.e. keeping the existing classes and implementing the new methods/interfaces. This will fail, but tests/Drupal/KernelTests/Core/File/MimeTypeTest.php passes. We'll need to update all the other calls from ::guess() to ::guessMimeType() in core, and probably remove some type hints.

No interdiff because it's a new approach.

Apparently no patch at all, uploading this time...

Couple of silly mistakes in that one, new patch. Still incomplete but want to see exactly how it fails before keeping going.

OK that seems to be failing the right way, now updating calls to ::guess() with ::guessMimeType(), removing some type hints from constructors etc.

OK so #53 turned out to be easier than I thought.

Comparing the two approaches now:

#47: we add completely new classes and services matching the new Symfony/Mime API. This makes for a cleaner break in Drupal 10 because we can just delete the old classes (more or less). However it requires a lot of duplication and doesn't seem to allow us to remove the deprecation suppressions. Any code wishing to support both interfaces being injected has to remove type hinting.

#53: we implement both the new and old interfaces in the existing classes, adding our own deprecation message when code is calling the old ::guess() method. Drupal 10 will need to un-implement the old interface and remove the old method. Contrib code can update to the new method, any code wishing to support both interfaces being injected has to remove type hinting (in core this is only in constructors). We're not able to remove the deprecation suppressions because the old interfaces and classes are still in use.

Overall: both allow us to be forwards compatible to about the same extent. Both have similar challenges in that we can't completely remove suppression of Symfony deprecations, but allow us to notify contrib of the change. I think #53 is slightly preferable because it's a much smaller change and doesn't have any significant additional drawbacks.

Being the maintainer of the Sophron module, that overrides the extension guesser on the service definition level, I'd rather vote for the approach in #53, that would allow to just add the new method guessMimeType, and not force to override two services, the deprecated one and the new one.

In both cases, it looks like that the module will definitely have to implement guessMimeType for sites using the Sophron guesser module, since core will start calling it, and a new release of the module will be needed. I guess any module working with MIME guessers will need to, anyway.

+++ b/core/tests/Drupal/KernelTests/Core/File/MimeTypeTest.php
@@ -46,13 +46,13 @@ public function testFileMimeTypeDetection() {
+        $this->assertSame($output, $expected, new FormattableMarkup('Mimetype for %input is %output (expected: %expected).', ['%input' => $prefix . $input, '%output' => $output, '%expected' => $expected]));
...
+      $this->assertSame($output, $expected, new FormattableMarkup('Mimetype (using default mappings) for %input is %output (expected: %expected).', ['%input' => $input, '%output' => $output, '%expected' => $expected]));

@@ -86,9 +86,24 @@ public function testFileMimeTypeDetection() {
+      $this->assertSame($output, $expected, new FormattableMarkup('Mimetype (using passed-in mappings) for %input is %output (expected: %expected).', ['%input' => $input, '%output' => $output, '%expected' => $expected]));

1) In PHPUnit, assertSame requires the expected value as the first argument, and the actual as the second. This would format nicely the standard failure message in case of test failure.

2) Can we avoid using FormattableMarkup for the custom failure message, and use string concatenation or sprintf instead?

3) Also, would be good to standardize on the tone of the custom failure message and/or decide that a message is actually not needed, see #3131946: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages.

Fixing the assertSame() calls including removing the custom assertion message.

This is technically out of scope for the patch except it's required for the deprecation test to not have random deprecated code usages, which brings it into scope.

In both cases, it looks like that the module will definitely have to implement guessMimeType for sites using the Sophron guesser module, since core will start calling it, and a new release of the module will be needed.

We could potentially do instanceof or method_exists() checks in all the places core calls ::guess()/::guessMimeType() - and trigger a deprecation error from those places too. If we do that, I think it should be a 'best effort' deprecation - i.e. add the code but no test coverage for the conditional, since it'll be scattered around and only help if there's no contrib modules calling ::guessMimeType().

Here's a start on #58 - just did one location for now so we can discuss the wording etc.

Check the wording and also not having syntax errors helps.

+++ b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php
@@ -173,7 +172,13 @@ public function handleFileUploadForField(FieldDefinitionInterface $field_definit
+    if ($this->mimeTypeGuesser instanceof '\Symfony\Component\Mime\MimeTypeGuesserInterface') {
+      $file->setMimeType($this->mimeTypeGuesser->guessMimeType($prepared_filename));
+    }
+    else {
+      @trigger_error('\Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface is deprecated in drupal:9.1.0 and will be removed in drupal:10.0.0, implement \Symfony\Component\Mime\MimeTypeGuesserInterface instead. See https://www.drupal.org/node/3133341', E_USER_DEPRECATED);
+      $file->setMimeType($this->mimeTypeGuesser->guess($prepared_filename));
+    }

Would it make sense to add a static helper somewhere, put the checks in there and in code go for sth like

+++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
@@ -351,7 +351,7 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
-        $image->setMimeType(\Drupal::service('file.mime_type.guesser')->guess($path));
+        $image->setMimeType(MimeType::guess(\Drupal::service('file.mime_type.guesser'), $path));

?

@mondrake I'm not sure about the static helper - this is cruft we need to add in less than ten places in core, so roughly 20-30 lines of code, and can remove again for 10.x. If we add a helper, that's a new API that we'd have to add undeprecated, then probably deprecate in Drupal 10 for Drupal 11.

This should fix the failure in #60. Once it's green I'll apply the same change to the other spots in core, and we can see how bad it looks.

Here we go. #61 would definitely make the patch look a bit nicer, just not sure about adding a helper to remove it again.

...

typos galore.

+++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
@@ -351,7 +351,14 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
+        if ($guesser instanceof \Symfony\Component\Mime\MimeTypeGuesserInterface) {
+          $image->setMimeType($guesser->guessMimeType($path));
+        }
+        else {
+          $image->setMimeType($guesser->guessMimeType($path));
+          @trigger_error('\Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface is deprecated in drupal:9.1.0 and will be removed in drupal:10.0.0, implement \Symfony\Component\Mime\MimeTypeGuesserInterface instead. See https://www.drupal.org/node/3133341', E_USER_DEPRECATED);
+        }

Reading this patch for the first time in a couple weeks, I'd say this deserves a static helper.
But reading #63, the added burden of waiting til Drupal 11 to remove this doesn't seem worth it.

I'd propose documenting VERY CLEARLY in the CR that repeatable part, and be done with it.

Made some changes to the CR: https://www.drupal.org/node/3126004/revisions/view/11816439/12053082

1. +++ b/core/lib/Drupal/Core/ProxyClass/File/MimeType/MimeTypeGuesser.php
   @@ -78,11 +86,19 @@ public function guess($path)
   -        public function addGuesser(\Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface $guesser, $priority = 0)
   +        public function addGuesser(\Symfony\Component\Mime\MimeTypeGuesserInterface $guesser, $priority = 0)
   

   Is this a BC break? It also happens in MimeTypeGuesser but it's not obvious from the patch because the short class name is the same but the FQCN and the typehint actually change.

2. +++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
   @@ -351,7 +351,14 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
   +        if ($guesser instanceof \Symfony\Component\Mime\MimeTypeGuesserInterface) {
   +          $image->setMimeType($guesser->guessMimeType($path));
   +        }
   +        else {
   +          $image->setMimeType($guesser->guessMimeType($path));
   +          @trigger_error('\Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface is deprecated in drupal:9.1.0 and will be removed in drupal:10.0.0, implement \Symfony\Component\Mime\MimeTypeGuesserInterface instead. See https://www.drupal.org/node/3133341', E_USER_DEPRECATED);
   +        }
   

   The second call here should just be ->guess()?

#1. Good spot. That method isn't on an interface, so I think we can just remove the type hint there.

#2. Yep.

Discussed this with @alexpott.

The test groups can't be discovered due to this error:

Symfony\Component\DependencyInjection\Exception\LogicException: Service consumer 'drupal.proxy_original_service.file.mime_type.guesser' class method Drupal\Core\File\MimeType\MimeTypeGuesser::addGuesser() has to type-hint an interface. in /Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php:149

So the tag handler pass depends on the method having a type hint, which we're trying to remove to avoid a bc break.

The only way to deal with this appears to be to add support for two interfaces to TaggedHandlersPass. Here's a patch which tries that - for the moment it hard-codes support for mime_type_guesser. We could potentially (but ideally in a follow-up...) add generic support for this - for example by allowing two interfaces to be specified in the service definition.

Can we add a temporary "combined" interface that extends both interfaces? Multiple inheritance of interfaces is allowed in PHP.

@longwave I considered that but the problem is that guessers would need to implement that new combined interface, so it would be the same as forcing them to switch to the new one.

Opened #3169771: Add generic support for deprecating interfaces used for tagged services.

Let's add an @see pointing to the followup then I think this is ready to go.

Addressing #77.

I also think this is good, except the below nitpicks.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
   @@ -139,11 +141,19 @@ protected function processServiceCollectorPass(array $pass, $consumer_id, Contai
   +      // @todo: remove in Drupal 10 or make this handling generic for future
   +      // situations with similar interface changes.
   +      // @see https://www.drupal.org/project/drupal/issues/3169771
   

   Mega-nitpick: No : after todo, capitalize Remove, indent the following lines two spaces, change @see to See as a new sentence, end the whole thing with .

   (@see can't be parsed inline)

2. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
   @@ -139,11 +141,19 @@ protected function processServiceCollectorPass(array $pass, $consumer_id, Contai
   +        $interface = '\Symfony\Component\Mime\MimeTypeGuesserInterface';
   +        $deprecated_interface = 'Symfony\Component\HttpFoundation\MimeType\MimeTypeGuesserInterface';
   

   Any reason not to have these as use statements and then do ClassName::class instead of strings?

3. +++ b/core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php
   @@ -889,6 +890,14 @@ public function __construct(ModuleHandlerInterface $module_handler) {
   +    @trigger_error(__METHOD__ . '() is deprecated in drupal:9.1.0 and will be removed before drupal:10.0.0, use ::guessMimeType() instead. See https://www.drupal.org/node/3133341', E_USER_DEPRECATED);
   

   Nitpick: here and in the other deprecations, the first sentence should end with 10.0.0 and then the next phrase should be a new sentence.

4. +++ b/core/modules/file/file.module
   @@ -945,7 +945,14 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +  if ($guesser instanceof \Symfony\Component\Mime\MimeTypeGuesserInterface) {
   

   Here and elsewhere: any particular reason to skip using use statements?

@tim.plunkett I went for FQDN instead of use statements so that the bc cruft is completely isolated to the code block - i.e. so that when it's removed, the person doing that doesn't also have to go up to the top of the file and remove the unused use statement (or as likely, leave it in, then a further issue to remove it). So, it was an intentional decision but not one I feel strongly about.

#79.1 and #79.3 both good points but will wait and see if anyone else has views either way on #2 and #4 before a re-roll.

I think we should use use statements:

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
@@ -139,11 +141,19 @@ protected function processServiceCollectorPass(array $pass, $consumer_id, Contai
+        $deprecated_interface = 'Symfony\Component\HttpFoundation\MimeType\MimeTypeGuesserInterface';

This should be Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface. An equivalent incorrect use statement would be highlighted by PhpStorm. Should this have test coverage?

Also, when we remove the legacy code, we will be adding back typehints in the instanceof cases, so the use statements will be needed then anyway.

+++ b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php
@@ -73,7 +72,7 @@ class TemporaryJsonapiFileFieldUploader {
+   * @var \Symfony\Mime\MimeTypeGuesserInterface

This should be \Symfony\Component\Mime\MimeTypeGuesserInterface.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
   @@ -130,6 +130,8 @@ protected function processServiceCollectorPass(array $pass, $consumer_id, Contai
   +      // Determine the ID.
   +      $deprecated_interface = FALSE;
   

   I think this should be outside of the loop before the foreach.

2. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
   @@ -139,11 +141,19 @@ protected function processServiceCollectorPass(array $pass, $consumer_id, Contai
   +      // Special handling for the mime_type_guesser tag. The interface has
   +      // changed upstream in Symfony. We have to support both interfaces.
   +      // @todo: remove in Drupal 10 or make this handling generic for future
   +      // situations with similar interface changes.
   +      // @see https://www.drupal.org/project/drupal/issues/3169771
   +      elseif ($tag === 'mime_type_guesser') {
   +        $interface = '\Symfony\Component\Mime\MimeTypeGuesserInterface';
   +        $deprecated_interface = 'Symfony\Component\HttpFoundation\MimeType\MimeTypeGuesserInterface';
   +      }
   

   I think this should be outside the loop where the // Determine the ID was.

3. I agree with @longwave we need test coverage that the file.mime_type.guesser services gets services tagged with mime_type_guesser added regardless of the interface they implement.

This should fix points from both #79 and #81.

Also, when we remove the legacy code, we will be adding back typehints in the instanceof cases, so the use statements will be needed then anyway.

This is only true for the cases which are using dependency injection, but that is about half the places checking instanceof so yeah avoiding the use statements wasn't saving much at all. All changed.

Edit: crossposted with #82...

Here's some test coverage, which also uncovered a bug or too.

#82.2 we can't do - this needs to take precedence over the other checks.

Re #82.2 and #86 - that's why I wrote

// Determine the ID was.

What I didn't realise that the // Determine the ID had moved to the just before the loop. I was trying to suggest the hardcoding override should go at the end of the loop. But the latest check in #86 looks okay. That said one advantage to moving

if ($tag === 'mime_type_guesser') {
  $interface = MimeTypeGuesserInterface::class;
  $deprecated_interface = LegacyMimeTypeGuesserInterface::class;
}

after the loop is that removal in D10 will have less change because the internals of the loop will not have to change.

Ah OK so the problem with putting it after the loop is we can't let this run, I tried to do it before and it blows up dramatically.

  else {
        $extra_params[$param->getName()] = $pos;
      }

What we could do is add the conditional inside this check - makes it happen later but extra level of nesting. I don't really think that's better or worse.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
@@ -129,8 +131,18 @@ protected function processServiceCollectorPass(array $pass, $consumer_id, Contai
+      if ($pos === 0 && $tag === 'mime_type_guesser') {

Why do we only need to do this when $pos is 0?

You could also write the block as

      if ($pos === 0 && $tag === 'mime_type_guesser') {
        $interface = MimeTypeGuesserInterface::class;
        $deprecated_interface = LegacyMimeTypeGuesserInterface::class;
        continue;
      }

and then avoid having to modify the following if into elseif, but not sure it's worth it.

I missed #88. Thanks for explaining. #86 is looking good.

@longwave it doesn't really matter, the method only has one argument, it just seemed more explicit - i.e. if we added a second param to addGuesser() this stops that breaking

Oh I see, thanks for explaining. Nothing more from me, just the coding standards issues and that test failure to fix now.

This should fix the coding standards issues.

I found one issue with the @expectedDeprecation annotation but that doesn't seem to be the problem, feels like I'm missing something obvious like a typo. Uploading progress for now...

1. +++ b/core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php
   @@ -889,6 +890,14 @@ public function __construct(ModuleHandlerInterface $module_handler) {
   +    @trigger_error(__METHOD__ . '() is deprecated in drupal:9.1.0 and will be removed before drupal:10.0.0. Use ::guessMimeType() instead. See https://www.drupal.org/node/3133341', E_USER_DEPRECATED);
   
   +++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeGuesser.php
   @@ -67,30 +68,45 @@ public function guess($path) {
   +    @trigger_error(__METHOD__ . '() is deprecated in drupal:9.1.0 and will be removed before drupal:10.0.0, use ::guessMimeType() instead. See https://www.drupal.org/node/3133341', E_USER_DEPRECATED);
   
   +++ b/core/tests/Drupal/KernelTests/Core/File/MimeTypeTest.php
   @@ -86,9 +84,24 @@ public function testFileMimeTypeDetection() {
   +   * @expectedDeprecation Drupal\Core\File\MimeType\MimeTypeGuesser::guess() is deprecated in drupal:9.1.0 and will be removed before drupal:10.0.0. Use ::guessMimeType() instead. See https://www.drupal.org/node/3133341
   

   One of these still has the comma after 10.0.0 instead of a new sentence, that's the deprecation fail

2. +++ b/core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php
   @@ -889,6 +890,14 @@ public function __construct(ModuleHandlerInterface $module_handler) {
   +  public function guessMimeType($path) : ?string {
   
   @@ -927,4 +936,11 @@ public function setMapping(array $mapping = NULL) {
   +  public function isGuesserSupported(): bool {
   

   Last nit is over the space between ) and :

   AFAIK we standardized on no space (setUp(): void comes to mind)

Don't think the comma is the cause of the error but it's a good spot anyway.

OK figured it out. For unit tests, the entire test can be legacy, but just one method can't be. So moving the legacy test to its own test class.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
   @@ -129,8 +131,18 @@ protected function processServiceCollectorPass(array $pass, $consumer_id, Contai
   +    $deprecated_interface = FALSE;
        foreach ($params as $pos => $param) {
   -      if ($param->getClass()) {
   +      // Special handling for the mime_type_guesser tag. The interface has
   +      // changed upstream in Symfony. We have to support both interfaces.
   +      // @todo Remove in Drupal 10 or make this handling generic for future
   +      //   situations with similar interface changes.
   +      //   See https://www.drupal.org/project/drupal/issues/3169771.
   +      if ($pos === 0 && $tag === 'mime_type_guesser') {
   +        $interface = MimeTypeGuesserInterface::class;
   +        $deprecated_interface = LegacyMimeTypeGuesserInterface::class;
   +      }
   +      elseif ($param->getClass()) {
            $interface = $param->getClass();
          }
          elseif ($param->getName() === 'id') {
   @@ -152,7 +164,9 @@ protected function processServiceCollectorPass(array $pass, $consumer_id, Contai
   
   @@ -152,7 +164,9 @@ protected function processServiceCollectorPass(array $pass, $consumer_id, Contai
            $method_name,
          ]));
        }
   -    $interface = $interface->getName();
   +    if (!is_string($interface)) {
   +      $interface = $interface->getName();
   +    }
   

   Instead of polluting our generic tagged handler pass with this should we just remove the mime_type_guesser service_collector tag from file.mime_type.guesser and add a new compiler pass to CoreServiceProvider that is just for this edge-case?

2. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
   @@ -161,7 +175,10 @@ protected function processServiceCollectorPass(array $pass, $consumer_id, Contai
   +        // Special handling for $deprecated_interface.
   +        if (!$deprecated_interface || !is_subclass_of($handler->getclass(), $deprecated_interface)) {
   +          throw new LogicException("Service '$id' for consumer '$consumer_id' does not implement $interface.");
   

   that would fix this special casing too

#97.1 is a great idea.

Updated patch that does that, and also (hopefully) fixes the lingering false positive deprecation error.

OK so MimeTypePassTest passes locally, it passes on jenkins if you ctrl-f for the test name, but then DrupalListener::endTest causes the overall test run to fail.

I think that the problem is that test discovery loads all classes in the \Drupal\Tests namespace, including the dummy class used for testing, which triggers the deprecation error outside of any tests, which then causes a failure.

Credit to @mondrake where a similar issue was discovered and worked around in #3169171-5: Fix AssertHelperTrait deprecation - the fix is to move the dummy class outside of the discovery namespace so it is only loaded when the test actually runs.

@longwave is 100% correct. You can get it to fail locally if you use --group for phpunit instead of passing in a class/directory.

As this is at least the second time this has happened we should probably spin off a new issue to see if we can skip deprecation warnings during test discovery and then we don't need the messy workaround and other people won't run into the same issue.

I just got to the same place then came back and saw this.

There is one other possible workaround, which is to add the deprecation message to the list of skipped deprecations. This lets the test exist as it should have been written, but obviously silly to have to add it to the list. Uploading for comparison.

edit: the silly debugging is only in the interdiff, not the patch.

Opened the spin-off #3170423: Suppress deprecation warnings during test discovery to avoid failures due to test classes.

#103 hmm not sure, IIRC at discovery time the deprecation silencing is not yet effective. But let's see.

So #103 works and is cleaner than my attempt, otherwise I think this is RTBC now so let's get this in and remove the skipped deprecation in the spinoff?

So I thought I'd give this a spin with https://www.drupal.org/project/mimeinfo and it breaks hard with

Declaration of Drupal\mimeinfo\File\MimeType\MimeTypeGuesser::addGuesser(Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface $guesser, $priority = 0) should be compatible with Drupal\Core\File\MimeType\MimeTypeGuesser::addGuesser($guesser, $priority = 0)

The reason this fails that that module is overriding the core base class to do:

  /**
   * {@inheritdoc}
   */
  public function addGuesser(MimeTypeGuesserInterface $guesser, $priority = 0) {
    // Symfony's guessers has non-interfaced "isSupported" method to check that
    // environment supports guessing mechanism. Allow all guessers define same
    // the method for same purposes. Otherwise consider that guesser is allowed
    // to use.
    /* @see \Symfony\Component\Mime\FileBinaryMimeTypeGuesser::isSupported() */
    if (\method_exists($guesser, 'isSupported') ? $guesser::isSupported() : TRUE) {
      parent::addGuesser($guesser, $priority);
    }

    return $this;
  }

I think this suggests an interesting way forward for us. We can leave the addGuesser method alone and deprecate it. And we can add a new addMimeGuesser that accepts the new interface. Then we can change our new service pass to call the correct method.

I think the new addMimeGuesser method should check the outcome of isGuesserSupported() as it is on the new interface before adding the guesser. This way we'll not break the mimeinfo module but allow guessers to implement either the legacy or the new interface in Drupal 9.

Here's an (untested) patch looking at #107.

I called the new method ::addMimeTypeGuesser(), ::addMimeGuesser() seemed too close to charades ;)

I hesitated on whether the new method should be type hinted or still handle both interfaces. It's not very much logic to check the interface in the compiler pass, and then decide which method to call, so I went for that approach (but only after writing the first version, so the interdiff is first for that version, then against that version).

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/MimeTypePass.php
   @@ -0,0 +1,70 @@
   +    // Sort all handlers by priority.
   +    arsort($handlers, SORT_NUMERIC);
   +
   +    // Add a method call for each handler to the consumer service
   +    // definition.
   +    foreach ($handlers as $id => $priority) {
   +      $arguments = [];
   +      $arguments[0] = new Reference($id);
   +      if (isset($priority_pos)) {
   +        $arguments[$priority_pos] = $priority;
   +      }
   +      if (isset($id_pos)) {
   +        $arguments[$id_pos] = $id;
   +      }
   +      // Sort the arguments by position.
   +      ksort($arguments);
   +      if (is_subclass_of($interfaces[$id], $interface)) {
   +        $consumer->addMethodCall('addMimeTypeGuesser', $arguments);
   +      }
   +      else {
   +        $consumer->addMethodCall('addGuesser', $arguments);
   +      }
   +    }
   

   Given we sort the guessers in MimeTypeGuesser is this sorting necessary here? I guess it is duplicating what is in TaggedHandlers. Feels funny to be always sorting but I guess that it is an existing issue and at least ensure consistency. /shrug.

2. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -477,7 +478,16 @@ function responsive_image_get_mime_type($image_style_name, $extension) {
   +  $guesser = \Drupal::service('file.mime_type.guesser');
   +  if ($guesser instanceof MimeTypeGuesserInterface) {
   +    return $guesser->guessMimeType($fake_path);
   +  }
   +  else {
   +    @trigger_error('\Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface is deprecated in drupal:9.1.0 and will be removed in drupal:10.0.0. Implement \Symfony\Component\Mime\MimeTypeGuesserInterface instead. See https://www.drupal.org/node/3133341', E_USER_DEPRECATED);
   +    return $guesser->guess($fake_path);
   +  }
   +
   +  return Drupal::service('file.mime_type.guesser.extension')->guessMimeType($fake_path);
   

   The three returns look odd. I think we should be only using the file.mime_type.guess.extension service here. One of the returns is unreachable.

Needs work for point 2

#1 yeah I just tried to change as little as possible from TaggedHandlers.

Patch should resolve #2.

+++ b/core/modules/responsive_image/responsive_image.module
@@ -477,7 +478,14 @@ function responsive_image_get_mime_type($image_style_name, $extension) {
-  return Drupal::service('file.mime_type.guesser.extension')->guess($fake_path);
+  $guesser = \Drupal::service('file.mime_type.guesser.extension');
+  if ($guesser instanceof MimeTypeGuesserInterface) {
+    return $guesser->guessMimeType($fake_path);
+  }
+  else {
+    @trigger_error('\Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface is deprecated in drupal:9.1.0 and will be removed in drupal:10.0.0. Implement \Symfony\Component\Mime\MimeTypeGuesserInterface instead. See https://www.drupal.org/node/3133341', E_USER_DEPRECATED);
+    return $guesser->guess($fake_path);
+  }

So we're okay to swap this from using the extension guesser directly? I think this needs to use the extension guesser directly because it is using a fake file and not a real one. Also the else is not necessary.

I think i'd do something like:

if (!$guesser instanceof MimeTypeGuesserInterface) { 
 @trigger_error('\Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface is deprecated in drupal:9.1.0 and will be removed in drupal:10.0.0. Implement \Symfony\Component\Mime\MimeTypeGuesserInterface instead. See https://www.drupal.org/node/3133341', E_USER_DEPRECATED);
  return $guesser->guess($fake_path);
}
return $guesser->guessMimeType($fake_path);

because then in D10 we're only removing complete lines.

So we're okay to swap this from using the extension guesser directly?

No that was a mistake in the original patch, but it's switched back in #111. Only difference is assigning it to a variable now.

Also the else is not necessary.

Good point.

Hi there. FWIW, I have tested #111 with the Sophron module on D9.1 and PHP 7.4 and, apart from deprecations, it still passes its own tests. https://travis-ci.org/github/mondrake/d8-unit/builds/728013068

+++ b/core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php
@@ -3,12 +3,13 @@
-use Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface;
+use Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface as LegacyMimeTypeGuesserInterface;

sorry just a question, I can't understand what the plan will be when Symfony will eventually be upgraded to 5.x: Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface is actually no longer existing there, so what will be the BC approach? Make a copy of it somewhere?

@mondrake we'd stop implementing the HttpFoundation component version of the interface, and implement only the MimeType version instead.

thanks, do I understand correctly Drupal 9 will remain on Symfony 4 for its entire cycle?

@mondrake yes that's right. IMO it would be nice to be able to allow people to composer update to Symfony 5 if they want to, but we aren't there yet with min/max testing etc.

So I think this can be set back to RTBC at this stage?

I can also confirm that the mimeinfo project's tests now only report deprecations and pass.

I think this is very nearly ready.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/MimeTypePass.php
@@ -0,0 +1,70 @@
+/**
+ * Adds @mime_type_guesser tagged services to handle forwards compatibility.
+ */
+class MimeTypePass implements CompilerPassInterface {

I think this needs to marked @internal and deprecated as will be removing this in Drupal 10 too - the file.mime_type.guesser service can then use the regular TaggedHandlersPass to have addMimeTypeGuesser called.

OK I think those are already internal-by-default, but it doesn't hurt to make all this explicit with new code. Back to RTBC since this is just two lines of boilerplate comments.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/MimeTypePass.php
@@ -11,6 +11,10 @@
+ * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. No direct
+ * replacement is provided. See https://www.drupal.org/node/3133341.

IIRC @see should be next line and without dot at the end, could be fixed on commit

#124 No inline @see, this is correct. Though the second line should be indented two spaces.

EDIT: I misread, I was thinking of inline comments //

Fixed CS and deprecation https://www.drupal.org/core/deprecation#how-class

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/MimeTypePass.php
@@ -36,7 +39,7 @@ public function process(ContainerBuilder $container) {
-          throw new LogicException("Service '$id' for consumer '$consumer_id' does not implement $interface.");
+          throw new LogicException("Service '$id' does not implement $interface.");

Good catch on the non-existent variable :) I thought we could also say "does not implement $interface or $deprecated_interface" here, but we don't really want people to use $deprecated_interface, so this is fine as is.

Committed c8cc155 and pushed to 9.1.x. Thanks!

diff --git a/core/lib/Drupal/Core/DependencyInjection/Compiler/MimeTypePass.php b/core/lib/Drupal/Core/DependencyInjection/Compiler/MimeTypePass.php
index 68fe61fa86..f20edc0956 100644
--- a/core/lib/Drupal/Core/DependencyInjection/Compiler/MimeTypePass.php
+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/MimeTypePass.php
@@ -38,7 +38,7 @@ public function process(ContainerBuilder $container) {
       $handler = $container->getDefinition($id);
       if (!is_subclass_of($handler->getClass(), $interface)) {
         // Special handling for $deprecated_interface.
-        if (!is_subclass_of($handler->getclass(), $deprecated_interface)) {
+        if (!is_subclass_of($handler->getClass(), $deprecated_interface)) {
           throw new LogicException("Service '$id' does not implement $interface.");
         }
       }

Fixed camelcase issue on commit. cspell++

Is there advice here on how contributed modules that provide mime type guessers should provide compatibility with both Drupal 9.0 and 9.1 now with the new interfaces? This doesn't seem to be covered in the change notice. We've run into this with #3185015: Remote Stream Wrapper does not work after upgrade drupal 9.

#131 here's what I did in the Sophron module: https://git.drupalcode.org/project/sophron/commit/236f1ce

Note that the proxy classes need to be updated as well
https://git.drupalcode.org/project/sophron/commit/bd0a3e0

See #3180540: Sophron guesser module failing on D9.1.

Hope this helps.

I ran into an issue with this update, where I couldn't run drush cr -y

PHP Fatal error: Declaration of Drupal\Core\ProxyClass\File\MimeType\ExtensionMimeTypeGuesser::guessMimeType($path): string must be compatible with Symfony\Component\Mime\MimeTypeGuesserInterface::guessMimeType(string $path): ?string in /Users/nathanandersen/dev/mdb/web/core/lib/Drupal/Core/ProxyClass/File/MimeType/ExtensionMimeTypeGuesser.php on line 15

Adding "string" type declaration to Drupal\Core\ProxyClass\File\MimeType\ExtensionMimeTypeGuesser::guessMimeType and Drupal\Core\ProxyClass\File\MimeType\ExtensionMimeTypeGuesser::guessMimeType($path)

fixed it. Is there a known issue for this?

Update:

I went ahead and created an issue: #3190265: guessMimeType declaration not compatible with Symfony interface