From 66bab25c7ce6b3eac118bee97c6c8f04ead54e98 Mon Sep 17 00:00:00 2001 From: Manuel Meister Date: Wed, 27 Sep 2023 11:16:51 +0200 Subject: [PATCH 1/8] FEATURE: Only output existing images and allow upscaling --- Classes/Domain/AbstractImageSource.php | 31 +----- .../Domain/AbstractScalableImageSource.php | 105 +++++++++++++++++- Classes/Domain/AssetImageSource.php | 4 +- .../Domain/ScalableImageSourceInterface.php | 2 +- .../Private/Fusion/Prototypes/Image.fusion | 10 +- .../Private/Fusion/Prototypes/Picture.fusion | 14 ++- .../Private/Fusion/Prototypes/Source.fusion | 12 +- 7 files changed, 131 insertions(+), 47 deletions(-) diff --git a/Classes/Domain/AbstractImageSource.php b/Classes/Domain/AbstractImageSource.php index 81d2b36..026178b 100644 --- a/Classes/Domain/AbstractImageSource.php +++ b/Classes/Domain/AbstractImageSource.php @@ -7,7 +7,6 @@ use Neos\Eel\ProtectedContextAwareInterface; use Neos\Flow\Annotations as Flow; use Neos\Flow\Log\Utility\LogEnvironment; -use Neos\Utility\Arrays; use Psr\Log\LoggerInterface; use Sitegeist\Kaleidoscope\EelHelpers\ImageSourceHelperInterface; @@ -197,39 +196,15 @@ public function withVariantPreset(string $presetIdentifier, string $presetVarian } /** - * Render sourceset Attribute for various media descriptors. + * Render sourceset Attribute non-scalable media. * * @param mixed $mediaDescriptors + * @param bool $allowUpScaling * * @return string */ - public function srcset($mediaDescriptors): string + public function srcset($mediaDescriptors, bool $allowUpScaling = false): string { - if ($this instanceof ScalableImageSourceInterface) { - $srcsetArray = []; - - if (is_array($mediaDescriptors) || $mediaDescriptors instanceof \Traversable) { - $descriptors = $mediaDescriptors; - } else { - $descriptors = Arrays::trimExplode(',', (string) $mediaDescriptors); - } - - foreach ($descriptors as $descriptor) { - if (preg_match('/^(?[0-9]+)w$/u', $descriptor, $matches)) { - $width = (int) $matches['width']; - $scaleFactor = $width / $this->width(); - $scaled = $this->scale($scaleFactor); - $srcsetArray[] = $scaled->src() . ' ' . $width . 'w'; - } elseif (preg_match('/^(?[0-9\\.]+)x$/u', $descriptor, $matches)) { - $factor = (float) $matches['factor']; - $scaled = $this->scale($factor); - $srcsetArray[] = $scaled->src() . ' ' . $factor . 'x'; - } - } - - return implode(', ', $srcsetArray); - } - return $this->src(); } diff --git a/Classes/Domain/AbstractScalableImageSource.php b/Classes/Domain/AbstractScalableImageSource.php index 60deaea..2e8924b 100644 --- a/Classes/Domain/AbstractScalableImageSource.php +++ b/Classes/Domain/AbstractScalableImageSource.php @@ -7,12 +7,14 @@ use Imagine\Image\Box; use Imagine\Image\ImagineInterface; use Neos\Flow\Annotations as Flow; +use Neos\Flow\Log\Utility\LogEnvironment; use Neos\Media\Domain\Model\Adjustment\CropImageAdjustment; use Neos\Media\Domain\Model\Adjustment\ImageAdjustmentInterface; use Neos\Media\Domain\Model\Adjustment\ResizeImageAdjustment; use Neos\Media\Domain\ValueObject\Configuration\Adjustment; use Neos\Media\Domain\ValueObject\Configuration\VariantPreset; use Neos\Utility\ObjectAccess; +use Neos\Utility\Arrays; use Sitegeist\Kaleidoscope\EelHelpers\ScalableImageSourceHelperInterface; abstract class AbstractScalableImageSource extends AbstractImageSource implements ScalableImageSourceInterface, ScalableImageSourceHelperInterface @@ -34,13 +36,18 @@ abstract class AbstractScalableImageSource extends AbstractImageSource implement */ protected $baseHeight; + /** + * @var bool + */ + protected $allowUpScaling = false; + /** * @param int|null $targetWidth * @param bool $preserveAspect * - * @return ImageSourceInterface + * @return ScalableImageSourceInterface */ - public function withWidth(int $targetWidth = null, bool $preserveAspect = false): ImageSourceInterface + public function withWidth(int $targetWidth = null, bool $preserveAspect = false): ScalableImageSourceInterface { $newSource = clone $this; $newSource->targetWidth = $targetWidth; @@ -60,9 +67,9 @@ public function withWidth(int $targetWidth = null, bool $preserveAspect = false) * @param int|null $targetHeight * @param bool $preserveAspect * - * @return ImageSourceInterface + * @return ScalableImageSourceInterface */ - public function withHeight(int $targetHeight = null, bool $preserveAspect = false): ImageSourceInterface + public function withHeight(int $targetHeight = null, bool $preserveAspect = false): ScalableImageSourceInterface { $newSource = clone $this; $newSource->targetHeight = $targetHeight; @@ -78,14 +85,32 @@ public function withHeight(int $targetHeight = null, bool $preserveAspect = fals return $newSource; } + /** + * @param int $targetWidth + * @param int $targetHeight + * + * @return ScalableImageSourceInterface + */ + public function withDimensions(int $targetWidth, int $targetHeight): ScalableImageSourceInterface + { + $newSource = clone $this; + $newSource->targetWidth = $targetWidth; + $newSource->targetHeight = $targetHeight; + + return $newSource; + } + + /** * @param float $factor + * @param bool $allowUpScaling * - * @return ImageSourceInterface + * @return ScalableImageSourceInterface */ - public function scale(float $factor): ImageSourceInterface + public function scale(float $factor, bool $allowUpScaling = false): ScalableImageSourceInterface { $scaledHelper = clone $this; + $scaledHelper->allowUpScaling = $allowUpScaling; if ($this->targetWidth && $this->targetHeight) { $scaledHelper = $scaledHelper->withDimensions((int) round($factor * $this->targetWidth), (int) round($factor * $this->targetHeight)); @@ -213,4 +238,72 @@ protected function createAdjustment(Adjustment $adjustmentConfiguration): ImageA return $adjustment; } + + /** + * Render srcset Attribute for various media descriptors. + * + * If upscaling is not allowed and the width is greater than the base width, + * use the base width. + * + * @param $mediaDescriptors + * @param bool $allowUpScaling + * + * @return string + */ + public function srcset($mediaDescriptors, bool $allowUpScaling = false): string + { + $srcsetArray = []; + + if (is_array($mediaDescriptors) || $mediaDescriptors instanceof \Traversable) { + $descriptors = $mediaDescriptors; + } else { + $descriptors = Arrays::trimExplode(',', (string)$mediaDescriptors); + } + + $srcsetType = null; + $maxScaleFactor = min($this->baseWidth / $this->targetWidth, $this->baseHeight / $this->targetHeight); + + foreach ($descriptors as $descriptor) { + $hasDescriptor = preg_match('/^(?[0-9]+)w$|^(?[0-9\\.]+)x$/u', $descriptor, $matches); + + if (!$hasDescriptor) { + $this->logger->warning(sprintf('Invalid media descriptor "%s". Missing type "x" or "w"', $descriptor), LogEnvironment::fromMethodName(__METHOD__)); + continue; + } + + if (!$srcsetType) { + $srcsetType = isset($matches['width']) ? 'width' : 'factor'; + } elseif (($srcsetType === 'width' && isset($matches['factor'])) || ($srcsetType === 'factor' && isset($matches['width']))) { + $this->logger->warning(sprintf('Mixed media descriptors are not valid: [%s]', implode(', ', is_array($descriptors) ? $descriptors : iterator_to_array($descriptors))), LogEnvironment::fromMethodName(__METHOD__)); + break; + } + + if ($srcsetType === 'width') { + $width = (int)$matches['width']; + $scaleFactor = $width / $this->width(); + if (!$allowUpScaling && ($width / $this->baseWidth > 1)) { + $srcsetArray[] = $this->src() . ' ' . $this->baseWidth . 'w'; + } else { + $scaled = $this->scale($scaleFactor, $allowUpScaling); + $srcsetArray[] = $scaled->src() . ' ' . $width . 'w'; + } + } elseif ($srcsetType === 'factor') { + $factor = (float)$matches['factor']; + if ( + !$allowUpScaling && ( + ($this->targetHeight && ($maxScaleFactor < $factor)) || + ($this->targetWidth && ($maxScaleFactor < $factor)) + ) + ) { + $scaled = $this->scale($maxScaleFactor, $allowUpScaling); + $srcsetArray[] = $scaled->src() . ' ' . $maxScaleFactor . 'x'; + } else { + $scaled = $this->scale($factor, $allowUpScaling); + $srcsetArray[] = $scaled->src() . ' ' . $factor . 'x'; + } + } + } + + return implode(', ', array_unique($srcsetArray)); + } } diff --git a/Classes/Domain/AssetImageSource.php b/Classes/Domain/AssetImageSource.php index ca3c244..9a0ce68 100644 --- a/Classes/Domain/AssetImageSource.php +++ b/Classes/Domain/AssetImageSource.php @@ -117,7 +117,7 @@ public function src(): string $async = $this->request ? $this->async : false; $allowCropping = true; - $allowUpScaling = false; + $allowUpScaling = $this->allowUpScaling; $thumbnailConfiguration = new ThumbnailConfiguration( $width, $width, @@ -154,7 +154,7 @@ public function dataSrc(): string $async = false; $allowCropping = true; - $allowUpScaling = false; + $allowUpScaling = $this->allowUpScaling; $thumbnailConfiguration = new ThumbnailConfiguration( $width, $width, diff --git a/Classes/Domain/ScalableImageSourceInterface.php b/Classes/Domain/ScalableImageSourceInterface.php index 9883ded..72f8e62 100644 --- a/Classes/Domain/ScalableImageSourceInterface.php +++ b/Classes/Domain/ScalableImageSourceInterface.php @@ -6,5 +6,5 @@ interface ScalableImageSourceInterface extends ImageSourceInterface { - public function scale(float $factor): ImageSourceInterface; + public function scale(float $factor, bool $allowUpScaling = false): ImageSourceInterface; } diff --git a/Resources/Private/Fusion/Prototypes/Image.fusion b/Resources/Private/Fusion/Prototypes/Image.fusion index 76f5a48..3e9e7f1 100644 --- a/Resources/Private/Fusion/Prototypes/Image.fusion +++ b/Resources/Private/Fusion/Prototypes/Image.fusion @@ -126,6 +126,8 @@ prototype(Sitegeist.Kaleidoscope:Image) < prototype(Neos.Fusion:Component) { format = null attributes = Neos.Fusion:DataStructure renderDimensionAttributes = true + allowSrcsetUpScaling = false + preserveAspect = true renderer = Neos.Fusion:Component { @if.hasImageSource = ${props.imageSource && Type.instance(props.imageSource, '\\Sitegeist\\Kaleidoscope\\Domain\\ImageSourceInterface')} @@ -133,8 +135,9 @@ prototype(Sitegeist.Kaleidoscope:Image) < prototype(Neos.Fusion:Component) { # apply format, width and height to the imageSource imageSource = ${props.imageSource} - imageSource.@process.applyWidth = ${props.width ? value.withWidth(props.width) : value} - imageSource.@process.applyHeight = ${props.height ? value.withHeight(props.height) : value} + imageSource.@process.applyDimensions = ${(props.width && props.height) ? value.withDimensions(props.width, props.height) : value} + imageSource.@process.applyWidth = ${(props.width && !props.height) ? value.withWidth(props.width, props.preserveAspect) : value} + imageSource.@process.applyHeight = ${(props.height && !props.width) ? value.withHeight(props.height, props.preserveAspect) : value} imageSource.@process.applyFormat = ${props.format ? value.withFormat(props.format) : value} srcset = ${props.srcset} @@ -145,11 +148,12 @@ prototype(Sitegeist.Kaleidoscope:Image) < prototype(Neos.Fusion:Component) { class = ${props.class} attributes = ${props.attributes} renderDimensionAttributes = ${props.renderDimensionAttributes} + allowSrcsetUpScaling = ${props.allowSrcsetUpScaling} renderer = afx` @@ -114,6 +119,8 @@ prototype(Sitegeist.Kaleidoscope:Picture) < prototype(Neos.Fusion:Component) { srcset={source.srcset ? source.srcset : props.srcset} sizes={source.sizes ? source.sizes : props.sizes} renderDimensionAttributes={props.renderDimensionAttributes} + allowSrcsetUpScaling={props.allowSrcsetUpScaling} + preserveAspect={props.preserveAspect} /> ` diff --git a/Resources/Private/Fusion/Prototypes/Source.fusion b/Resources/Private/Fusion/Prototypes/Source.fusion index 3939ed7..0fba9c5 100644 --- a/Resources/Private/Fusion/Prototypes/Source.fusion +++ b/Resources/Private/Fusion/Prototypes/Source.fusion @@ -13,6 +13,8 @@ prototype(Sitegeist.Kaleidoscope:Source) < prototype(Neos.Fusion:Component) { type = null media = null renderDimensionAttributes = true + allowSrcsetUpScaling = false + preserveAspect = true renderer = Neos.Fusion:Component { @@ -29,19 +31,21 @@ prototype(Sitegeist.Kaleidoscope:Source) < prototype(Neos.Fusion:Component) { isScalableSource = ${imageSource && Type.instance(imageSource, '\\Sitegeist\\Kaleidoscope\\Domain\\ScalableImageSourceInterface')} imageSource = ${imageSource} - imageSource.@process.applyWidth = ${width ? value.withWidth(width) : value} - imageSource.@process.applyHeight = ${height ? value.withHeight(height) : value} - imageSource.@process.applyFormat = ${format ? value.withFormat(format) : value} + imageSource.@process.applyDimensions = ${(props.width && props.height) ? value.withDimensions(props.width, props.height) : value} + imageSource.@process.applyWidth = ${(props.width && !props.height) ? value.withWidth(props.width, props.preserveAspect) : value} + imageSource.@process.applyHeight = ${(props.height && !props.width) ? value.withHeight(props.height, props.preserveAspect) : value} + imageSource.@process.applyFormat = ${props.format ? value.withFormat(props.format) : value} type = ${format ? 'image/' + format : props.type} srcset = ${srcset} sizes = ${sizes} media = ${props.media} renderDimensionAttributes = ${props.renderDimensionAttributes} + allowSrcsetUpScaling = ${props.allowSrcsetUpScaling} renderer = afx` Date: Thu, 26 Oct 2023 13:13:54 +0200 Subject: [PATCH 2/8] FEATURE: Implement tests --- .../AbstractScalableImageSourceTest.php | 38 +++++++++---------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/Tests/Unit/Domain/AbstractScalableImageSourceTest.php b/Tests/Unit/Domain/AbstractScalableImageSourceTest.php index 4f3ec86..fd82337 100644 --- a/Tests/Unit/Domain/AbstractScalableImageSourceTest.php +++ b/Tests/Unit/Domain/AbstractScalableImageSourceTest.php @@ -38,23 +38,22 @@ public function srcsetWithWidthAdheresToDefinition() $dummy = new DummyImageSource('https://example.com', 'Test', 'Test', 400, 400, '999', 'fff', 'Test'); $this->assertEquals( 'https://example.com?w=200&h=200&bg=999&fg=fff&t=Test 200w, https://example.com?w=400&h=400&bg=999&fg=fff&t=Test 400w, https://example.com?w=600&h=600&bg=999&fg=fff&t=Test 600w', - $dummy->srcset('200w, 400w, 600w') + $dummy->srcset('200w, 400w, 600w', allowUpScaling: true) ); } /** * If the actual image is smaller than the requested size, then the image should be returned in its original size. * @test - * @todo */ - #public function srcsetWithWidthShouldOutputOnlyAvailableSources() - #{ - # $dummy = new DummyImageSource('https://example.com', 'Test', 'Test', 500, 500, '999', 'fff', 'Test'); - # $this->assertEquals( - # 'https://example.com?w=200&h=200&bg=999&fg=fff&t=Test 200w, https://example.com?w=400&h=400&bg=999&fg=fff&t=Test 400w, https://example.com?w=500&h=500&bg=999&fg=fff&t=Test 500w', - # $dummy->srcset('200w, 400w, 600w') - # ); - #} + public function srcsetWithWidthShouldOutputOnlyAvailableSources() + { + $dummy = new DummyImageSource('https://example.com', 'Test', 'Test', 500, 500, '999', 'fff', 'Test'); + $this->assertEquals( + 'https://example.com?w=200&h=200&bg=999&fg=fff&t=Test 200w, https://example.com?w=400&h=400&bg=999&fg=fff&t=Test 400w, https://example.com?w=500&h=500&bg=999&fg=fff&t=Test 500w', + $dummy->srcset('200w, 400w, 600w') + ); + } /** * @test @@ -72,15 +71,14 @@ public function srcsetWithRatioAdheresToDefinition() /** * If the actual image is smaller than the requested size, then the image should be returned in its original size. * @test - * @todo */ - #public function srcsetWithRatioShouldOutputOnlyAvailableSources() - #{ - # $dummy = new DummyImageSource('https://example.com', 'Test', 'Test', 30, 12, '999', 'fff', 'Test'); - # $copy = $dummy->withWidth(20, true); - # $this->assertEquals( - # 'https://example.com?w=20&h=8&bg=999&fg=fff&t=Test 1x, https://example.com?w=30&h=12&bg=999&fg=fff&t=Test 1.5x', - # $copy->srcset('1x, 2x') - # ); - #} + public function srcsetWithRatioShouldOutputOnlyAvailableSources() + { + $dummy = new DummyImageSource('https://example.com', 'Test', 'Test', 30, 12, '999', 'fff', 'Test'); + $copy = $dummy->withWidth(20, true); + $this->assertEquals( + 'https://example.com?w=20&h=8&bg=999&fg=fff&t=Test 1x, https://example.com?w=30&h=12&bg=999&fg=fff&t=Test 1.5x', + $copy->srcset('1x, 2x') + ); + } } \ No newline at end of file From 70a7fcae01e595149aaf61962606287d433cc83f Mon Sep 17 00:00:00 2001 From: Manuel Meister Date: Thu, 26 Oct 2023 13:14:37 +0200 Subject: [PATCH 3/8] BUGFIX: Prevent division by zero --- Classes/Domain/AbstractScalableImageSource.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Classes/Domain/AbstractScalableImageSource.php b/Classes/Domain/AbstractScalableImageSource.php index 2e8924b..0c2f8b1 100644 --- a/Classes/Domain/AbstractScalableImageSource.php +++ b/Classes/Domain/AbstractScalableImageSource.php @@ -261,7 +261,7 @@ public function srcset($mediaDescriptors, bool $allowUpScaling = false): string } $srcsetType = null; - $maxScaleFactor = min($this->baseWidth / $this->targetWidth, $this->baseHeight / $this->targetHeight); + $maxScaleFactor = min($this->baseWidth / $this->width(), $this->baseHeight / $this->height()); foreach ($descriptors as $descriptor) { $hasDescriptor = preg_match('/^(?[0-9]+)w$|^(?[0-9\\.]+)x$/u', $descriptor, $matches); From 2b39b81ba9ad425d12a02d8e41056e57132009a2 Mon Sep 17 00:00:00 2001 From: Manuel Meister Date: Thu, 26 Oct 2023 14:13:23 +0200 Subject: [PATCH 4/8] FEATURE: Test mixed and invalid descriptors --- .../Domain/AbstractScalableImageSource.php | 4 +- .../AbstractScalableImageSourceTest.php | 77 +++++++++++++++++-- 2 files changed, 73 insertions(+), 8 deletions(-) diff --git a/Classes/Domain/AbstractScalableImageSource.php b/Classes/Domain/AbstractScalableImageSource.php index 0c2f8b1..96ddfe4 100644 --- a/Classes/Domain/AbstractScalableImageSource.php +++ b/Classes/Domain/AbstractScalableImageSource.php @@ -264,7 +264,7 @@ public function srcset($mediaDescriptors, bool $allowUpScaling = false): string $maxScaleFactor = min($this->baseWidth / $this->width(), $this->baseHeight / $this->height()); foreach ($descriptors as $descriptor) { - $hasDescriptor = preg_match('/^(?[0-9]+)w$|^(?[0-9\\.]+)x$/u', $descriptor, $matches); + $hasDescriptor = preg_match('/^(?[0-9]+)w$|^(?[0-9\\.]+)x$/u', $descriptor, $matches, PREG_UNMATCHED_AS_NULL); if (!$hasDescriptor) { $this->logger->warning(sprintf('Invalid media descriptor "%s". Missing type "x" or "w"', $descriptor), LogEnvironment::fromMethodName(__METHOD__)); @@ -275,7 +275,7 @@ public function srcset($mediaDescriptors, bool $allowUpScaling = false): string $srcsetType = isset($matches['width']) ? 'width' : 'factor'; } elseif (($srcsetType === 'width' && isset($matches['factor'])) || ($srcsetType === 'factor' && isset($matches['width']))) { $this->logger->warning(sprintf('Mixed media descriptors are not valid: [%s]', implode(', ', is_array($descriptors) ? $descriptors : iterator_to_array($descriptors))), LogEnvironment::fromMethodName(__METHOD__)); - break; + continue; } if ($srcsetType === 'width') { diff --git a/Tests/Unit/Domain/AbstractScalableImageSourceTest.php b/Tests/Unit/Domain/AbstractScalableImageSourceTest.php index fd82337..3af6040 100644 --- a/Tests/Unit/Domain/AbstractScalableImageSourceTest.php +++ b/Tests/Unit/Domain/AbstractScalableImageSourceTest.php @@ -8,12 +8,20 @@ class AbstractScalableImageSourceTest extends BaseTestCase { + protected $logger = null; + + protected function setUp(): void + { + parent::setUp(); + $this->logger = $this->createMock(\Psr\Log\LoggerInterface::class); + } + /** * @test */ public function aspectRatioIsHonored() { - $dummy = new DummyImageSource('https://example.com', 'Test', 'Test', 400, 400, '999', 'fff', 'Test'); + $dummy = $this->getDummyImageSource(400, 400); $copy = $dummy->withWidth(200, true); $this->assertEquals(200, $copy->height()); } @@ -23,7 +31,7 @@ public function aspectRatioIsHonored() */ public function srcsetIsGenerated() { - $dummy = new DummyImageSource('https://example.com', 'Test', 'Test', 400, 400, '999', 'fff', 'Test'); + $dummy = $this->getDummyImageSource(400, 400); $this->assertEquals( 'https://example.com?w=200&h=200&bg=999&fg=fff&t=Test 200w, https://example.com?w=400&h=400&bg=999&fg=fff&t=Test 400w', $dummy->srcset('200w, 400w') @@ -35,7 +43,7 @@ public function srcsetIsGenerated() */ public function srcsetWithWidthAdheresToDefinition() { - $dummy = new DummyImageSource('https://example.com', 'Test', 'Test', 400, 400, '999', 'fff', 'Test'); + $dummy = $this->getDummyImageSource(400, 400); $this->assertEquals( 'https://example.com?w=200&h=200&bg=999&fg=fff&t=Test 200w, https://example.com?w=400&h=400&bg=999&fg=fff&t=Test 400w, https://example.com?w=600&h=600&bg=999&fg=fff&t=Test 600w', $dummy->srcset('200w, 400w, 600w', allowUpScaling: true) @@ -48,7 +56,7 @@ public function srcsetWithWidthAdheresToDefinition() */ public function srcsetWithWidthShouldOutputOnlyAvailableSources() { - $dummy = new DummyImageSource('https://example.com', 'Test', 'Test', 500, 500, '999', 'fff', 'Test'); + $dummy = $this->getDummyImageSource(500, 500); $this->assertEquals( 'https://example.com?w=200&h=200&bg=999&fg=fff&t=Test 200w, https://example.com?w=400&h=400&bg=999&fg=fff&t=Test 400w, https://example.com?w=500&h=500&bg=999&fg=fff&t=Test 500w', $dummy->srcset('200w, 400w, 600w') @@ -60,7 +68,7 @@ public function srcsetWithWidthShouldOutputOnlyAvailableSources() */ public function srcsetWithRatioAdheresToDefinition() { - $dummy = new DummyImageSource('https://example.com', 'Test', 'Test', 400, 200, '999', 'fff', 'Test'); + $dummy = $this->getDummyImageSource(400, 200); $copy = $dummy->withHeight(50, true); $this->assertEquals( 'https://example.com?w=100&h=50&bg=999&fg=fff&t=Test 1x, https://example.com?w=200&h=100&bg=999&fg=fff&t=Test 2x, https://example.com?w=300&h=150&bg=999&fg=fff&t=Test 3x', @@ -74,11 +82,68 @@ public function srcsetWithRatioAdheresToDefinition() */ public function srcsetWithRatioShouldOutputOnlyAvailableSources() { - $dummy = new DummyImageSource('https://example.com', 'Test', 'Test', 30, 12, '999', 'fff', 'Test'); + $dummy = $this->getDummyImageSource(30, 12); $copy = $dummy->withWidth(20, true); $this->assertEquals( 'https://example.com?w=20&h=8&bg=999&fg=fff&t=Test 1x, https://example.com?w=30&h=12&bg=999&fg=fff&t=Test 1.5x', $copy->srcset('1x, 2x') ); } + + /** + * Log a warning if the descriptors are mixed between width and factor + * @test + */ + public function srcsetShouldWarnIfMixedDescriptors() + { + $dummy = $this->getDummyImageSource(650, 320); + $this->logger->expects($this->once())->method('warning')->with($this->equalTo('Mixed media descriptors are not valid: [1x, 100w]')); + + $dummy->srcset('1x, 100w'); + } + + /** + * Skip srcset descriptor if it does not match the first matched descriptor + * @test + */ + public function srcsetShouldSkipMixedDescriptors() + { + $dummy = $this->getDummyImageSource(500, 300); + $this->assertEquals( + 'https://example.com?w=200&h=120&bg=999&fg=fff&t=Test 200w, https://example.com?w=440&h=264&bg=999&fg=fff&t=Test 440w', + $dummy->srcset('200w, 1x, 440w') + ); + } + + /** + * Log a warning if the descriptor is invalid + * @test + */ + public function srcsetShouldWarnIfMissingDescriptor() + { + $dummy = $this->getDummyImageSource(30, 12); + $this->logger->expects($this->once())->method('warning')->with($this->equalTo('Invalid media descriptor "1a". Missing type "x" or "w"')); + + $dummy->srcset('1a, 10w'); + } + + /** + * Should skip srcset descriptor if either width w or factor x is missing + * @test + */ + public function srcsetShouldSkipMissingDescriptors() + { + $dummy = $this->getDummyImageSource(200, 400); + $this->assertEquals( + 'https://example.com?w=100&h=200&bg=999&fg=fff&t=Test 100w, https://example.com?w=200&h=400&bg=999&fg=fff&t=Test 200w', + $dummy->srcset('100w, 150, 200w') + ); + } + + protected function getDummyImageSource($width, $height) + { + $dummy = new DummyImageSource('https://example.com', 'Test', 'Test', $width, $height, '999', 'fff', 'Test'); + $this->inject($dummy, 'logger', $this->logger); + return $dummy; + } } \ No newline at end of file From b2dfa765f0ea9cb22e6ac226dfcdad0ed69d6f69 Mon Sep 17 00:00:00 2001 From: Manuel Meister Date: Thu, 26 Oct 2023 14:18:17 +0200 Subject: [PATCH 5/8] BUGFIX: Remove unnecessary named parameter --- Tests/Unit/Domain/AbstractScalableImageSourceTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Unit/Domain/AbstractScalableImageSourceTest.php b/Tests/Unit/Domain/AbstractScalableImageSourceTest.php index 3af6040..d0f5a4d 100644 --- a/Tests/Unit/Domain/AbstractScalableImageSourceTest.php +++ b/Tests/Unit/Domain/AbstractScalableImageSourceTest.php @@ -46,7 +46,7 @@ public function srcsetWithWidthAdheresToDefinition() $dummy = $this->getDummyImageSource(400, 400); $this->assertEquals( 'https://example.com?w=200&h=200&bg=999&fg=fff&t=Test 200w, https://example.com?w=400&h=400&bg=999&fg=fff&t=Test 400w, https://example.com?w=600&h=600&bg=999&fg=fff&t=Test 600w', - $dummy->srcset('200w, 400w, 600w', allowUpScaling: true) + $dummy->srcset('200w, 400w, 600w', true) ); } From 1f6cb4d084878d159b550edab8645b49b5ae338f Mon Sep 17 00:00:00 2001 From: Manuel Meister Date: Thu, 26 Oct 2023 14:41:45 +0200 Subject: [PATCH 6/8] TASK: Increase php requirement to 7.4 --- .github/workflows/build.yml | 2 -- composer.json | 1 + 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index d75bb02..f2e50bc 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -16,8 +16,6 @@ jobs: php-versions: ['8.0', '8.1'] neos-versions: ['8.3'] include: - - php-versions: '7.3' - neos-versions: '7.3' - php-versions: '7.4' neos-versions: '7.3' - php-versions: '8.0' diff --git a/composer.json b/composer.json index 09c223c..513d2b6 100644 --- a/composer.json +++ b/composer.json @@ -4,6 +4,7 @@ "name": "sitegeist/kaleidoscope", "license": "GPL-3.0-or-later", "require": { + "php": "^7.4 || ^8.0", "neos/neos": "^7.0 || ^8.0 || ^9.0 || dev-master", "neos/fusion-afx": "^7.0 || ^8.0 || ^9.0 || dev-master", "neos/media": "*", From a54b57c75c602a42c4a21838016e839220f173eb Mon Sep 17 00:00:00 2001 From: Manuel Meister Date: Mon, 3 Jun 2024 17:11:21 +0200 Subject: [PATCH 7/8] Replace $allowUpScaling with supportsUpscaling() --- Classes/Domain/AbstractImageSource.php | 3 +-- .../Domain/AbstractScalableImageSource.php | 22 ++++++------------- Classes/Domain/AssetImageSource.php | 9 ++++++-- Classes/Domain/DummyImageSource.php | 13 ++++++++++- .../Domain/ScalableImageSourceInterface.php | 3 ++- .../Private/Fusion/Prototypes/Image.fusion | 4 +--- .../Private/Fusion/Prototypes/Picture.fusion | 3 --- .../Private/Fusion/Prototypes/Source.fusion | 4 +--- .../AbstractScalableImageSourceTest.php | 8 +++---- composer.json | 1 - 10 files changed, 35 insertions(+), 35 deletions(-) diff --git a/Classes/Domain/AbstractImageSource.php b/Classes/Domain/AbstractImageSource.php index 5dcf31f..5eb6757 100644 --- a/Classes/Domain/AbstractImageSource.php +++ b/Classes/Domain/AbstractImageSource.php @@ -220,11 +220,10 @@ public function withVariantPreset(string $presetIdentifier, string $presetVarian * Render sourceset Attribute non-scalable media. * * @param mixed $mediaDescriptors - * @param bool $allowUpScaling * * @return string */ - public function srcset($mediaDescriptors, bool $allowUpScaling = false): string + public function srcset($mediaDescriptors): string { return $this->src(); } diff --git a/Classes/Domain/AbstractScalableImageSource.php b/Classes/Domain/AbstractScalableImageSource.php index 96ddfe4..fa63e85 100644 --- a/Classes/Domain/AbstractScalableImageSource.php +++ b/Classes/Domain/AbstractScalableImageSource.php @@ -36,11 +36,6 @@ abstract class AbstractScalableImageSource extends AbstractImageSource implement */ protected $baseHeight; - /** - * @var bool - */ - protected $allowUpScaling = false; - /** * @param int|null $targetWidth * @param bool $preserveAspect @@ -103,14 +98,12 @@ public function withDimensions(int $targetWidth, int $targetHeight): ScalableIma /** * @param float $factor - * @param bool $allowUpScaling * * @return ScalableImageSourceInterface */ - public function scale(float $factor, bool $allowUpScaling = false): ScalableImageSourceInterface + public function scale(float $factor): ScalableImageSourceInterface { $scaledHelper = clone $this; - $scaledHelper->allowUpScaling = $allowUpScaling; if ($this->targetWidth && $this->targetHeight) { $scaledHelper = $scaledHelper->withDimensions((int) round($factor * $this->targetWidth), (int) round($factor * $this->targetHeight)); @@ -246,11 +239,10 @@ protected function createAdjustment(Adjustment $adjustmentConfiguration): ImageA * use the base width. * * @param $mediaDescriptors - * @param bool $allowUpScaling * * @return string */ - public function srcset($mediaDescriptors, bool $allowUpScaling = false): string + public function srcset($mediaDescriptors): string { $srcsetArray = []; @@ -281,24 +273,24 @@ public function srcset($mediaDescriptors, bool $allowUpScaling = false): string if ($srcsetType === 'width') { $width = (int)$matches['width']; $scaleFactor = $width / $this->width(); - if (!$allowUpScaling && ($width / $this->baseWidth > 1)) { + if (!$this->supportsUpscaling() && ($width / $this->baseWidth > 1)) { $srcsetArray[] = $this->src() . ' ' . $this->baseWidth . 'w'; } else { - $scaled = $this->scale($scaleFactor, $allowUpScaling); + $scaled = $this->scale($scaleFactor); $srcsetArray[] = $scaled->src() . ' ' . $width . 'w'; } } elseif ($srcsetType === 'factor') { $factor = (float)$matches['factor']; if ( - !$allowUpScaling && ( + !$this->supportsUpscaling() && ( ($this->targetHeight && ($maxScaleFactor < $factor)) || ($this->targetWidth && ($maxScaleFactor < $factor)) ) ) { - $scaled = $this->scale($maxScaleFactor, $allowUpScaling); + $scaled = $this->scale($maxScaleFactor); $srcsetArray[] = $scaled->src() . ' ' . $maxScaleFactor . 'x'; } else { - $scaled = $this->scale($factor, $allowUpScaling); + $scaled = $this->scale($factor); $srcsetArray[] = $scaled->src() . ' ' . $factor . 'x'; } } diff --git a/Classes/Domain/AssetImageSource.php b/Classes/Domain/AssetImageSource.php index 411fec9..0765109 100644 --- a/Classes/Domain/AssetImageSource.php +++ b/Classes/Domain/AssetImageSource.php @@ -69,6 +69,11 @@ public function __construct(ImageInterface $asset, ?string $title = null, ?strin $this->baseHeight = $this->asset->getHeight(); } + public function supportsUpscaling(): bool + { + return false; + } + /** * Use the variant generated from the given variant preset in this image source. * @@ -128,7 +133,7 @@ public function src(): string $async = $this->request ? $this->async : false; $allowCropping = true; - $allowUpScaling = $this->allowUpScaling; + $allowUpScaling = $this->supportsUpscaling(); $thumbnailConfiguration = new ThumbnailConfiguration( $width, $width, @@ -168,7 +173,7 @@ public function dataSrc(): string $async = false; $allowCropping = true; - $allowUpScaling = $this->allowUpScaling; + $allowUpScaling = $this->supportsUpscaling(); $thumbnailConfiguration = new ThumbnailConfiguration( $width, $width, diff --git a/Classes/Domain/DummyImageSource.php b/Classes/Domain/DummyImageSource.php index b19da61..70b46ad 100644 --- a/Classes/Domain/DummyImageSource.php +++ b/Classes/Domain/DummyImageSource.php @@ -35,6 +35,11 @@ class DummyImageSource extends AbstractScalableImageSource */ protected $baseUri; + /** + * @var bool + */ + private $allowUpScaling; + /** * @param string $baseUri * @param string|null $title @@ -45,7 +50,7 @@ class DummyImageSource extends AbstractScalableImageSource * @param string|null $foregroundColor * @param string|null $text */ - public function __construct(string $baseUri, ?string $title = null, ?string $alt = null, ?int $baseWidth = null, ?int $baseHeight = null, ?string $backgroundColor = null, ?string $foregroundColor = null, ?string $text = null) + public function __construct(string $baseUri, ?string $title = null, ?string $alt = null, ?int $baseWidth = null, ?int $baseHeight = null, ?string $backgroundColor = null, ?string $foregroundColor = null, ?string $text = null, bool $allowUpScaling = true) { parent::__construct($title, $alt); $this->baseUri = $baseUri; @@ -54,6 +59,12 @@ public function __construct(string $baseUri, ?string $title = null, ?string $alt $this->backgroundColor = $backgroundColor ?? '999'; $this->foregroundColor = $foregroundColor ?? 'fff'; $this->text = $text ?? ''; + $this->allowUpScaling = $allowUpScaling; + } + + public function supportsUpscaling(): bool + { + return $this->allowUpScaling; } /** diff --git a/Classes/Domain/ScalableImageSourceInterface.php b/Classes/Domain/ScalableImageSourceInterface.php index 72f8e62..5e72624 100644 --- a/Classes/Domain/ScalableImageSourceInterface.php +++ b/Classes/Domain/ScalableImageSourceInterface.php @@ -6,5 +6,6 @@ interface ScalableImageSourceInterface extends ImageSourceInterface { - public function scale(float $factor, bool $allowUpScaling = false): ImageSourceInterface; + public function supportsUpscaling(): bool; + public function scale(float $factor): ImageSourceInterface; } diff --git a/Resources/Private/Fusion/Prototypes/Image.fusion b/Resources/Private/Fusion/Prototypes/Image.fusion index c157b20..4354266 100644 --- a/Resources/Private/Fusion/Prototypes/Image.fusion +++ b/Resources/Private/Fusion/Prototypes/Image.fusion @@ -127,7 +127,6 @@ prototype(Sitegeist.Kaleidoscope:Image) < prototype(Neos.Fusion:Component) { quality = null attributes = Neos.Fusion:DataStructure renderDimensionAttributes = true - allowSrcsetUpScaling = false preserveAspect = true renderer = Neos.Fusion:Component { @@ -150,12 +149,11 @@ prototype(Sitegeist.Kaleidoscope:Image) < prototype(Neos.Fusion:Component) { class = ${props.class} attributes = ${props.attributes} renderDimensionAttributes = ${props.renderDimensionAttributes} - allowSrcsetUpScaling = ${props.allowSrcsetUpScaling} renderer = afx` diff --git a/Resources/Private/Fusion/Prototypes/Source.fusion b/Resources/Private/Fusion/Prototypes/Source.fusion index 7560058..9ef526b 100644 --- a/Resources/Private/Fusion/Prototypes/Source.fusion +++ b/Resources/Private/Fusion/Prototypes/Source.fusion @@ -14,7 +14,6 @@ prototype(Sitegeist.Kaleidoscope:Source) < prototype(Neos.Fusion:Component) { type = null media = null renderDimensionAttributes = true - allowSrcsetUpScaling = false preserveAspect = true renderer = Neos.Fusion:Component { @@ -44,11 +43,10 @@ prototype(Sitegeist.Kaleidoscope:Source) < prototype(Neos.Fusion:Component) { sizes = ${sizes} media = ${props.media} renderDimensionAttributes = ${props.renderDimensionAttributes} - allowSrcsetUpScaling = ${props.allowSrcsetUpScaling} renderer = afx` getDummyImageSource(400, 400); + $dummy = $this->getDummyImageSource(400, 400, true); $this->assertEquals( 'https://example.com?w=200&h=200&bg=999&fg=fff&t=Test 200w, https://example.com?w=400&h=400&bg=999&fg=fff&t=Test 400w, https://example.com?w=600&h=600&bg=999&fg=fff&t=Test 600w', - $dummy->srcset('200w, 400w, 600w', true) + $dummy->srcset('200w, 400w, 600w') ); } @@ -140,9 +140,9 @@ public function srcsetShouldSkipMissingDescriptors() ); } - protected function getDummyImageSource($width, $height) + protected function getDummyImageSource($width, $height, $allowUpScaling = false) { - $dummy = new DummyImageSource('https://example.com', 'Test', 'Test', $width, $height, '999', 'fff', 'Test'); + $dummy = new DummyImageSource('https://example.com', 'Test', 'Test', $width, $height, '999', 'fff', 'Test', $allowUpScaling); $this->inject($dummy, 'logger', $this->logger); return $dummy; } diff --git a/composer.json b/composer.json index 513d2b6..09c223c 100644 --- a/composer.json +++ b/composer.json @@ -4,7 +4,6 @@ "name": "sitegeist/kaleidoscope", "license": "GPL-3.0-or-later", "require": { - "php": "^7.4 || ^8.0", "neos/neos": "^7.0 || ^8.0 || ^9.0 || dev-master", "neos/fusion-afx": "^7.0 || ^8.0 || ^9.0 || dev-master", "neos/media": "*", From 60476f472195a186db4187df8235e675b70342e4 Mon Sep 17 00:00:00 2001 From: Manuel Meister Date: Thu, 20 Jun 2024 17:22:14 +0200 Subject: [PATCH 8/8] FIX: Remove preserveAspect --- Resources/Private/Fusion/Prototypes/Image.fusion | 6 ++---- Resources/Private/Fusion/Prototypes/Picture.fusion | 11 +++-------- Resources/Private/Fusion/Prototypes/Source.fusion | 8 +++----- 3 files changed, 8 insertions(+), 17 deletions(-) diff --git a/Resources/Private/Fusion/Prototypes/Image.fusion b/Resources/Private/Fusion/Prototypes/Image.fusion index 4354266..b9ca0b6 100644 --- a/Resources/Private/Fusion/Prototypes/Image.fusion +++ b/Resources/Private/Fusion/Prototypes/Image.fusion @@ -127,7 +127,6 @@ prototype(Sitegeist.Kaleidoscope:Image) < prototype(Neos.Fusion:Component) { quality = null attributes = Neos.Fusion:DataStructure renderDimensionAttributes = true - preserveAspect = true renderer = Neos.Fusion:Component { @if.hasImageSource = ${props.imageSource && Type.instance(props.imageSource, '\\Sitegeist\\Kaleidoscope\\Domain\\ImageSourceInterface')} @@ -135,9 +134,8 @@ prototype(Sitegeist.Kaleidoscope:Image) < prototype(Neos.Fusion:Component) { # apply format, width and height to the imageSource imageSource = ${props.imageSource} - imageSource.@process.applyDimensions = ${(props.width && props.height) ? value.withDimensions(props.width, props.height) : value} - imageSource.@process.applyWidth = ${(props.width && !props.height) ? value.withWidth(props.width, props.preserveAspect) : value} - imageSource.@process.applyHeight = ${(props.height && !props.width) ? value.withHeight(props.height, props.preserveAspect) : value} + imageSource.@process.applyWidth = ${props.width ? value.withWidth(props.width) : value} + imageSource.@process.applyHeight = ${props.height ? value.withHeight(props.height) : value} imageSource.@process.applyFormat = ${props.format ? value.withFormat(props.format) : value} imageSource.@process.applyQuality = ${props.quality ? value.withQuality(props.quality) : value} diff --git a/Resources/Private/Fusion/Prototypes/Picture.fusion b/Resources/Private/Fusion/Prototypes/Picture.fusion index cbea8ce..43fb779 100644 --- a/Resources/Private/Fusion/Prototypes/Picture.fusion +++ b/Resources/Private/Fusion/Prototypes/Picture.fusion @@ -65,7 +65,6 @@ prototype(Sitegeist.Kaleidoscope:Picture) < prototype(Neos.Fusion:Component) { imgAttributes = Neos.Fusion:DataStructure content = '' renderDimensionAttributes = true - preserveAspect = true # # put the values that shall be applied to sources automatically to the context @@ -86,10 +85,9 @@ prototype(Sitegeist.Kaleidoscope:Picture) < prototype(Neos.Fusion:Component) { # apply format, width and height to the imageSource imageSource = ${props.imageSource} - imageSource.@process.applyDimensions = ${(props.width && props.height) ? value.withDimensions(props.width, props.height) : value} - imageSource.@process.applyWidth = ${(props.width && !props.height) ? value.withWidth(props.width, props.preserveAspect) : value} - imageSource.@process.applyHeight = ${(props.height && !props.width) ? value.withHeight(props.height, props.preserveAspect) : value} - imageSource.@process.applyFormat = ${props.format ? value.withFormat(props.format) : value} + imageSource.@process.applyWidth = ${props.width ? value.setWidth(props.width) : value} + imageSource.@process.applyHeight = ${props.height ? value.setHeight(props.height) : value} + imageSource.@process.applyFormat = ${props.format ? value.setFormat(props.format) : value} imageSource.@process.applyQuality = ${props.quality ? value.setQuality(props.quality) : value} srcset = ${props.srcset} @@ -104,7 +102,6 @@ prototype(Sitegeist.Kaleidoscope:Picture) < prototype(Neos.Fusion:Component) { imgAttributes = ${props.imgAttributes} content = ${props.content} renderDimensionAttributes = ${props.renderDimensionAttributes} - preserveAspect = ${props.preserveAspect} renderer = afx` @@ -121,7 +118,6 @@ prototype(Sitegeist.Kaleidoscope:Picture) < prototype(Neos.Fusion:Component) { srcset={source.srcset ? source.srcset : props.srcset} sizes={source.sizes ? source.sizes : props.sizes} renderDimensionAttributes={props.renderDimensionAttributes} - preserveAspect={props.preserveAspect} /> ` diff --git a/Resources/Private/Fusion/Prototypes/Source.fusion b/Resources/Private/Fusion/Prototypes/Source.fusion index 9ef526b..abf914f 100644 --- a/Resources/Private/Fusion/Prototypes/Source.fusion +++ b/Resources/Private/Fusion/Prototypes/Source.fusion @@ -14,7 +14,6 @@ prototype(Sitegeist.Kaleidoscope:Source) < prototype(Neos.Fusion:Component) { type = null media = null renderDimensionAttributes = true - preserveAspect = true renderer = Neos.Fusion:Component { @@ -32,10 +31,9 @@ prototype(Sitegeist.Kaleidoscope:Source) < prototype(Neos.Fusion:Component) { isScalableSource = ${imageSource && Type.instance(imageSource, '\\Sitegeist\\Kaleidoscope\\Domain\\ScalableImageSourceInterface')} imageSource = ${imageSource} - imageSource.@process.applyDimensions = ${(props.width && props.height) ? value.withDimensions(props.width, props.height) : value} - imageSource.@process.applyWidth = ${(props.width && !props.height) ? value.withWidth(props.width, props.preserveAspect) : value} - imageSource.@process.applyHeight = ${(props.height && !props.width) ? value.withHeight(props.height, props.preserveAspect) : value} - imageSource.@process.applyFormat = ${props.format ? value.withFormat(props.format) : value} + imageSource.@process.applyWidth = ${width ? value.withWidth(width) : value} + imageSource.@process.applyHeight = ${height ? value.withHeight(height) : value} + imageSource.@process.applyFormat = ${format ? value.withFormat(format) : value} imageSource.@process.applyQuality = ${quality ? value.withQuality(quality) : value} type = ${format ? 'image/' + format : props.type}