Handle NULL mime types in template_preprocess_file_link()

I created the Patch for it.

Hi,
I can reproduce the issue in Drupal 10.3.x, when I attempt to view the page with a file reference to a file with NULL as the mimetype I got this error TypeError: Drupal\file\IconMimeTypes::getIconClass(): Argument #1 ($mimeType) must be of type string, null given, called in /var/www/html/core/modules/file/file.module on line 774 in Drupal\file\IconMimeTypes::getIconClass() (line 21 of core/modules/file/src/IconMimeTypes.php).

I tested the same scenario by applying the patch provided in comment #3. The patch is applied cleanly and the error was cleared. So, I created MR with the same code changes.

I am attaching before & after screenshots for reference.

This issue is also reproducible in 11.x and was able to fix the issue with the same code changes.

I'll be happy to create MR for 11.x also if required. Please suggest how we can proceed further.

Fixes should be against 11.x

Also we should probably do some debugging vs putting a null check. Could be masking a larger issue

Hi,
Created an MR for 11. x.

Despite the db column allowing NULL. our API assumes there will always be a mime type. Why do you have NULL mimetypes?

Or the DB setting is wrong.

\Drupal\Core\File\MimeType\MimeTypeGuesser::guessMimeType() defaults to 'application/octet-stream' if it can't determine a mime type. Would it make sense to default to this rather than empty string?

Well, it's better than getting an error, right?

Thanks for the patch.
In our case it happens if we send a webform graphql mutation with a pdf attached.

Added a default mime type constant of \Drupal\Core\File\MimeType\MimeTypeGuesser::DEFAULT_MIME_TYPE with value 'application/octet-stream' and use that instead of empty string.

Hiding some older patches and MRs.

Updated IS to reflect latest proposed resolution.

Updated title.

Seems pretty straight forward, not sure test coverage is needed here.

We have other code that assumes getMimeType() returns a string...

Consider

    $mimetype = $file->getMimeType();
    $mimetype = explode('/', $mimetype);

in

\Drupal\media\Plugin\media\Source\File::getThumbnail

This will also cause a deprection.

I think we should change \Drupal\file\Entity\File::getMimeType() to return the default value - or it could even do something like File::preCreate()... and guess the mimetype if we have a URI

  /**
   * {@inheritdoc}
   */
  public function getMimeType() {
    $mimetype = $this->get('filemime')->value;
    if ($mimetype === NULL) {
      $uri = $this->getFileUri();
      $mimetype = $uri !== NULL ? \Drupal::service('file.mime_type.guesser')->guess($uri) : MimeTypeGuesser::DEFAULT_MIME_TYPE;
    }
    return $mimetype;
  }

We should also update the interface to tell people not to return a NULL

I think this would be a good change because then all code can rely on getMimeType() returning something sensible rather than having to deal with the NULL situation (which must be very rare given the preCreate().

Re: #23 I updated \Drupal\file\Entity\File::getMimeType() to return the default mime type if NULL, and also the docs on \Drupal\file\FileInterface::getMimeType() to match this behaviour.

Feedback appears to be addressed.

One minor change, fine to self RTBC

Self-RTBTC

We can clean up a bit more here \Drupal\file\Plugin\Field\FieldFormatter\FileMediaFormatterBase::getSourceFiles has code that checks for NULL, which is no longer possible.

In searching for callers of this method, the number of places we're treating the return as a string is an excellent case to make this change.

Rebased bump.

Moved changes to a new branch from main and opened a new MR

MR !16258 looks ready to me.

I don't think we should be changing the way File::getMimeType works - we should handle it in the theme layer

We also need a test here - thanks!

Reduced the change to the bare minimum and added a test.

The minimal change in the preprocess hook addresses the earlier concerns. Test coverage looks good too. Back to RTBC

I think we can consolidate the test into the Drupal\Tests\file\Kernel\Formatter\FileEntityFormatterTest class. Maybe even just modify testFormatterFileLinkWithQueryString to do the assertions there.

Consolidated the test into FileEntityFormatterTest.

Feedback appears to be addressed.

Committed and pushed 61a6cb2 to main and 4210702 to 11.x. Thanks!

I think this can go to 11.4.x as well, confirming with other committers.

Confirmed with @longwave for 11.4.x backport.