Skip to content
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.

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

Refactor exception handling of Binary\Loader\ChainLoader class #1434

Open
wants to merge 2 commits into
base: 3.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 8 additions & 28 deletions src/Binary/Loader/ChainLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,19 @@

namespace Liip\ImagineBundle\Binary\Loader;

use Liip\ImagineBundle\Exception\Binary\Loader\ChainAttemptNotLoadableException;
use Liip\ImagineBundle\Exception\Binary\Loader\ChainNotLoadableException;
use Liip\ImagineBundle\Exception\Binary\Loader\NotLoadableException;

class ChainLoader implements LoaderInterface
{
/**
* @var LoaderInterface[]
* @var array<string, LoaderInterface>
*/
private array $loaders;

/**
* @param LoaderInterface[] $loaders
* @param array<string, LoaderInterface> $loaders
*/
public function __construct(array $loaders)
{
Expand All @@ -37,36 +39,14 @@ public function find($path)
{
$exceptions = [];

foreach ($this->loaders as $loader) {
foreach ($this->loaders as $configName => $loader) {
try {
return $loader->find($path);
} catch (\Exception $e) {
$exceptions[$e->getMessage()] = $loader;
} catch (NotLoadableException $loaderException) {
$exceptions[] = new ChainAttemptNotLoadableException($configName, $loader, $loaderException);
}
}

throw new NotLoadableException(self::getLoaderExceptionMessage($path, $exceptions, $this->loaders));
}

/**
* @param array<string, LoaderInterface> $exceptions
* @param array<string, LoaderInterface> $loaders
*/
private static function getLoaderExceptionMessage(string $path, array $exceptions, array $loaders): string
{
$loaderMessages = array_map(static function (string $name, LoaderInterface $loader) {
return sprintf('%s=[%s]', (new \ReflectionObject($loader))->getShortName(), $name);
}, array_keys($loaders), $loaders);

$exceptionMessages = array_map(static function (string $message, LoaderInterface $loader) {
return sprintf('%s=[%s]', (new \ReflectionObject($loader))->getShortName(), $message);
}, array_keys($exceptions), $exceptions);

return vsprintf('Source image not resolvable "%s" using "%s" %d loaders (internal exceptions: %s).', [
$path,
implode(', ', $loaderMessages),
\count($loaders),
implode(', ', $exceptionMessages),
]);
throw new ChainNotLoadableException($path, ...$exceptions);
}
}
53 changes: 53 additions & 0 deletions src/Exception/Binary/Loader/ChainAttemptNotLoadableException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?php

/*
* This file is part of the `liip/LiipImagineBundle` project.
*
* (c) https://github.com/liip/LiipImagineBundle/graphs/contributors
*
* For the full copyright and license information, please view the LICENSE.md
* file that was distributed with this source code.
*/

namespace Liip\ImagineBundle\Exception\Binary\Loader;

use Liip\ImagineBundle\Binary\Loader\LoaderInterface;

final class ChainAttemptNotLoadableException extends NotLoadableException
{
private string $loaderIndex;
private LoaderInterface $loaderClass;
Copy link
Member

Choose a reason for hiding this comment

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

i would prefer to call this $loader. $loaderClass suggests its the class name of the loader, but it is the actual instance.

then again, looking at the rest of the code: we do not use the class anywhere afterwards. how about doing the reflection thing in the constructor and have this field be private string $loaderClassName?


public function __construct(string $loaderIndex, LoaderInterface $loaderClass, NotLoadableException $loaderException)
{
$this->loaderIndex = $loaderIndex;
$this->loaderClass = $loaderClass;

parent::__construct($this->compileMessageTxt(), 0, $loaderException);
}

public function getLoaderIndex(): string
{
return $this->loaderIndex;
}

public function getLoaderClass(): LoaderInterface
Copy link
Member

Choose a reason for hiding this comment

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

this is not called from outside the class, i think we can drop the method

{
return $this->loaderClass;
}

public function getLoaderClassName(): string
Copy link
Member

Choose a reason for hiding this comment

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

but this one should stay as is, it gives you the classname of the loader, so i like the name.

if we move reflection to the constructor, this will become a simple getter for $this->loaderClassName

{
return (new \ReflectionObject($this->getLoaderClass()))->getShortName();
}

public function getLoaderException(): string
{
return $this->getPrevious()->getMessage();
}

private function compileMessageTxt(): string
{
return sprintf('%s=[%s]', $this->getLoaderClassName(), $this->getLoaderIndex());
}
}
49 changes: 49 additions & 0 deletions src/Exception/Binary/Loader/ChainNotLoadableException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<?php

/*
* This file is part of the `liip/LiipImagineBundle` project.
*
* (c) https://github.com/liip/LiipImagineBundle/graphs/contributors
*
* For the full copyright and license information, please view the LICENSE.md
* file that was distributed with this source code.
*/

namespace Liip\ImagineBundle\Exception\Binary\Loader;

final class ChainNotLoadableException extends NotLoadableException
{
public function __construct(string $path, ChainAttemptNotLoadableException ...$exceptions)
{
parent::__construct(self::compileExceptionMessage($path, ...$exceptions));
}

private static function compileExceptionMessage(string $path, ChainAttemptNotLoadableException ...$exceptions): string
{
return vsprintf('Source image not resolvable "%s" using "%s" %d loaders (internal exceptions: %s).', [
$path,
self::compileLoaderConfigMaps(...$exceptions),
\count($exceptions),
self::compileLoaderErrorsList(...$exceptions),
]);
}

private static function compileLoaderConfigMaps(ChainAttemptNotLoadableException ...$exceptions): string
{
return self::implodeMappedExceptions(static function (ChainAttemptNotLoadableException $e): string {
return $e->getMessage();
}, ...$exceptions);
}

private static function compileLoaderErrorsList(ChainAttemptNotLoadableException ...$exceptions): string
{
return self::implodeMappedExceptions(static function (ChainAttemptNotLoadableException $e): string {
return sprintf('%s=[%s]', $e->getLoaderClassName(), $e->getLoaderException());
}, ...$exceptions);
}

private static function implodeMappedExceptions(\Closure $mapper, ChainAttemptNotLoadableException ...$exceptions): string
{
return implode(', ', array_map($mapper, $exceptions));
}
}
35 changes: 29 additions & 6 deletions tests/AbstractTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -271,16 +271,39 @@ protected function createObjectMock(string $object, array $methods = [], bool $c
return $builder->getMock();
}

/**
* @param object $object
*/
protected function getVisibilityRestrictedMethod($object, string $name): \ReflectionMethod
protected static function getReflectionObject(object $object): \ReflectionObject
{
return new \ReflectionObject($object);
}

protected static function getReflectionObjectName(object $object): string
{
$r = new \ReflectionObject($object);
return self::getReflectionObject($object)->getName();
}

$m = $r->getMethod($name);
protected function getVisibilityRestrictedMethod($object, string $name): \ReflectionMethod
{
$m = self::getReflectionObject($object)->getMethod($name);
$m->setAccessible(true);

return $m;
}

protected static function generateRandomInteger(int $lowerBound = null, int $upperBound = null): int
{
$lowerBound = $lowerBound ?? 0;
$upperBound = $upperBound ?? 100000000;

if ($lowerBound > $upperBound) {
throw new \LogicException('Lower-bound argument must be less-than or equal-to upper-bound argument!');
}

try {
return random_int($lowerBound, $upperBound);
} catch (\Throwable $e) {
throw new \RuntimeException(vsprintf('Failed to generate random number between %d and %d: %s', [
$lowerBound, $upperBound, $e->getMessage()
]));
}
}
}
Loading