Increase PHPStan level to 6

[franmomu]

Q	A
Branch?	3.x
Bug fix?	no
New feature?	no
BC breaks?	yes
Deprecations?	no
Fixed tickets
License	MIT
Doc

This PR raises PHPStan level to 6, the new baseline is only for the iterable array types (I've created a new one so it's easier to regenerate it), it requires quite a lot of work and knowledge about the package to fill all the iterable types.

There is a checkMissingIterableValueType option for that, but IMO is better to start adding these types.

[dbu]

thanks. and agree with adding the baseline.

one question:

[dbu]

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

[franmomu]

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.

[robfrawley]

@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).

[robfrawley]

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

[dbu]

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

[robfrawley]

"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.