ExtensionMimeTypeGuesser breaks other mime_type_guesser services

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()

I don't really have an opinion on which approach is better, but this looks good to me as well. Updated the change notice for the new approach, so as far as I can tell this is good to go again.

@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 discussed and 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.

Automatically closed - issue fixed for 2 weeks with no activity.

FTR, fixing this issue created an E_DEPRECATED warning elsewhere which has not been spotted by the test suite on D.o: 🐛 Deprecated function: explode(): Passing null to parameter #2 ($string) of type string is deprecated in Drupal\file\Plugin\Field\FieldFormatter\FileMediaFormatterBase::mimeTypeApplies() (line 171 of Plugin/Field/FieldFormatter/FileMediaFormatterBase.php) Needs work

Although it breaks functional tests on downstream... ;S 🐛 E_DEPRECATED warnings makes functional tests fail Active