sonata-project / SonataMediaBundle

Symfony SonataMediaBundle
https://docs.sonata-project.org/projects/SonataMediaBundle
MIT License
449 stars 496 forks source link

`ResizerInterface::getBox` wrong PHPStan types #2419

Closed mvhirsch closed 8 months ago

mvhirsch commented 11 months ago

Environment

PHP version

$ symfony php -v
PHP 7.4.33 (cli) (built: Aug  1 2023 08:17:22) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Zend OPcache v7.4.33, Copyright (c), by Zend Technologies
    with blackfire v1.88.0~linux-x64-non_zts74, https://blackfire.io, by Blackfire

Subject

I've implemented my own Resizer. After writing a simple test, PHPStan now complains that my test code looks wrong:

        /** @var ResizerInterface $resizer */
        $resizer = self::$container->get(MyResizer::class);

        $box = $resizer->getBox($media, ['width' => 1280, 'height' => null]);
        self::assertEquals(new Box(1280, 720), $box);

ResizerInterface::getBox declares $settings to be a FormatOptions (imported from MediaProviderInterface):

https://github.com/sonata-project/SonataMediaBundle/blob/bc844058947e62c10f20edb3d94620e292582231/src/Resizer/ResizerInterface.php#L33-L38

https://github.com/sonata-project/SonataMediaBundle/blob/bc844058947e62c10f20edb3d94620e292582231/src/Provider/MediaProviderInterface.php#L27-L35

As far as I can tell, no implemented Resizer uses any other array keys than width and height.

Minimal repository with the bug

Steps to reproduce

phpstan analyze --level 5

Expected results

No PHPStan errors reported.

Actual results

Parameter #2 $settings of method Sonata\MediaBundle\Resizer\ResizerInterface::getBox() expects array{width: int|null, height: int|null, quality: int, format: string, constraint: bool, resizer: string|null, resizer_options: array<string, bool|int|string|null>}, array{width: 1280, height: null} given.
Array does not have offset 'quality'.
VincentLanglet commented 8 months ago

When looking about getBox usage, we always pass an array with all the key

array{ 
  *  width: int|null, 
  *  height: int|null, 
  *  quality: int, 
  *  format: string, 
  *  constraint: bool, 
  *  resizer: string|null, 
  *  resizer_options: array<string, string|bool|int|null>, 
  * } 

So it seems natural require to supports those keys if you implement the interface.

You might decide to use different width/height based on the quality or the format for instance.

If the issue is about one call in your test, you should test your method in real condition, by adding.

'quality' => 80,
'format' => 'jpg',
'constraint' => true,
'resizer' => null,
'resizer_options' => [],

in your array.

It doesn't seems worth to change the signature of the interface for this.