[BUGFIX] GifBuilder returns already generated images 44/59644/14
authorMarkus Klösges <mkloesges@gmx.de>
Tue, 5 Feb 2019 12:26:24 +0000 (13:26 +0100)
committerBenni Mack <benni@typo3.org>
Sat, 9 Mar 2019 13:48:13 +0000 (14:48 +0100)
When combining image files with gifbuilder, the hash to identify the
resulting file is now stable with respect to the fact whether the
files are cropped or scaled in the current request or already cropped
before. That leads to stable hashes whenever the same images are
processed with the same configuration, and allows reuse as intended.

Also ensure that fileInfo returned from ContentObjectRenderer contains
width and height information as int, when they are returned from
database as that may lead to different serialized representations of the
configuration

Resolves: #44518
Resolves: #86947
Resolves: #87224
Releases: 8.7, 9.5, master
Change-Id: I833585034cacaf5a0ad66ba3ff04ac3920421085
Reviewed-on: https://review.typo3.org/c/59644
Tested-by: TYPO3com <noreply@typo3.com>
Tested-by: Guido Schmechel <guido.schmechel@brandung.de>
Tested-by: Benni Mack <benni@typo3.org>
Reviewed-by: Oliver Klee <typo3-coding@oliverklee.de>
Reviewed-by: Guido Schmechel <guido.schmechel@brandung.de>
Reviewed-by: Wouter Wolters <typo3@wouterwolters.nl>
Reviewed-by: Stephan Großberndt <stephan.grossberndt@typo3.org>
Reviewed-by: Benni Mack <benni@typo3.org>
typo3/sysext/frontend/Classes/ContentObject/ContentObjectRenderer.php
typo3/sysext/frontend/Classes/Imaging/GifBuilder.php
typo3/sysext/frontend/Tests/Functional/Imaging/GifBuilderTest.php [new file with mode: 0644]

index e398d24..3e7d4a3 100644 (file)
@@ -4329,8 +4329,8 @@ class ContentObjectRenderer implements LoggerAwareInterface
                     $processedFileObject = $fileObject->process(ProcessedFile::CONTEXT_IMAGECROPSCALEMASK, $processingConfiguration);
                     if ($processedFileObject->isProcessed()) {
                         $imageResource = [
-                            0 => $processedFileObject->getProperty('width'),
-                            1 => $processedFileObject->getProperty('height'),
+                            0 => (int)$processedFileObject->getProperty('width'),
+                            1 => (int)$processedFileObject->getProperty('height'),
                             2 => $processedFileObject->getExtension(),
                             3 => $processedFileObject->getPublicUrl(),
                             'origFile' => $fileObject->getPublicUrl(),
index fa1e24a..d1e0813 100644 (file)
@@ -204,8 +204,15 @@ class GifBuilder extends GraphicalFunctions
                                     // Use normal path from fileInfo if it is a non-FAL file (even non-FAL files have originalFile set, but only non-FAL files have origFile set)
                                     $this->setup[$theKey . '.']['file'] = $fileInfo[3];
                                 }
-                                $this->setup[$theKey . '.']['BBOX'] = $fileInfo;
-                                $this->objBB[$theKey] = $fileInfo;
+
+                                // only pass necessary parts of fileInfo further down, to not incorporate facts as
+                                // CropScaleMask runs in this request, that may not occur in subsequent calls and change
+                                // the md5 of the generated file name
+                                $essentialFileInfo = $fileInfo;
+                                unset($essentialFileInfo['originalFile'], $essentialFileInfo['processedFile']);
+
+                                $this->setup[$theKey . '.']['BBOX'] = $essentialFileInfo;
+                                $this->objBB[$theKey] = $essentialFileInfo;
                                 if ($conf['mask']) {
                                     $maskInfo = $this->getResource($conf['mask'], $conf['mask.']);
                                     if ($maskInfo) {
diff --git a/typo3/sysext/frontend/Tests/Functional/Imaging/GifBuilderTest.php b/typo3/sysext/frontend/Tests/Functional/Imaging/GifBuilderTest.php
new file mode 100644 (file)
index 0000000..c6ba624
--- /dev/null
@@ -0,0 +1,80 @@
+<?php
+declare(strict_types = 1);
+namespace TYPO3\CMS\Frontend\Tests\Functional\ContentObject;
+
+/*
+ * This file is part of the TYPO3 CMS project.
+ *
+ * It is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License, either version 2
+ * of the License, or any later version.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE.txt file that was distributed with this source code.
+ *
+ * The TYPO3 project - inspiring people to share!
+ */
+
+use TYPO3\CMS\Core\Core\Environment;
+use TYPO3\CMS\Core\Resource\StorageRepository;
+use TYPO3\CMS\Frontend\Imaging\GifBuilder;
+
+/**
+ * Testcase for TYPO3\CMS\Frontend\Imaging\GifBuilder
+ */
+class GifBuilderTest extends \TYPO3\TestingFramework\Core\Functional\FunctionalTestCase
+{
+    /**
+     * Check hashes of Images overlayed with other images are idempotent
+     *
+     * @test
+     */
+    public function overlayImagesHasStableHash()
+    {
+        $this->importDataSet('PACKAGE:typo3/testing-framework/Resources/Core/Functional/Fixtures/sys_file_storage.xml');
+        $this->setUpBackendUserFromFixture(1);
+
+        copy(
+            Environment::getFrameworkBasePath() . '/frontend/Tests/Functional/Fixtures/Images/kasper-skarhoj1.jpg',
+            Environment::getPublicPath() . '/fileadmin/kasper-skarhoj1.jpg'
+        );
+
+        $storageRepository = (new StorageRepository())->findByUid(1);
+        $file = $storageRepository->getFile('kasper-skarhoj1.jpg');
+
+        $this->assertFalse($file->isMissing());
+
+        $fileArray = [
+            'XY' => '[10.w],[10.h]',
+            'format' => 'jpg',
+            'quality' => 88,
+            '10' => 'IMAGE',
+            '10.' => [
+                'file.width' => 300,
+                'file' => $file,
+            ],
+            '30' => 'IMAGE',
+            '30.' => [
+                'file' => $file,
+                'file.' => [
+                    'align' => 'l,t',
+                    'width' => 100
+                ]
+            ]
+        ];
+
+        $gifBuilder = new GifBuilder();
+        $gifBuilder->start($fileArray, []);
+        $setup1 = $gifBuilder->setup;
+        $fileName1 = $gifBuilder->gifBuild();
+
+        // Recreate a fresh GifBuilder instance, to catch inconsistencies in hashing for different instances
+        $gifBuilder = new GifBuilder();
+        $gifBuilder->start($fileArray, []);
+        $setup2 = $gifBuilder->setup;
+        $fileName2 = $gifBuilder->gifBuild();
+
+        $this->assertSame($setup1, $setup2, 'The Setup resulting from two equal configurations must be equal');
+        $this->assertSame($fileName1, $fileName2);
+    }
+}