ExtensionMimeTypeGuesser breaks other mime_type_guesser services

Thanks for sharing this info.
I created a small patch to make ExtensionMimeTypeGuesser::guess() adhere to its interface.
This will probably break some existing tests but it does fix the fallback to other mime_type_guesser services.

The failed tests need fixing.

Next to the already outlined benefit (in the ticket description), this change also allows to differentiate between a detected (guessed) 'application/octet-stream' mime type and no detected one.

we could add a FallbackMimeTypeGuesser that always returns the default and has a very low priority allowing others to intervene before the fallback

This seems like the best approach, as it avoids breaking backward compatibility while fixing this issue. Sites could also opt out by removing the fallback service if they wanted to for some reason.

We could also add a deprecation warning if the fallback is triggered to warn users that the default will be removed in Drupal 10.

Implemented a fallback guesser so other guessers can be inserted in the chain.

The existing test conveniently covers both the full chain and the existing extension guesser in separate cases so we can reuse that to test both the fallback and the NULL return.

The NULL return also means we have to fix the return type in the proxy class at the same time, which is currently missing the nullable part.

Testing a patch from this issue with Drupal core 9.4.x, you also need the patch from #3190265-28: guessMimeType declaration not compatible with Symfony interface otherwise Drupal won't allow a NULL return value.

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.

Re-rolled #11 in MR !8288.

For backwards compatibility concern can we add a deprecation for the return with a CR that says in D12 it will return null.

As per https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... we can't really trigger a deprecation warning or use the @deprecated annotation. Instead we need to document the behaviour change in the docblock, and add a @todo to switch to returning NULL in the next major.

For example:

   * @return string|null
   *   The mime type or NULL, if none could be guessed.
   *   For BC, this currently returns 'application/octet-stream' when none could
   *   be guessed but will return NULL in drupal:12.0.0.

and // @todo https://www.drupal.org/node/3156672 Return NULL in Drupal 12.0.0.

Tried a new approach to add a constructor param to trigger a deprecation.

Hiding patch files

Changed deprecation to 12 vs 11. But deprecation looks good.

Why do we need the deprecation here? The fallback means the end result is the same if you use the mime type guesser service properly, IMHO if you are calling file.mime_type.guesser.extension directly you are on your own.

@longwave I thought we were doing this issue to fix the problem of guessers that come after.

Thus, any mime type guesser that would otherwise come afterwards is ignored.

However, I think we can use priority to allow workarounds.

Reverted.

Hmm. I'm having second thoughts about reverting. I think we need to return NULL.

Sorry for all the noise. I think I went back and forth with this too many times, and made changes I shouldn't have.

I'm wondering whether adding a fallback guesser is really necessary and whether the best place to add the fallback is in \Drupal\Core\File\MimeType\MimeTypeGuesser::guessMimeType()

Agree there is no need for a FallbackMimeTypeGuesser if we can return 'application/octet-stream' by default from \Drupal\Core\File\MimeType\MimeTypeGuesser::guessMimeType()

@tstoeckler the advantage of this approach is now we don't need to be concerned with enforcing which mime type guesser comes last. Also less services.

Given this is a bug fix but also a minor behaviour change let's not commit this to 10.3, but I think it can go in 10.4 and 11.0.

Committed and pushed cc9baee953 to 11.x and 98eea842a3 to 11.0.x and 2d42a30bba to 10.4.x. Thanks!

Will update and publish the change record.

Discussed with @longwave and @alexpott. It's weird to have a behavior change of any kind in 10.4 and 11.0 but not 10.3 -- 10.4 is supposed to be a maintenance minor that limits disruptive bugfixes, and 11.0 is almost-RC-ish as of the week this was committed.

We agreed the disruption is minimal enough to backport to 10.3.x, which resolves potential 10.3/11.0 parity issues with this API. There is a CR, so the few projects that might be affected can get the info they need.

If this does cause more serious issues for contrib or sites, please open a followup issue linking this one and tag it "Contributed project blocker" if appropriate. Thanks!

FTR, Sophron module was adjusted to mirror this.

FTR, fixing this issue created an E_DEPRECATED warning elsewhere which has not been spotted by the test suite on D.o: #3466462: Fix handling of unknown file extensions in FileMediaFormatterBase

Although it breaks functional tests on downstream... ;S #3467081: E_DEPRECATED warnings makes functional tests fail

Amending attribution.