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.

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

Jump to bottom

Increase PHPStan level to 6 #1432

Merged
merged 1 commit into from
Nov 22, 2021
Merged

Increase PHPStan level to 6 #1432

Show file tree
Hide file tree
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
Diff view
Diff view
5 changes: 5 additions & 0 deletions .phpstan/baseline.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,8 @@ parameters:
- message: "#^Unreachable statement \\- code above always terminates\\.$#"
count: 1
path: ../src/Controller/ImagineController.php

# BC Layer for Symfony < 5.1
- message: "#^Parameter \\#2 \\$default of method Symfony\\\\Component\\\\HttpFoundation\\\\InputBag\\<string\\>\\:\\:get\\(\\) expects string\\|null, array given\\.$#"
count: 1
path: ../src/Controller/ImagineController.php
622 changes: 622 additions & 0 deletions .phpstan/iterable_types_baseline.neon
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion phpstan.neon.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
includes:
- .phpstan/baseline.neon
- .phpstan/iterable_types_baseline.neon
- vendor/phpstan/phpstan-symfony/extension.neon
parameters:
level: 4
level: 6
paths:
- src
treatPhpDocTypesAsCertain: false
Expand Down
6 changes: 6 additions & 0 deletions src/Binary/Loader/AbstractDoctrineLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,14 @@ abstract class AbstractDoctrineLoader implements LoaderInterface
{
protected ObjectManager $manager;

/**
* @phpstan-var class-string
*/
protected string $modelClass;

/**
* @phpstan-param class-string $modelClass
*/
public function __construct(ObjectManager $manager, string $modelClass)
{
$this->manager = $manager;
Expand Down
19 changes: 10 additions & 9 deletions src/Binary/Loader/ChainLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,23 +49,24 @@ public function find($path)
}

/**
* @param \Exception[] $exceptions
* @param array<string, LoaderInterface> $exceptions
Copy link
Member

Choose a reason for hiding this comment

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

are the exceptions really an array of LoaderInterface and not of an exception class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, this is where is called:

LiipImagineBundle/src/Binary/Loader/ChainLoader.php

Lines 36 to 49 in fec3726

public function find($path)
{
$exceptions = [];
foreach ($this->loaders as $loader) {
try {
return $loader->find($path);
} catch (\Exception $e) {
$exceptions[$e->getMessage()] = $loader;
}
}
throw new NotLoadableException(self::getLoaderExceptionMessage($path, $exceptions, $this->loaders));
}

where loaders are:

LiipImagineBundle/src/Binary/Loader/ChainLoader.php

Lines 28 to 30 in fec3726

$this->loaders = array_filter($loaders, function ($loader) {
return $loader instanceof LoaderInterface;
});

and the array_walk is receiving LoaderInterface:

LiipImagineBundle/src/Binary/Loader/ChainLoader.php

Lines 56 to 62 in fec3726

array_walk($loaders, function (LoaderInterface &$loader, string $name): void {
$loader = sprintf('%s=[%s]', (new \ReflectionObject($loader))->getShortName(), $name);
});
array_walk($exceptions, function (LoaderInterface &$loader, string $message): void {
$loader = sprintf('%s=[%s]', (new \ReflectionObject($loader))->getShortName(), $message);
});

But, based on the name, maybe it was a mistake.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dbu: The ChainLoader was thrown together without much thought, with no real planning, and no refactoring since, as we had needed to quickly resolve a dozen or so issues and feature requests at that time and, while not elegant, "it worked (TM)". ;-) That said, I must admit that the above handling is definitely hard to follow and conflates more responsibilities onto the loader class than it should have knowledge of.

Do you find an implementation such as github.com/liip/LiipImagineBundle/compare/3.x...robfrawley:3.x to be clearer? Essentially, I wrapped any failures into their own exception class and also refactored the final thrown error handling logic (like message compilation) into its own Exception class, as well. Seems to work as expected; all the Binary\Loader tests continue to pass with these changes (I'll have to check the whole test sweet later, though, to be sure nothing is inadvertently affected). The refactored code above needs some additional love itself, too, but I'll tackle that depending on the reception.

I can create a PR if the consensus is that the change is worthwhile, or feel free to discuss alternative approaches to cleaning it up. Either way, I try to take responsibility for any subpar code I introduced myself. :-) The real question is whether we'd prefer the code to be concise and cryptic (as it is) or verbose and readable (as I've outlined).

Copy link
Collaborator

@robfrawley robfrawley Nov 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Forgot to ping @franmomu relating to the above comment: #1432 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the explanation @franmomu , i will merge the phpstan cleanups then, as that was the only question.

thanks @robfrawley for the refactoring suggestion. i like the direction this is taking! please do make a PR for it, and lets look at the details in that PR. (i myself have not contributed much to this bundle before taking up maintenance a few weeks ago, so i am not too familiar with its history. that also means i am not attached to any particular solution but very open to improve things ;-) )

Copy link
Collaborator

Choose a reason for hiding this comment

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

"Thanks @robfrawley for the refactoring suggestion. [...] Please do make a PR for it..." -@dbu

Sounds good; see #1434. 👍

By the way, I'm so glad you all finally cleaned up the directory structure of this bundle by adding root src, test directories, etc. I tried to make that happen years ago, but it was rejected at the time to minimize merge and rebase issues. ;-)

Anyway, my point is, having gone through a cursory review of your recent maintenance work, it looks like you guys have been doing wonders updating and modernizing this bundle.

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

array_walk($exceptions, function (LoaderInterface &$loader, string $message): void {
$loader = sprintf('%s=[%s]', (new \ReflectionObject($loader))->getShortName(), $message);
});
$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(', ', $loaders),
implode(', ', $loaderMessages),
\count($loaders),
implode(', ', $exceptions),
implode(', ', $exceptionMessages),
]);
}
}
2 changes: 1 addition & 1 deletion src/DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ private function addLoadersSections(ArrayNodeDefinition $resolversPrototypeNode)
/**
* @param FactoryInterface[] $factories
*/
private function addConfigurationSections(array $factories, ArrayNodeDefinition $definition, $type): void
private function addConfigurationSections(array $factories, ArrayNodeDefinition $definition, string $type): void
{
foreach ($factories as $f) {
$f->addConfiguration($definition->children()->arrayNode($f->getName()));
Expand Down
6 changes: 3 additions & 3 deletions src/DependencyInjection/LiipImagineExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,17 +123,17 @@ private function createFilterSets(array $defaultFilterSets, array $filterSets):
}, $filterSets);
}

private function loadResolvers(array $config, ContainerBuilder $container)
private function loadResolvers(array $config, ContainerBuilder $container): void
{
$this->createFactories($this->resolversFactories, $config, $container);
}

private function loadLoaders(array $config, ContainerBuilder $container)
private function loadLoaders(array $config, ContainerBuilder $container): void
{
$this->createFactories($this->loadersFactories, $config, $container);
}

private function createFactories(array $factories, array $configurations, ContainerBuilder $container)
private function createFactories(array $factories, array $configurations, ContainerBuilder $container): void
{
foreach ($configurations as $name => $conf) {
$factories[key($conf)]->create($container, $name, $conf[key($conf)]);
Expand Down
2 changes: 1 addition & 1 deletion src/Factory/Config/Filter/Argument/PointFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
*/
final class PointFactory
{
public function create($x = null, $y = null): Point
public function create(int $x = null, int $y = null): Point
{
return new Point($x, $y);
}
Expand Down
8 changes: 7 additions & 1 deletion src/Imagine/Cache/Resolver/AbstractFilesystemResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,14 @@ public function remove(array $paths, array $filters): void
return;
}

$firstFilter = current($filters);

if (!\is_string($firstFilter) || '' === $firstFilter) {
return;
}

// TODO: this logic has to be refactored.
[$rootCachePath] = explode(current($filters), $this->getFilePath('whateverpath', current($filters)));
[$rootCachePath] = explode($firstFilter, $this->getFilePath('whateverpath', $firstFilter));

if (empty($paths)) {
$filtersCachePaths = [];
Expand Down
2 changes: 1 addition & 1 deletion src/Imagine/Cache/Resolver/AwsS3Resolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ public function remove(array $paths, array $filters): void

if (empty($paths)) {
try {
$this->storage->deleteMatchingObjects($this->bucket, null, sprintf(
$this->storage->deleteMatchingObjects($this->bucket, '', sprintf(
'/%s/i',
implode('|', $filters)
));
Expand Down
4 changes: 2 additions & 2 deletions src/Imagine/Filter/Loader/FixedFilterLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ public function load(ImageInterface $image, array $options = []): ImageInterface
// define filters
$resize = new Resize($size);
$origin = new Point(
floor(($size->getWidth() - $box->getWidth()) / 2),
floor(($size->getHeight() - $box->getHeight()) / 2)
(int) floor(($size->getWidth() - $box->getWidth()) / 2),
(int) floor(($size->getHeight() - $box->getHeight()) / 2)
);
$crop = new Crop($origin, $box);

Expand Down
2 changes: 1 addition & 1 deletion src/Imagine/Filter/Loader/ScaleFilterLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public function load(ImageInterface $image, array $options = []): ImageInterface
}

if ($this->isImageProcessable($ratio)) {
$filter = new Resize(new Box(round($origWidth * $ratio), round($origHeight * $ratio)));
$filter = new Resize(new Box((int) round($origWidth * $ratio), (int) round($origHeight * $ratio)));

return $filter->apply($image);
}
Expand Down
4 changes: 4 additions & 0 deletions src/Imagine/Filter/RelativeResize.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@
class RelativeResize implements FilterInterface
{
private string $method;

/**
* @var mixed
*/
private $parameter;

/**
Expand Down
5 changes: 1 addition & 4 deletions src/Templating/LazyFilterExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,7 @@ public function getFilters(): array
];
}

/**
* {@inheritdoc}
*/
public function getName()
public function getName(): string
{
return 'liip_imagine_lazy';
}
Expand Down
2 changes: 1 addition & 1 deletion src/Utility/Framework/SymfonyFramework.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,6 @@ public static function isKernelLessThan(int $major, int $minor = null, int $patc

private static function kernelVersionCompare(string $operator, int $major, int $minor = null, int $patch = null): bool
{
return version_compare(Kernel::VERSION_ID, sprintf("%d%'.02d%'.02d", $major, $minor ?: 0, $patch ?: 0), $operator);
return version_compare((string) Kernel::VERSION_ID, sprintf("%d%'.02d%'.02d", $major, $minor ?: 0, $patch ?: 0), $operator);
}
}
26 changes: 7 additions & 19 deletions tests/Functional/AbstractSetupWebTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,25 +19,13 @@
*/
class AbstractSetupWebTestCase extends AbstractWebTestCase
{
/**
* @var KernelBrowser
*/
protected $client;

/**
* @var Filesystem
*/
protected $filesystem;

/**
* @var string
*/
protected $webRoot;

/**
* @var string
*/
protected $cacheRoot;
protected KernelBrowser $client;

protected Filesystem $filesystem;

protected string $webRoot;

protected string $cacheRoot;

protected function setUp(): void
{
Expand Down
9 changes: 2 additions & 7 deletions tests/Functional/AbstractWebTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,7 @@ public static function getKernelClass(): string
return AppKernel::class;
}

/**
* @return object
*/
protected function getService(string $name)
protected function getService(string $name): ?object
{
if (property_exists($this, 'container')) {
return static::$container->get($name);
Expand All @@ -48,11 +45,9 @@ protected function getParameter(string $name)
}

/**
* @param object $object
*
* @return mixed
*/
protected function getPrivateProperty($object, string $name)
protected function getPrivateProperty(object $object, string $name)
{
$r = new \ReflectionObject($object);

Expand Down