diff --git a/app/Uploads/ImageRepo.php b/app/Uploads/ImageRepo.php index da0b7d379..01b65f882 100644 --- a/app/Uploads/ImageRepo.php +++ b/app/Uploads/ImageRepo.php @@ -2,6 +2,8 @@ use BookStack\Auth\Permissions\PermissionService; use BookStack\Entities\Page; +use BookStack\Exceptions\ImageUploadException; +use Exception; use Illuminate\Database\Eloquent\Builder; use Symfony\Component\HttpFoundation\File\UploadedFile; @@ -15,10 +17,6 @@ class ImageRepo /** * ImageRepo constructor. - * @param Image $image - * @param ImageService $imageService - * @param \BookStack\Auth\Permissions\PermissionService $permissionService - * @param \BookStack\Entities\Page $page */ public function __construct( Image $image, @@ -35,10 +33,8 @@ class ImageRepo /** * Get an image with the given id. - * @param $id - * @return Image */ - public function getById($id) + public function getById($id): Image { return $this->image->findOrFail($id); } @@ -46,13 +42,8 @@ class ImageRepo /** * Execute a paginated query, returning in a standard format. * Also runs the query through the restriction system. - * @param $query - * @param int $page - * @param int $pageSize - * @param bool $filterOnPage - * @return array */ - private function returnPaginated($query, $page = 1, $pageSize = 24) + private function returnPaginated($query, $page = 1, $pageSize = 24): array { $images = $query->orderBy('created_at', 'desc')->skip($pageSize * ($page - 1))->take($pageSize + 1)->get(); $hasMore = count($images) > $pageSize; @@ -71,13 +62,6 @@ class ImageRepo /** * Fetch a list of images in a paginated format, filtered by image type. * Can be filtered by uploaded to and also by name. - * @param string $type - * @param int $page - * @param int $pageSize - * @param int $uploadedTo - * @param string|null $search - * @param callable|null $whereClause - * @return array */ public function getPaginatedByType( string $type, @@ -86,7 +70,8 @@ class ImageRepo int $uploadedTo = null, string $search = null, callable $whereClause = null - ) { + ): array + { $imageQuery = $this->image->newQuery()->where('type', '=', strtolower($type)); if ($uploadedTo !== null) { @@ -109,13 +94,6 @@ class ImageRepo /** * Get paginated gallery images within a specific page or book. - * @param string $type - * @param string $filterType - * @param int $page - * @param int $pageSize - * @param int|null $uploadedTo - * @param string|null $search - * @return array */ public function getEntityFiltered( string $type, @@ -124,7 +102,8 @@ class ImageRepo int $pageSize = 24, int $uploadedTo = null, string $search = null - ) { + ): array + { $contextPage = $this->page->findOrFail($uploadedTo); $parentFilter = null; @@ -144,16 +123,9 @@ class ImageRepo /** * Save a new image into storage and return the new image. - * @param UploadedFile $uploadFile - * @param string $type - * @param int $uploadedTo - * @param int|null $resizeWidth - * @param int|null $resizeHeight - * @param bool $keepRatio - * @return Image - * @throws \BookStack\Exceptions\ImageUploadException + * @throws ImageUploadException */ - public function saveNew(UploadedFile $uploadFile, $type, $uploadedTo = 0, int $resizeWidth = null, int $resizeHeight = null, bool $keepRatio = true) + public function saveNew(UploadedFile $uploadFile, string $type, int $uploadedTo = 0, int $resizeWidth = null, int $resizeHeight = null, bool $keepRatio = true): Image { $image = $this->imageService->saveNewFromUpload($uploadFile, $type, $uploadedTo, $resizeWidth, $resizeHeight, $keepRatio); $this->loadThumbs($image); @@ -161,29 +133,22 @@ class ImageRepo } /** - * Save a drawing the the database; - * @param string $base64Uri - * @param int $uploadedTo - * @return Image - * @throws \BookStack\Exceptions\ImageUploadException + * Save a drawing the the database. + * @throws ImageUploadException */ - public function saveDrawing(string $base64Uri, int $uploadedTo) + public function saveDrawing(string $base64Uri, int $uploadedTo): Image { $name = 'Drawing-' . user()->getShortName(40) . '-' . strval(time()) . '.png'; - $image = $this->imageService->saveNewFromBase64Uri($base64Uri, $name, 'drawio', $uploadedTo); - return $image; + return $this->imageService->saveNewFromBase64Uri($base64Uri, $name, 'drawio', $uploadedTo); } /** * Update the details of an image via an array of properties. - * @param Image $image - * @param array $updateDetails - * @return Image - * @throws \BookStack\Exceptions\ImageUploadException - * @throws \Exception + * @throws ImageUploadException + * @throws Exception */ - public function updateImageDetails(Image $image, $updateDetails) + public function updateImageDetails(Image $image, $updateDetails): Image { $image->fill($updateDetails); $image->save(); @@ -191,14 +156,11 @@ class ImageRepo return $image; } - /** * Destroys an Image object along with its revisions, files and thumbnails. - * @param Image $image - * @return bool - * @throws \Exception + * @throws Exception */ - public function destroyImage(Image $image = null) + public function destroyImage(Image $image = null): bool { if ($image) { $this->imageService->destroy($image); @@ -208,8 +170,7 @@ class ImageRepo /** * Destroy all images of a certain type. - * @param string $imageType - * @throws \Exception + * @throws Exception */ public function destroyByType(string $imageType) { @@ -222,9 +183,7 @@ class ImageRepo /** * Load thumbnails onto an image object. - * @param Image $image - * @throws \BookStack\Exceptions\ImageUploadException - * @throws \Exception + * @throws Exception */ protected function loadThumbs(Image $image) { @@ -238,42 +197,33 @@ class ImageRepo * Get the thumbnail for an image. * If $keepRatio is true only the width will be used. * Checks the cache then storage to avoid creating / accessing the filesystem on every check. - * @param Image $image - * @param int $width - * @param int $height - * @param bool $keepRatio - * @return string - * @throws \BookStack\Exceptions\ImageUploadException - * @throws \Exception + * @throws Exception */ - protected function getThumbnail(Image $image, $width = 220, $height = 220, $keepRatio = false) + protected function getThumbnail(Image $image, ?int $width = 220, ?int $height = 220, bool $keepRatio = false): ?string { try { return $this->imageService->getThumbnail($image, $width, $height, $keepRatio); - } catch (\Exception $exception) { + } catch (Exception $exception) { return null; } } /** * Get the raw image data from an Image. - * @param Image $image - * @return null|string */ - public function getImageData(Image $image) + public function getImageData(Image $image): ?string { try { return $this->imageService->getImageData($image); - } catch (\Exception $exception) { + } catch (Exception $exception) { return null; } } /** * Get the validation rules for image files. - * @return string */ - public function getImageValidationRules() + public function getImageValidationRules(): string { return 'image_extension|no_double_extension|mimes:jpeg,png,gif,bmp,webp,tiff'; } diff --git a/app/Uploads/ImageService.php b/app/Uploads/ImageService.php index e7668471b..756149fe7 100644 --- a/app/Uploads/ImageService.php +++ b/app/Uploads/ImageService.php @@ -254,7 +254,16 @@ class ImageService extends UploadService } else { $thumb->fit($width, $height); } - return (string)$thumb->encode(); + + $thumbData = (string)$thumb->encode(); + + // Use original image data if we're keeping the ratio + // and the resizing does not save any space. + if ($keepRatio && strlen($thumbData) > strlen($imageData)) { + return $imageData; + } + + return $thumbData; } /** diff --git a/tests/Uploads/ImageTest.php b/tests/Uploads/ImageTest.php index 0615a95ce..3f6c021a7 100644 --- a/tests/Uploads/ImageTest.php +++ b/tests/Uploads/ImageTest.php @@ -36,6 +36,30 @@ class ImageTest extends TestCase ]); } + public function test_image_display_thumbnail_generation_does_not_increase_image_size() + { + $page = Page::first(); + $admin = $this->getAdmin(); + $this->actingAs($admin); + + $originalFile = $this->getTestImageFilePath('compressed.png'); + $originalFileSize = filesize($originalFile); + $imgDetails = $this->uploadGalleryImage($page, 'compressed.png'); + $relPath = $imgDetails['path']; + + $this->assertTrue(file_exists(public_path($relPath)), 'Uploaded image found at path: '. public_path($relPath)); + $displayImage = $imgDetails['response']->thumbs->display; + + $displayImageRelPath = implode('/', array_slice(explode('/', $displayImage), 3)); + $displayImagePath = public_path($displayImageRelPath); + $displayFileSize = filesize($displayImagePath); + + $this->deleteImage($relPath); + $this->deleteImage($displayImageRelPath); + + $this->assertEquals($originalFileSize, $displayFileSize, 'Display thumbnail generation should not increase image size'); + } + public function test_image_edit() { $editor = $this->getEditor(); diff --git a/tests/Uploads/UsesImages.php b/tests/Uploads/UsesImages.php index aa5ffe4c7..9ce559acd 100644 --- a/tests/Uploads/UsesImages.php +++ b/tests/Uploads/UsesImages.php @@ -10,9 +10,13 @@ trait UsesImages * Get the path to our basic test image. * @return string */ - protected function getTestImageFilePath() + protected function getTestImageFilePath(?string $fileName = null) { - return base_path('tests/test-data/test-image.png'); + if (is_null($fileName)) { + $fileName = 'test-image.png'; + } + + return base_path('tests/test-data/' . $fileName); } /** @@ -20,9 +24,9 @@ trait UsesImages * @param $fileName * @return UploadedFile */ - protected function getTestImage($fileName) + protected function getTestImage($fileName, ?string $testDataFileName = null) { - return new UploadedFile($this->getTestImageFilePath(), $fileName, 'image/png', 5238, null, true); + return new UploadedFile($this->getTestImageFilePath($testDataFileName), $fileName, 'image/png', 5238, null, true); } /** @@ -52,9 +56,9 @@ trait UsesImages * @param string $contentType * @return \Illuminate\Foundation\Testing\TestResponse */ - protected function uploadImage($name, $uploadedTo = 0, $contentType = 'image/png') + protected function uploadImage($name, $uploadedTo = 0, $contentType = 'image/png', ?string $testDataFileName = null) { - $file = $this->getTestImage($name); + $file = $this->getTestImage($name, $testDataFileName); return $this->withHeader('Content-Type', $contentType) ->call('POST', '/images/gallery', ['uploaded_to' => $uploadedTo], [], ['file' => $file], []); } @@ -66,22 +70,23 @@ trait UsesImages * @param Page|null $page * @return array */ - protected function uploadGalleryImage(Page $page = null) + protected function uploadGalleryImage(Page $page = null, ?string $testDataFileName = null) { if ($page === null) { $page = Page::query()->first(); } - $imageName = 'first-image.png'; + $imageName = $testDataFileName ?? 'first-image.png'; $relPath = $this->getTestImagePath('gallery', $imageName); $this->deleteImage($relPath); - $upload = $this->uploadImage($imageName, $page->id); + $upload = $this->uploadImage($imageName, $page->id, 'image/png', $testDataFileName); $upload->assertStatus(200); return [ 'name' => $imageName, 'path' => $relPath, - 'page' => $page + 'page' => $page, + 'response' => json_decode($upload->getContent()), ]; } diff --git a/tests/test-data/compressed.png b/tests/test-data/compressed.png new file mode 100644 index 000000000..e42ef58f0 Binary files /dev/null and b/tests/test-data/compressed.png differ