From 5b216500976019734e67b3edd8b17c8e18c9c8cc 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 402732ecda992..91173d41afa12 100644 --- a/lib/private/Preview/Generator.php +++ b/lib/private/Preview/Generator.php @@ -128,7 +128,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); } @@ -173,7 +173,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()); @@ -201,14 +202,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; } @@ -340,7 +344,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()); @@ -356,12 +360,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); } @@ -381,4 +386,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 a7d702ac032d4..925548ef26da3 100644 --- a/lib/private/legacy/image.php +++ b/lib/private/legacy/image.php @@ -304,6 +304,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 faa68b28cb93bb7368a1817cbbe59880ce3bce5b 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 91173d41afa12..70a831530edb8 100644 --- a/lib/private/Preview/Generator.php +++ b/lib/private/Preview/Generator.php @@ -128,9 +128,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; @@ -173,7 +177,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); @@ -390,6 +401,7 @@ private function getPreviewFolder(File $file) { /** * @param string $mimeType * @return null|string + * @throws \InvalidArgumentException */ private function getExtention($mimeType) { switch ($mimeType) { @@ -400,7 +412,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 925548ef26da3..a0159b927f9d7 100644 --- a/lib/private/legacy/image.php +++ b/lib/private/legacy/image.php @@ -305,11 +305,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) {