Images: Rolled out image memory handling to image actions

- Moved thumnbail loading out of repo into ImageResizer. - Updated gallery and editor image handling to show errors where possible to indicate memory issues for resizing/thumbs. - Updated gallery to load image data in a per-image basis via edit form for more resiliant thumb/data fetching. Data was previously provided via gallery listing, which could be affected by failing generation of other images. - Updated image manager double click handling to be more pleasant and not flash away the edit form. - Updated editor handlers to use main URL when thumbs fail to load.
2024-10-30 07:32:39 +01:00 · 2023-10-01 13:05:18 +01:00 · 2023-10-01 13:05:18 +01:00 · b2d48d9a7f
commit b2d48d9a7f
parent 20bcbd76ef
15 changed files with 142 additions and 78 deletions
--- a/app/Uploads/Controllers/DrawioImageController.php
+++ b/app/Uploads/Controllers/DrawioImageController.php
@ -5,6 +5,8 @@ namespace BookStack\Uploads\Controllers;
 use BookStack\Exceptions\ImageUploadException;
 use BookStack\Http\Controller;
 use BookStack\Uploads\ImageRepo;
+use BookStack\Uploads\ImageResizer;
+use BookStack\Util\OutOfMemoryHandler;
 use Exception;
 use Illuminate\Http\Request;

@ -19,7 +21,7 @@ class DrawioImageController extends Controller
     * Get a list of gallery images, in a list.
     * Can be paged and filtered by entity.
     */
-    public function list(Request $request)
+    public function list(Request $request, ImageResizer $resizer)
    {
        $page = $request->get('page', 1);
        $searchTerm = $request->get('search', null);
@ -27,11 +29,20 @@ class DrawioImageController extends Controller
        $parentTypeFilter = $request->get('filter_type', null);

        $imgData = $this->imageRepo->getEntityFiltered('drawio', $parentTypeFilter, $page, 24, $uploadedToFilter, $searchTerm);
-
-        return view('pages.parts.image-manager-list', [
+        $viewData = [
+            'warning' => '',
            'images'  => $imgData['images'],
            'hasMore' => $imgData['has_more'],
-        ]);
+        ];
+
+        new OutOfMemoryHandler(function () use ($viewData) {
+            $viewData['warning'] = trans('errors.image_gallery_thumbnail_memory_limit');
+            return response()->view('pages.parts.image-manager-list', $viewData, 200);
+        });
+
+        $resizer->loadGalleryThumbnailsForMany($imgData['images']);
+
+        return view('pages.parts.image-manager-list', $viewData);
    }

    /**
--- a/app/Uploads/Controllers/GalleryImageController.php
+++ b/app/Uploads/Controllers/GalleryImageController.php
@ -5,6 +5,7 @@ namespace BookStack\Uploads\Controllers;
 use BookStack\Exceptions\ImageUploadException;
 use BookStack\Http\Controller;
 use BookStack\Uploads\ImageRepo;
+use BookStack\Uploads\ImageResizer;
 use BookStack\Util\OutOfMemoryHandler;
 use Illuminate\Http\Request;
 use Illuminate\Support\Facades\App;
@ -22,7 +23,7 @@ class GalleryImageController extends Controller
     * Get a list of gallery images, in a list.
     * Can be paged and filtered by entity.
     */
-    public function list(Request $request)
+    public function list(Request $request, ImageResizer $resizer)
    {
        $page = $request->get('page', 1);
        $searchTerm = $request->get('search', null);
@ -30,11 +31,20 @@ class GalleryImageController extends Controller
        $parentTypeFilter = $request->get('filter_type', null);

        $imgData = $this->imageRepo->getEntityFiltered('gallery', $parentTypeFilter, $page, 30, $uploadedToFilter, $searchTerm);
-
-        return view('pages.parts.image-manager-list', [
+        $viewData = [
+            'warning' => '',
            'images'  => $imgData['images'],
            'hasMore' => $imgData['has_more'],
-        ]);
+        ];
+
+        new OutOfMemoryHandler(function () use ($viewData) {
+            $viewData['warning'] = trans('errors.image_gallery_thumbnail_memory_limit');
+            return response()->view('pages.parts.image-manager-list', $viewData, 200);
+        });
+
+        $resizer->loadGalleryThumbnailsForMany($imgData['images']);
+
+        return view('pages.parts.image-manager-list', $viewData);
    }

    /**
--- a/app/Uploads/Controllers/ImageController.php
+++ b/app/Uploads/Controllers/ImageController.php
@ -4,9 +4,11 @@ namespace BookStack\Uploads\Controllers;

 use BookStack\Exceptions\ImageUploadException;
 use BookStack\Exceptions\NotFoundException;
+use BookStack\Exceptions\NotifyException;
 use BookStack\Http\Controller;
 use BookStack\Uploads\Image;
 use BookStack\Uploads\ImageRepo;
+use BookStack\Uploads\ImageResizer;
 use BookStack\Uploads\ImageService;
 use BookStack\Util\OutOfMemoryHandler;
 use Exception;
@ -16,7 +18,8 @@ class ImageController extends Controller
 {
    public function __construct(
        protected ImageRepo $imageRepo,
-        protected ImageService $imageService
+        protected ImageService $imageService,
+        protected ImageResizer $imageResizer,
    ) {
    }

@ -98,12 +101,20 @@ class ImageController extends Controller
            $dependantPages = $this->imageRepo->getPagesUsingImage($image);
        }

-        $this->imageRepo->loadThumbs($image, false);
-
-        return view('pages.parts.image-manager-form', [
+        $viewData = [
            'image'          => $image,
            'dependantPages' => $dependantPages ?? null,
-        ]);
+            'warning'        => '',
+        ];
+
+        new OutOfMemoryHandler(function () use ($viewData) {
+            $viewData['warning'] = trans('errors.image_thumbnail_memory_limit');
+            return response()->view('pages.parts.image-manager-form', $viewData);
+        });
+
+        $this->imageResizer->loadGalleryThumbnailsForImage($image, false);
+
+        return view('pages.parts.image-manager-form', $viewData);
    }

    /**
@ -135,15 +146,16 @@ class ImageController extends Controller
            return $this->jsonError(trans('errors.image_thumbnail_memory_limit'));
        });

-        $this->imageRepo->loadThumbs($image, true);
+        $this->imageResizer->loadGalleryThumbnailsForImage($image, true);

        return response(trans('components.image_rebuild_thumbs_success'));
    }

    /**
     * Check related page permission and ensure type is drawio or gallery.
+     * @throws NotifyException
     */
-    protected function checkImagePermission(Image $image)
+    protected function checkImagePermission(Image $image): void
    {
        if ($image->type !== 'drawio' && $image->type !== 'gallery') {
            $this->showPermissionError();
--- a/app/Uploads/Controllers/ImageGalleryApiController.php
+++ b/app/Uploads/Controllers/ImageGalleryApiController.php
@ -6,6 +6,7 @@ use BookStack\Entities\Models\Page;
 use BookStack\Http\ApiController;
 use BookStack\Uploads\Image;
 use BookStack\Uploads\ImageRepo;
+use BookStack\Uploads\ImageResizer;
 use Illuminate\Http\Request;

 class ImageGalleryApiController extends ApiController
@ -15,7 +16,8 @@ class ImageGalleryApiController extends ApiController
    ];

    public function __construct(
-        protected ImageRepo $imageRepo
+        protected ImageRepo $imageRepo,
+        protected ImageResizer $imageResizer,
    ) {
    }

@ -130,7 +132,7 @@ class ImageGalleryApiController extends ApiController
     */
    protected function formatForSingleResponse(Image $image): array
    {
-        $this->imageRepo->loadThumbs($image, false);
+        $this->imageResizer->loadGalleryThumbnailsForImage($image, false);
        $data = $image->toArray();
        $data['created_by'] = $image->createdBy;
        $data['updated_by'] = $image->updatedBy;
@ -138,6 +140,7 @@ class ImageGalleryApiController extends ApiController

        $escapedUrl = htmlentities($image->url);
        $escapedName = htmlentities($image->name);
+
        if ($image->type === 'drawio') {
            $data['content']['html'] = "<div drawio-diagram=\"{$image->id}\"><img src=\"{$escapedUrl}\"></div>";
            $data['content']['markdown'] = $data['content']['html'];
--- a/app/Uploads/Image.php
+++ b/app/Uploads/Image.php
@ -52,7 +52,7 @@ class Image extends Model
     */
    public function getThumb(?int $width, ?int $height, bool $keepRatio = false): ?string
    {
-        return app()->make(ImageResizer::class)->resizeToThumbnailUrl($this, $width, $height, $keepRatio, false, true);
+        return app()->make(ImageResizer::class)->resizeToThumbnailUrl($this, $width, $height, $keepRatio, false);
    }

    /**
--- a/app/Uploads/ImageRepo.php
+++ b/app/Uploads/ImageRepo.php
@ -30,19 +30,13 @@ class ImageRepo
     * Execute a paginated query, returning in a standard format.
     * Also runs the query through the restriction system.
     */
-    private function returnPaginated(Builder $query, int $page = 1, int $pageSize = 24): array
+    protected function returnPaginated(Builder $query, int $page = 1, int $pageSize = 24): array
    {
        $images = $query->orderBy('created_at', 'desc')->skip($pageSize * ($page - 1))->take($pageSize + 1)->get();
-        $hasMore = count($images) > $pageSize;
-
-        $returnImages = $images->take($pageSize);
-        $returnImages->each(function (Image $image) {
-            $this->loadThumbs($image, false);
-        });

        return [
-            'images'   => $returnImages,
-            'has_more' => $hasMore,
+            'images'   => $images->take($pageSize),
+            'has_more' => count($images) > $pageSize,
        ];
    }

@ -120,7 +114,7 @@ class ImageRepo
        $image = $this->imageService->saveNewFromUpload($uploadFile, $type, $uploadedTo, $resizeWidth, $resizeHeight, $keepRatio);

        if ($type !== 'system') {
-            $this->loadThumbs($image, true);
+            $this->imageResizer->loadGalleryThumbnailsForImage($image, true);
        }

        return $image;
@ -134,7 +128,7 @@ class ImageRepo
    public function saveNewFromData(string $imageName, string $imageData, string $type, int $uploadedTo = 0): Image
    {
        $image = $this->imageService->saveNew($imageName, $imageData, $type, $uploadedTo);
-        $this->loadThumbs($image, true);
+        $this->imageResizer->loadGalleryThumbnailsForImage($image, true);

        return $image;
    }
@ -161,7 +155,7 @@ class ImageRepo
        $image->fill($updateDetails);
        $image->updated_by = user()->id;
        $image->save();
-        $this->loadThumbs($image, false);
+        $this->imageResizer->loadGalleryThumbnailsForImage($image, false);

        return $image;
    }
@ -182,7 +176,7 @@ class ImageRepo
        $image->save();

        $this->imageService->replaceExistingFromUpload($image->path, $image->type, $file);
-        $this->loadThumbs($image, true);
+        $this->imageResizer->loadGalleryThumbnailsForImage($image, true);
    }

    /**
@ -214,29 +208,6 @@ class ImageRepo
        }
    }

-    /**
-     * Load thumbnails onto an image object.
-     */
-    public function loadThumbs(Image $image, bool $shouldCreate): void
-    {
-        $image->setAttribute('thumbs', [
-            'gallery' => $this->getThumbnail($image, 150, 150, false, $shouldCreate),
-            'display' => $this->getThumbnail($image, 1680, null, true, $shouldCreate),
-        ]);
-    }
-
-    /**
-     * Get a thumbnail URL for the given image.
-     */
-    protected function getThumbnail(Image $image, ?int $width, ?int $height, bool $keepRatio, bool $shouldCreate): ?string
-    {
-        try {
-            return $this->imageResizer->resizeToThumbnailUrl($image, $width, $height, $keepRatio, $shouldCreate);
-        } catch (Exception $exception) {
-            return null;
-        }
-    }
-
    /**
     * Get the raw image data from an Image.
     */
--- a/app/Uploads/ImageResizer.php
+++ b/app/Uploads/ImageResizer.php
@ -11,12 +11,42 @@ use Intervention\Image\ImageManager;

 class ImageResizer
 {
+    protected const THUMBNAIL_CACHE_TIME = 604_800; // 1 week
+
    public function __construct(
        protected ImageManager $intervention,
        protected ImageStorage $storage,
    ) {
    }

+    /**
+     * Load gallery thumbnails for a set of images.
+     * @param iterable<Image> $images
+     */
+    public function loadGalleryThumbnailsForMany(iterable $images, bool $shouldCreate = false): void
+    {
+        foreach ($images as $image) {
+            $this->loadGalleryThumbnailsForImage($image, $shouldCreate);
+        }
+    }
+
+    /**
+     * Load gallery thumbnails into the given image instance.
+     */
+    public function loadGalleryThumbnailsForImage(Image $image, bool $shouldCreate): void
+    {
+        $thumbs = ['gallery' => null, 'display' => null];
+
+        try {
+            $thumbs['gallery'] = $this->resizeToThumbnailUrl($image, 150, 150, false, $shouldCreate);
+            $thumbs['display'] = $this->resizeToThumbnailUrl($image, 1680, null, true, $shouldCreate);
+        } catch (Exception $exception) {
+            // Prevent thumbnail errors from stopping execution
+        }
+
+        $image->setAttribute('thumbs', $thumbs);
+    }
+
    /**
     * Get the thumbnail for an image.
     * If $keepRatio is true only the width will be used.
@ -29,8 +59,7 @@ class ImageResizer
        ?int $width,
        ?int $height,
        bool $keepRatio = false,
-        bool $shouldCreate = false,
-        bool $canCreate = false,
+        bool $shouldCreate = false
    ): ?string {
        // Do not resize GIF images where we're not cropping
        if ($keepRatio && $this->isGif($image)) {
@ -52,7 +81,7 @@ class ImageResizer
        // If thumbnail has already been generated, serve that and cache path
        $disk = $this->storage->getDisk($image->type);
        if (!$shouldCreate && $disk->exists($thumbFilePath)) {
-            Cache::put($thumbCacheKey, $thumbFilePath, 60 * 60 * 72);
+            Cache::put($thumbCacheKey, $thumbFilePath, static::THUMBNAIL_CACHE_TIME);

            return $this->storage->getPublicUrl($thumbFilePath);
        }
@ -61,19 +90,15 @@ class ImageResizer

        // Do not resize apng images where we're not cropping
        if ($keepRatio && $this->isApngData($image, $imageData)) {
-            Cache::put($thumbCacheKey, $image->path, 60 * 60 * 72);
+            Cache::put($thumbCacheKey, $image->path, static::THUMBNAIL_CACHE_TIME);

            return $this->storage->getPublicUrl($image->path);
        }

-        if (!$shouldCreate && !$canCreate) {
-            return null;
-        }
-
        // If not in cache and thumbnail does not exist, generate thumb and cache path
        $thumbData = $this->resizeImageData($imageData, $width, $height, $keepRatio);
        $disk->put($thumbFilePath, $thumbData, true);
-        Cache::put($thumbCacheKey, $thumbFilePath, 60 * 60 * 72);
+        Cache::put($thumbCacheKey, $thumbFilePath, static::THUMBNAIL_CACHE_TIME);

        return $this->storage->getPublicUrl($thumbFilePath);
    }
--- a/lang/en/errors.php
+++ b/lang/en/errors.php
@ -51,8 +51,9 @@ return [
    'image_upload_error' => 'An error occurred uploading the image',
    'image_upload_type_error' => 'The image type being uploaded is invalid',
    'image_upload_replace_type' => 'Image file replacements must be of the same type',
-    'image_upload_memory_limit' => 'Failed to handle image upload and/or create thumbnails due to system resource limits',
-    'image_thumbnail_memory_limit' => 'Failed to create image size variations due to system resource limits',
+    'image_upload_memory_limit' => 'Failed to handle image upload and/or create thumbnails due to system resource limits.',
+    'image_thumbnail_memory_limit' => 'Failed to create image size variations due to system resource limits.',
+    'image_gallery_thumbnail_memory_limit' => 'Failed to create gallery thumbnails due to system resource limits.',
    'drawing_data_not_found' => 'Drawing data could not be loaded. The drawing file might no longer exist or you may not have permission to access it.',

    // Attachments
--- a/resources/js/components/image-manager.js
+++ b/resources/js/components/image-manager.js
@ -1,6 +1,4 @@
-import {
-    onChildEvent, onSelect, removeLoading, showLoading,
-} from '../services/dom';
+import {onChildEvent, onSelect, removeLoading, showLoading,} from '../services/dom';
 import {Component} from './component';

 export class ImageManager extends Component {
@ -229,8 +227,8 @@ export class ImageManager extends Component {
        this.loadGallery();
    }

-    onImageSelectEvent(event) {
-        const image = JSON.parse(event.detail.data);
+    async onImageSelectEvent(event) {
+        let image = JSON.parse(event.detail.data);
        const isDblClick = ((image && image.id === this.lastSelected.id)
            && Date.now() - this.lastSelectedTime < 400);
        const alreadySelected = event.target.classList.contains('selected');
@ -238,12 +236,15 @@ export class ImageManager extends Component {
            el.classList.remove('selected');
        });

-        if (!alreadySelected) {
+        if (!alreadySelected && !isDblClick) {
            event.target.classList.add('selected');
-            this.loadImageEditForm(image.id);
-        } else {
+            image = await this.loadImageEditForm(image.id);
+        } else if (!isDblClick) {
            this.resetEditForm();
+        } else if (isDblClick) {
+            image = this.lastSelected;
        }
+
        this.selectButton.classList.toggle('hidden', alreadySelected);

        if (isDblClick && this.callback) {
@ -265,6 +266,9 @@ export class ImageManager extends Component {
        this.formContainer.innerHTML = formHtml;
        this.formContainerPlaceholder.setAttribute('hidden', '');
        window.$components.init(this.formContainer);
+
+        const imageDataEl = this.formContainer.querySelector('#image-manager-form-image-data');
+        return JSON.parse(imageDataEl.text);
    }

    runLoadMore() {
--- a/resources/js/markdown/actions.js
+++ b/resources/js/markdown/actions.js
@ -34,7 +34,7 @@ export class Actions {
        const imageManager = window.$components.first('image-manager');

        imageManager.show(image => {
-            const imageUrl = image.thumbs.display || image.url;
+            const imageUrl = image.thumbs?.display || image.url;
            const selectedText = this.#getSelectionText();
            const newText = `[![${selectedText || image.name}](${imageUrl})](${image.url})`;
            this.#replaceSelection(newText, newText.length);
@ -417,7 +417,7 @@ export class Actions {
            const newContent = `[![](${data.thumbs.display})](${data.url})`;
            this.#findAndReplaceContent(placeHolderText, newContent);
        } catch (err) {
-            window.$events.emit('error', this.editor.config.text.imageUploadError);
+            window.$events.error(err?.data?.message || this.editor.config.text.imageUploadError);
            this.#findAndReplaceContent(placeHolderText, '');
            console.error(err);
        }
--- a/resources/js/wysiwyg/drop-paste-handling.js
+++ b/resources/js/wysiwyg/drop-paste-handling.js
@ -61,7 +61,7 @@ function paste(editor, options, event) {
                editor.dom.replace(newEl, id);
            }).catch(err => {
                editor.dom.remove(id);
-                window.$events.emit('error', options.translations.imageUploadErrorText);
+                window.$events.error(err?.data?.message || options.translations.imageUploadErrorText);
                console.error(err);
            });
        }, 10);
--- a/resources/js/wysiwyg/plugins-imagemanager.js
+++ b/resources/js/wysiwyg/plugins-imagemanager.js
@ -11,7 +11,7 @@ function register(editor) {
            /** @type {ImageManager} * */
            const imageManager = window.$components.first('image-manager');
            imageManager.show(image => {
-                const imageUrl = image.thumbs.display || image.url;
+                const imageUrl = image.thumbs?.display || image.url;
                let html = `<a href="${image.url}" target="_blank">`;
                html += `<img src="${imageUrl}" alt="${image.name}">`;
                html += '</a>';
--- a/resources/sass/_components.scss
+++ b/resources/sass/_components.scss
@ -457,6 +457,18 @@ body.flexbox-support #entity-selector-wrap .popup-body .form-group {
  text-align: center;
 }

+.image-manager-list .image-manager-list-warning {
+  grid-column: 1 / -1;
+  aspect-ratio: auto;
+}
+
+.image-manager-warning {
+  @include lightDark(background, #FFF, #333);
+  color: var(--color-warning);
+  font-weight: bold;
+  border-inline: 3px solid var(--color-warning);
+}
+
 .image-manager-sidebar {
  width: 300px;
  margin: 0 auto;
--- a/resources/views/pages/parts/image-manager-form.blade.php
+++ b/resources/views/pages/parts/image-manager-form.blade.php
@ -8,8 +8,17 @@
     option:dropzone:file-accept="image/*"
     class="image-manager-details">

+    @if($warning ?? '')
+        <div class="image-manager-warning px-m py-xs flex-container-row gap-xs items-center mb-l">
+            <div>@icon('warning')</div>
+            <div class="flex">{{ $warning }}</div>
+        </div>
+    @endif
+
    <div refs="dropzone@status-area dropzone@drop-target"></div>

+    <script id="image-manager-form-image-data" type="application/json">@json($image)</script>
+
    <form component="ajax-form"
          option:ajax-form:success-message="{{ trans('components.image_update_success') }}"
          option:ajax-form:method="put"
--- a/resources/views/pages/parts/image-manager-list.blade.php
+++ b/resources/views/pages/parts/image-manager-list.blade.php
@ -1,3 +1,9 @@
+@if($warning ?? '')
+    <div class="image-manager-list-warning image-manager-warning px-m py-xs flex-container-row gap-xs items-center">
+        <div>@icon('warning')</div>
+        <div class="flex">{{ $warning }}</div>
+    </div>
+@endif
@foreach($images as $index => $image)
 <div>
    <button component="event-emit-select"
@ -5,7 +11,7 @@
         option:event-emit-select:data="{{ json_encode($image) }}"
         class="image anim fadeIn text-link"
         style="animation-delay: {{ min($index * 10, 260) . 'ms' }};">
-        <img src="{{ $image->thumbs['gallery'] }}"
+        <img src="{{ $image->thumbs['gallery'] ?? '' }}"
             alt="{{ $image->name }}"
             role="none"
             width="150"