From 50b880edeba3166cd7718c1fb3447f618194337b Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 4 Jan 2018 10:00:07 +0100 Subject: [PATCH 1/2] Don't lie about the preview mimetype For legacy reasons we stored all the previews with a png extention. However we did not put png data in them all the time. This caused the preview endpoints to always report that a preview is a png file. Which was a lie. Since we abstract away from the storage etc in the previewmanager. There is no need anymore to store them as .png files and instead we can use the actual file extention. Signed-off-by: Roeland Jago Douma --- lib/private/Preview/Generator.php | 36 +++++++++++++++++++++++------ lib/private/legacy/image.php | 18 +++++++++++++++ lib/public/IImage.php | 6 +++++ tests/lib/Preview/GeneratorTest.php | 8 +++++++ 4 files changed, 61 insertions(+), 7 deletions(-) diff --git a/lib/private/Preview/Generator.php b/lib/private/Preview/Generator.php index 5a264c2bbd543..4f9f7f3bc1ff0 100644 --- a/lib/private/Preview/Generator.php +++ b/lib/private/Preview/Generator.php @@ -120,7 +120,7 @@ public function getPreview(File $file, $width = -1, $height = -1, $crop = false, // Try to get a cached preview. Else generate (and store) one try { - $file = $this->getCachedPreview($previewFolder, $width, $height, $crop); + $file = $this->getCachedPreview($previewFolder, $width, $height, $crop, $maxPreview->getMimeType()); } catch (NotFoundException $e) { $file = $this->generatePreview($previewFolder, $maxPreview, $width, $height, $crop, $maxWidth, $maxHeight); } @@ -165,7 +165,8 @@ private function getMaxPreview(ISimpleFolder $previewFolder, File $file, $mimeTy continue; } - $path = (string)$preview->width() . '-' . (string)$preview->height() . '-max.png'; + $ext = $this->getExtention($preview->dataMimeType()); + $path = (string)$preview->width() . '-' . (string)$preview->height() . '-max.' . $ext; try { $file = $previewFolder->newFile($path); $file->putContent($preview->data()); @@ -193,14 +194,17 @@ private function getPreviewSize(ISimpleFile $file) { * @param int $width * @param int $height * @param bool $crop + * @param string $mimeType * @return string */ - private function generatePath($width, $height, $crop) { + private function generatePath($width, $height, $crop, $mimeType) { $path = (string)$width . '-' . (string)$height; if ($crop) { $path .= '-crop'; } - $path .= '.png'; + + $ext = $this->getExtention($mimeType); + $path .= '.' . $ext; return $path; } @@ -332,7 +336,7 @@ private function generatePreview(ISimpleFolder $previewFolder, ISimpleFile $maxP } - $path = $this->generatePath($width, $height, $crop); + $path = $this->generatePath($width, $height, $crop, $preview->dataMimeType()); try { $file = $previewFolder->newFile($path); $file->putContent($preview->data()); @@ -348,12 +352,13 @@ private function generatePreview(ISimpleFolder $previewFolder, ISimpleFile $maxP * @param int $width * @param int $height * @param bool $crop + * @param string $mimeType * @return ISimpleFile * * @throws NotFoundException */ - private function getCachedPreview(ISimpleFolder $previewFolder, $width, $height, $crop) { - $path = $this->generatePath($width, $height, $crop); + private function getCachedPreview(ISimpleFolder $previewFolder, $width, $height, $crop, $mimeType) { + $path = $this->generatePath($width, $height, $crop, $mimeType); return $previewFolder->getFile($path); } @@ -373,4 +378,21 @@ private function getPreviewFolder(File $file) { return $folder; } + + /** + * @param string $mimeType + * @return null|string + */ + private function getExtention($mimeType) { + switch ($mimeType) { + case 'image/png': + return 'png'; + case 'image/jpeg': + return 'jpg'; + case 'image/gif': + return 'gif'; + default: + return null; + } + } } diff --git a/lib/private/legacy/image.php b/lib/private/legacy/image.php index 120b19d1cff12..eb7baf5134cd3 100644 --- a/lib/private/legacy/image.php +++ b/lib/private/legacy/image.php @@ -317,6 +317,24 @@ public function resource() { return $this->resource; } + /** + * @return null|string Returns the mimetype of the data + */ + public function dataMimeType() { + if (!$this->valid()) { + return null; + } + + switch ($this->mimeType) { + case 'image/png': + case 'image/jpeg': + case 'image/gif': + return $this->mimeType; + default: + return 'image/png'; + } + } + /** * @return null|string Returns the raw image data. */ diff --git a/lib/public/IImage.php b/lib/public/IImage.php index f63a1b8ca608d..70e8b3cff751e 100644 --- a/lib/public/IImage.php +++ b/lib/public/IImage.php @@ -102,6 +102,12 @@ public function save($filePath = null, $mimeType = null); */ public function resource(); + /** + * @return string Returns the raw data mimetype + * @since 13.0.0 + */ + public function dataMimeType(); + /** * @return string Returns the raw image data. * @since 8.1.0 diff --git a/tests/lib/Preview/GeneratorTest.php b/tests/lib/Preview/GeneratorTest.php index f1383b0691b4e..130cccdf09e2f 100644 --- a/tests/lib/Preview/GeneratorTest.php +++ b/tests/lib/Preview/GeneratorTest.php @@ -93,6 +93,8 @@ public function testGetCachedPreview() { $maxPreview = $this->createMock(ISimpleFile::class); $maxPreview->method('getName') ->willReturn('1000-1000-max.png'); + $maxPreview->method('getMimeType') + ->willReturn('image/png'); $previewFolder->method('getDirectoryListing') ->willReturn([$maxPreview]); @@ -170,6 +172,7 @@ public function testGetNewPreview() { $image->method('width')->willReturn(2048); $image->method('height')->willReturn(2048); $image->method('valid')->willReturn(true); + $image->method('dataMimeType')->willReturn('image/png'); $this->helper->method('getThumbnail') ->will($this->returnCallback(function ($provider, $file, $x, $y) use ($invalidProvider, $validProvider, $image) { @@ -185,6 +188,7 @@ public function testGetNewPreview() { $maxPreview = $this->createMock(ISimpleFile::class); $maxPreview->method('getName')->willReturn('2048-2048-max.png'); + $maxPreview->method('getMimeType')->willReturn('image/png'); $previewFile = $this->createMock(ISimpleFile::class); @@ -219,6 +223,7 @@ public function testGetNewPreview() { $image->method('data') ->willReturn('my resized data'); $image->method('valid')->willReturn(true); + $image->method('dataMimeType')->willReturn('image/png'); $previewFile->expects($this->once()) ->method('putContent') @@ -362,6 +367,8 @@ public function testCorrectSize($maxX, $maxY, $reqX, $reqY, $crop, $mode, $expec $maxPreview = $this->createMock(ISimpleFile::class); $maxPreview->method('getName') ->willReturn($maxX . '-' . $maxY . '-max.png'); + $maxPreview->method('getMimeType') + ->willReturn('image/png'); $previewFolder->method('getDirectoryListing') ->willReturn([$maxPreview]); @@ -382,6 +389,7 @@ public function testCorrectSize($maxX, $maxY, $reqX, $reqY, $crop, $mode, $expec $image->method('height')->willReturn($maxY); $image->method('width')->willReturn($maxX); $image->method('valid')->willReturn(true); + $image->method('dataMimeType')->willReturn('image/png'); $preview = $this->createMock(ISimpleFile::class); $previewFolder->method('newFile') From 87e99651f5d7b0f86bcae2a7366f6ae4f0600a87 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Sun, 7 Jan 2018 11:46:24 +0100 Subject: [PATCH 2/2] Only return 1 type Throw proper exception if we can't get the mimetype for a preview. Catch it later on so we can just return a not found for the preview. Signed-off-by: Roeland Jago Douma --- lib/private/Preview/Generator.php | 22 +++++++++++++++++----- lib/private/legacy/image.php | 5 +++-- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/lib/private/Preview/Generator.php b/lib/private/Preview/Generator.php index 4f9f7f3bc1ff0..db3b4e87f724e 100644 --- a/lib/private/Preview/Generator.php +++ b/lib/private/Preview/Generator.php @@ -120,9 +120,13 @@ public function getPreview(File $file, $width = -1, $height = -1, $crop = false, // Try to get a cached preview. Else generate (and store) one try { - $file = $this->getCachedPreview($previewFolder, $width, $height, $crop, $maxPreview->getMimeType()); - } catch (NotFoundException $e) { - $file = $this->generatePreview($previewFolder, $maxPreview, $width, $height, $crop, $maxWidth, $maxHeight); + try { + $file = $this->getCachedPreview($previewFolder, $width, $height, $crop, $maxPreview->getMimeType()); + } catch (NotFoundException $e) { + $file = $this->generatePreview($previewFolder, $maxPreview, $width, $height, $crop, $maxWidth, $maxHeight); + } + } catch (\InvalidArgumentException $e) { + throw new NotFoundException(); } return $file; @@ -165,7 +169,14 @@ private function getMaxPreview(ISimpleFolder $previewFolder, File $file, $mimeTy continue; } - $ext = $this->getExtention($preview->dataMimeType()); + // Try to get the extention. + try { + $ext = $this->getExtention($preview->dataMimeType()); + } catch (\InvalidArgumentException $e) { + // Just continue to the next iteration if this preview doesn't have a valid mimetype + continue; + } + $path = (string)$preview->width() . '-' . (string)$preview->height() . '-max.' . $ext; try { $file = $previewFolder->newFile($path); @@ -382,6 +393,7 @@ private function getPreviewFolder(File $file) { /** * @param string $mimeType * @return null|string + * @throws \InvalidArgumentException */ private function getExtention($mimeType) { switch ($mimeType) { @@ -392,7 +404,7 @@ private function getExtention($mimeType) { case 'image/gif': return 'gif'; default: - return null; + throw new \InvalidArgumentException('Not a valid mimetype'); } } } diff --git a/lib/private/legacy/image.php b/lib/private/legacy/image.php index eb7baf5134cd3..d132c0e3fd96e 100644 --- a/lib/private/legacy/image.php +++ b/lib/private/legacy/image.php @@ -318,11 +318,12 @@ public function resource() { } /** - * @return null|string Returns the mimetype of the data + * @return string Returns the mimetype of the data. Returns the empty string + * if the data is not valid. */ public function dataMimeType() { if (!$this->valid()) { - return null; + return ''; } switch ($this->mimeType) {