8000 Postpone native guessers instantiation in MimeTypeGuesser. by vudaltsov · Pull Request #27323 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Postpone native guessers instantiation in MimeTypeGuesser. #27323

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Postpone native guessers instantiation in MimeTypeGuesser. #27323

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
8000
Diff view
Diff view
44 changes: 29 additions & 15 deletions src/Symfony/Component/HttpFoundation/File/MimeType/MimeTypeGuesser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@

namespace Symfony\Component\HttpFoundation\File\MimeType;

use Symfony\Component\HttpFoundation\File\Exception\FileNotFoundException;
use Symfony\Component\HttpFoundation\File\Exception\AccessDeniedException;
use Symfony\Component\HttpFoundation\File\Exception\FileNotFoundException;

/**
* A singleton mime type guesser.
Expand Down Expand Up @@ -44,15 +44,17 @@ class MimeTypeGuesser implements MimeTypeGuesserInterface
*
* @var MimeTypeGuesser
*/
private static $instance = null;
private static $instance;

/**
* All registered MimeTypeGuesserInterface instances.
*
* @var array
* @var MimeTypeGuesserInterface[]
*/
protected $guessers = array();

private $nativeGuessersLoaded = false;

/**
* Returns the singleton instance.
*
Expand All @@ -75,18 +77,8 @@ public static function reset()
self::$instance = null;
}

/**
* Registers all natively provided mime type guessers.
*/
private function __construct()
{
if (FileBinaryMimeTypeGuesser::isSupported()) {
$this->register(new FileBinaryMimeTypeGuesser());
}

if (FileinfoMimeTypeGuesser::isSupported()) {
$this->register(new FileinfoMimeTypeGuesser());
}
}

/**
Expand Down Expand Up @@ -125,18 +117,40 @@ public function guess($path)
throw new AccessDeniedException($path);
}

if (!$this->guessers) {
if (!$guessers = $this->getGuessers()) {
$msg = 'Unable to guess the mime type as no guessers are available';
if (!FileinfoMimeTypeGuesser::isSupported()) {
$msg .= ' (Did you enable the php_fileinfo extension?)';
}
throw new \LogicException($msg);
}

foreach ($this->guessers as $guesser) {
foreach ($guessers as $guesser) {
if (null !== $mimeType = $guesser->guess($path)) {
return $mimeType;
}
}
}

/**
* @return MimeTypeGuesserInterface[]
*/
private function getGuessers()
{
if (!$this->nativeGuessersLoaded) {
Copy link
Contributor
@stloyd stloyd May 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for new variable, just use: if (!$this->guessers) {}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not going to work in case someone registers new guessers later with register(). Then native guessers will never be appended and that might affect the guessing process since there will be no default guessers.

// register all natively provided mime type guessers.

if (FileinfoMimeTypeGuesser::isSupported()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just asking: couldn't we detect if native guessers are available during the application compilation with a compiler pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I understand what you are proposing. Yes, this can be done once while compilation. But then it's better not to use a Singleton and have just a normal ChainGuesser, empty by default. The problem is that MimeTypeGuesser as a Singleton is already used everywhere, so it's easier to fix it this way.

$this->guessers[] = new FileinfoMimeTypeGuesser();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this changes the existing behavior, as they are now registered at the end while the existing logic registers them before custom ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In existing code native guessers are prepended to an empty array in a private constructor. By the time guess is called, native loaders are always at the end of the array.

In this PR they are added later. So to have the same behavior, I add them to the end of the array in a reversed order.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I forgot the fact that register was prepending the guessers

}

if (FileBinaryMimeTypeGuesser::isSupported()) {
$this->guessers[] = new FileBinaryMimeTypeGuesser();
}

$this->nativeGuessersLoaded = true;
}

return $this->guessers;
}
}
0