Added creation of custom image sizes on request #10184

allouis · 2018-11-19T09:59:46Z

This implements the custom image sizes to work with #10182

Changes

`saveRaw` method on the `LocalFileStorage`

This updates the LocalFileStorage to have the ability to take a buffer and a full path, and store it, overwriting anything that was already there. This is a destructive method and shouldn't be used lightly.

We need this method to ensure that we can write resized images exactly where we want them to be, not based on the current month or year. Being able to write a buffer means we don't have to save a temp file and then delete it.

`delete` method on the `LocalFileStorage`

This adds the delete method used in the ThemeStorage class to the LocalFileStorage - it's need to remove the sizes directory when the theme is changed. I've also removed the method from ThemeStorage as it inherits from LocalFileStorage

`handleImageSize` middleware

This middleware is added to the to the /content/<images!!> path and will detect an image beginning with /size/<requestedSize> - after which 1 of 3 things will happen

That image exists, it is served
That image does not exist, but we have the config for that size, created the image, and then GOTO 1
That image does not exist, and we have no config for that size, redirect to the image without the /size/<requestedSize> prefix.

If the prefix is not detected this middleware will fall through.

Theme service `activate` method deletes `sizes` directory

~~It also adds some code the the theme service, to delete the sizes directory whenever a theme is activated.~~

This i'm not 100% on - I feel like this should be handled in n event listen for the 'services.themes.activated' event, but couldn't decide on where that listener should be created. As the sizes directory is theme specific (it relies on config in the themes package.json) this didn't seem like an inappropriate place to put it - just not the best.

ErisDS · 2018-11-26T13:47:38Z

I have concerns that deleting the image file cache on each activation is 1) unnecessary and 2) heavy. I think this can be avoided by using a file path which uses a 1:1 ref to the image file.

E.g.
either /100x200/fill/ & /100x200/fit/
or a simple rep like 100x200 for the default of fill and 100x200t for fit.
the specifics don't really matter.

Doing this would prevent us from needing to clear the cache, as the same name would never be used to reference different sizes. I think this also prevents the need for the saveRaw method.

If we want to still delete images to prevent buildup, we can, but with the 1:1 reference, we can reduce the load down to only deleting images when a size is removed from the theme. In this case, there's no chance of edge cases when requests cross over (i.e. a deletion and creation are called at the same time). There should be significantly less "file thrashing" and I think there are other ways in which a more specific naming convention affords us more flexibility.

If we are keeping the saveRaw function around, I would suggest calling it forceSave but that's a minor detail.

If we want to keep the existing approach, it IMO needs load testing.

JohnONolan · 2018-11-26T14:48:03Z

Am fine with those suggestions from product standpoint, and actually this does match cloudinary a bit closer tbh - https://cloudinary.com/documentation/image_transformations

So could take some inspiration from naming

https://res.cloudinary.com/demo/image/upload/w_300,h_200,c_crop/sample.jpg

as

https://demo.ghost.io/content/images/size/w300/image.jpg

Skip the comma, and we have no concept of crop mode atm so skip that too. (Note renamed sizes to size in path

Could we at least clear image cache whenever Ghost upgrades though? Is that acceptable

ErisDS · 2018-11-26T14:58:20Z

Could we at least clear image cache whenever Ghost upgrades though? Is that acceptable

It's totally acceptable to me conceptually.

I'm only not 100% that we know when Ghost upgrades to hang an event off of 🤔Which is weird, and explains why we struggle with update notifications 🤔 🤔 🤔 🤔 I can think of several future things we'd want that for...

At the very very least, we know when we do a database migration, and can definitely clear the image cache on those right now.

ErisDS · 2018-11-26T15:04:11Z

P.S. I think cloudinary URLs are perfect ref material here, but doesn't

https://res.cloudinary.com/demo/image/upload/w_300,h_200,c_crop/sample.jpg

become

https://demo.ghost.io/content/images/size/w300/h200/sample.jpg

or no slash

https://demo.ghost.io/content/images/size/w300h200/sample.jpg

If both w & h are specified?

Also, what happened to fit & fill crop modes from the spec?!

JohnONolan · 2018-11-26T15:41:41Z

https://demo.ghost.io/content/images/size/w300h200/sample.jpg

yeah that one

Also, what happened to fit & fill crop modes from the spec?

CSS happened

.image-class {
  object-fit: fill;
  object-fit: contain;
  object-fit: cover;
  object-fit: none;
  object-fit: scale-down;
}

Can certainly add more image cropping later if we want but actually not that essential for the sake of shipping a first version

ErisDS · 2018-11-26T16:46:07Z

👍

JohnONolan · 2018-11-28T05:44:57Z

If we are keeping the saveRaw function around, I would suggest calling it forceSave but that's a minor detail.

Forgot to add, I'm not sure about this suggestion. Egg ran naming past me for this, and the way I understood it was to mean that the regular save method also does [other things] - and the purpose of saveRaw is as an unsafe/unmoderated method which doesn't add anything or protect anything.

To me Raw indicates both "no additions, and no protection" - while Force only encompasses "no protection"

allouis · 2018-12-03T07:11:03Z

Updated this to no longer do any "garbage collection" of the resized images and to work correctly with the dimension urls

core/server/adapters/storage/LocalFileStorage.js

core/server/web/shared/middlewares/image/handle-image-sizes.js

+const storage = require('../../../../adapters/storage');
+const activeTheme = require('../../../../services/themes/active');
+
+const SIZE_PATH_REGEX = /^\/size\/([^/]+)\//;


core/server/web/shared/middlewares/image/handle-image-sizes.js

+
+    // CASE: unknown size or missing size config
+    const imageDimensionConfig = imageDimensions[requestedDimension];
+    if (!imageDimensionConfig || (!imageDimensionConfig.width && !imageDimensionConfig.height)) {


core/server/web/shared/middlewares/image/handle-image-sizes.js

naz

The change that needs addressing is #10184 (comment), the rest is more of an optional styling improvement. Would also be nice to get some unit test in 👍

core/test/unit/web/middleware/image/handle-image-sizes_spec.js

allouis · 2018-12-03T13:05:56Z

@gargol I've updated this PR - address the main issue, and added some test - they're a start.

core/server/web/shared/middlewares/image/handle-image-sizes.js

@@ -0,0 +1,66 @@
+const path = require('path');
+const sharp = require('sharp');


ErisDS · 2018-12-04T09:18:32Z

Need to raise a followup issue for this to reinstate deletion on update as a dependency on #10236.

This reverts commit 9204dfcc73a6a79d597dbf23651817bcbfc59991.

This reverts commit b45fdb405a05faeaf4bd87e977c4ac64ff96b057.

This reverts commit a587cd6bae45b68a293b2d5cfd9b7705a29e7bfa.

Co-Authored-By: allouis <[email protected]>

allouis · 2018-12-13T11:50:23Z

@ErisDS @gargol This has moved the sharp stuff into the manipulator lib and checks that sharp exists. I think the manipulator lib should be refactored to reuses the resize method created here - but i'm not going to do that in this PR 👻

kirrg001 · 2018-12-13T11:56:15Z

core/server/lib/image/manipulator.js

+        require('sharp');
+        return resizeImage(buffer, options);
+    } catch (e) {
+        return Promise.resolve(buffer);


FYI design hint: libraries should return an error, callers need to decide what to do if an error is thrown/returned. Callers can ignore errors, callers can return errors to the next chain. Whatever...

Furthermore: If sharp isn't installed or resizeImage throws an error, we would save the same buffer stream as the resized image? Is that wanted? 🤔

The lib returns a promise, which will reject if an error occurs, so the consumer of this is totally capable of catching any errors.

resizeImage returns promise, so errors from there will be handled by the library consumer - if sharp doesn't exist then yeah - we will save the resizedImage - i think this is a reasonable fallback for the functionality. @ErisDS what do you think?

The lib returns a promise, which will reject if an error occurs, so the consumer of this is totally capable of catching any errors.

IMO a library should always return a specific/custom/handled error. Otherwise, multiple/different callers need to handle an error from a third party library or from node. Plus the callers would need to wrap the error in case they want to forward the error.

Hmm yeah - I think a specific error is good, I'm not sure how specific we can be on top of sharps errors though.

I noticed in this file we use InternalServer errors but that doesn't seem correct as the library has nothing to do with servers it just deals with image buffers and the filesystem 🦐

Do you think we should create some image manipulation errors to use in the module?

I noticed in this file we use InternalServer errors but that doesn't seem correct as the library has nothing to do with servers it just deals with image buffers and the filesystem 🦐

There are many definitions of what an internat server error is.

Do you think we should create some image manipulation errors to use in the module?

In general: We did not create custom error types for custom situations in the past, because

an error is mostly http driven

the most common http codes are reflected in our error set

we can granulary define an error using "message", "type", "statusCode"

There is no need to block this PR, but it's important that all of us have a common thinking of how libraries are designed, how error handling works etc :)

resizeImage returns promise, so errors from there will be handled by the library consumer - if sharp doesn't exist then yeah - we will save the resizedImage - i think this is a reasonable fallback for the functionality.

This just needs answering 👍

Merging as is - will do a bit of clean up later ☺️

naz · 2018-12-13T12:23:02Z

core/server/lib/image/manipulator.js

 module.exports.process = process;
+module.exports.safeResizeImage = (buffer, options) => {
+    try {


Like this separation of a check logic for sharp being installed, maybe we should unify this handling for both safeResizeImage & process to keep it DRY? 🤔

I like that idea, but I think can be done in a separate PR so that we can refactor them to share some sharp code too ☺️

@todo

refs #10181 * Added initial handleImageSizes middleware * Implemented saveRaw method on local file storage * Wired up handleImageSizes middleware * Implemented delete for LocalFileStorage * Removed delete method from theme Storage class * Deleted sizes directory when theme is activated * Ensured that smaller images are not enlarged * Renamed sizes -> size * Exited middleware as early as possible * Called getStorage as late as possible * Updated image sizes middleware to handle dimension paths * Revert "Deleted sizes directory when theme is activated" This reverts commit 9204dfcc73a6a79d597dbf23651817bcbfc59991. * Revert "Removed delete method from theme Storage class" This reverts commit b45fdb405a05faeaf4bd87e977c4ac64ff96b057. * Revert "Implemented delete for LocalFileStorage" This reverts commit a587cd6bae45b68a293b2d5cfd9b7705a29e7bfa. * Fixed typo Co-Authored-By: allouis <[email protected]> * Redirected to original image if no image_sizes config * Refactored redirection because rule of three * Updated comments * Added rubbish tests * Added @todo comment for handleImageSizes tests * Added safeResizeImage method to image manipulator * Used image manipulator lib in image_size middleware

allouis assigned naz Nov 26, 2018

allouis changed the title ~~[WIP] Create custom image sizes on request~~ Create custom image sizes on request Nov 26, 2018

allouis changed the title ~~Create custom image sizes on request~~ Added creation of custom image sizes on request Nov 26, 2018

JohnONolan mentioned this pull request Nov 26, 2018

Responsive images TryGhost/Casper#505

Merged

naz reviewed Dec 3, 2018

View reviewed changes

core/server/adapters/storage/LocalFileStorage.js Outdated Show resolved Hide resolved

naz reviewed Dec 3, 2018

View reviewed changes

core/server/web/shared/middlewares/image/handle-image-sizes.js

const storage = require('../../../../adapters/storage');

const activeTheme = require('../../../../services/themes/active');

const SIZE_PATH_REGEX = /^\/size\/([^/]+)\//;

This comment was marked as abuse.

Sign in to view

naz reviewed Dec 3, 2018

View reviewed changes

core/server/web/shared/middlewares/image/handle-image-sizes.js Show resolved Hide resolved

naz reviewed Dec 3, 2018

View reviewed changes

core/server/web/shared/middlewares/image/handle-image-sizes.js Show resolved Hide resolved

naz suggested changes Dec 3, 2018

View reviewed changes

allouis commented Dec 3, 2018

View reviewed changes

core/test/unit/web/middleware/image/handle-image-sizes_spec.js Show resolved Hide resolved

naz approved these changes Dec 3, 2018

View reviewed changes

kirrg001 reviewed Dec 3, 2018

View reviewed changes

allouis added 7 commits December 13, 2018 18:47

Implemented saveRaw method on local file storage

26d2417

Added initial handleImageSizes middleware

5f3e346

Wired up handleImageSizes middleware

20c78da

Implemented delete for LocalFileStorage

52fd709

Removed delete method from theme Storage class

274fc7e

Deleted sizes directory when theme is activated

2b23205

Ensured that smaller images are not enlarged

80d5fb8

allouis and others added 15 commits December 13, 2018 18:47

Renamed sizes -> size

79f2371

Exited middleware as early as possible

9d3cdce

Called getStorage as late as possible

ddbc78d

Updated image sizes middleware to handle dimension paths

6adbb2d

Revert "Deleted sizes directory when theme is activated"

7e791ed

This reverts commit 9204dfcc73a6a79d597dbf23651817bcbfc59991.

Revert "Removed delete method from theme Storage class"

515e23f

This reverts commit b45fdb405a05faeaf4bd87e977c4ac64ff96b057.

Revert "Implemented delete for LocalFileStorage"

e9d67e7

This reverts commit a587cd6bae45b68a293b2d5cfd9b7705a29e7bfa.

Fixed typo

c2ee4ea

Co-Authored-By: allouis <[email protected]>

Redirected to original image if no image_sizes config

9186c3e

Refactored redirection because rule of three

3a4e3f0

Updated comments

ad6d41a

Added rubbish tests

10884ee

Added @todo comment for handleImageSizes tests

008a387

Added safeResizeImage method to image manipulator

58b3d8a

Used image manipulator lib in image_size middleware

46bddcd

kirrg001 reviewed Dec 13, 2018

View reviewed changes

ErisDS approved these changes Dec 13, 2018

View reviewed changes

naz reviewed Dec 13, 2018

View reviewed changes

allouis merged commit 7099dd4 into TryGhost:master Dec 13, 2018

allouis deleted the create-custom-image-sizes-on-request branch December 13, 2018 13:25

MaluNoPeleke mentioned this pull request Jan 28, 2019

Broken images with 3.0.4 godofredoninja/Mapache#90

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added creation of custom image sizes on request #10184

Added creation of custom image sizes on request #10184

allouis commented Nov 19, 2018 •

edited

Loading

ErisDS commented Nov 26, 2018

JohnONolan commented Nov 26, 2018

ErisDS commented Nov 26, 2018 •

edited

Loading

ErisDS commented Nov 26, 2018

JohnONolan commented Nov 26, 2018 •

edited

Loading

ErisDS commented Nov 26, 2018

JohnONolan commented Nov 28, 2018

allouis commented Dec 3, 2018

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

naz left a comment

allouis commented Dec 3, 2018

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

ErisDS commented Dec 4, 2018

allouis commented Dec 13, 2018

kirrg001 Dec 13, 2018

allouis Dec 13, 2018

kirrg001 Dec 13, 2018

allouis Dec 13, 2018

kirrg001 Dec 13, 2018

allouis Dec 13, 2018

naz Dec 13, 2018

allouis Dec 13, 2018

		@@ -0,0 +1,66 @@
		const path = require('path');
		const sharp = require('sharp');

Added creation of custom image sizes on request #10184

Added creation of custom image sizes on request #10184

Conversation

allouis commented Nov 19, 2018 • edited Loading

Changes

saveRaw method on the LocalFileStorage

delete method on the LocalFileStorage

handleImageSize middleware

Theme service activate method deletes sizes directory

ErisDS commented Nov 26, 2018

JohnONolan commented Nov 26, 2018

ErisDS commented Nov 26, 2018 • edited Loading

ErisDS commented Nov 26, 2018

JohnONolan commented Nov 26, 2018 • edited Loading

ErisDS commented Nov 26, 2018

JohnONolan commented Nov 28, 2018

allouis commented Dec 3, 2018

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

naz left a comment

Choose a reason for hiding this comment

allouis commented Dec 3, 2018

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

ErisDS commented Dec 4, 2018

allouis commented Dec 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allouis commented Nov 19, 2018 •

edited

Loading

`saveRaw` method on the `LocalFileStorage`

`delete` method on the `LocalFileStorage`

`handleImageSize` middleware

Theme service `activate` method deletes `sizes` directory

ErisDS commented Nov 26, 2018 •

edited

Loading

JohnONolan commented Nov 26, 2018 •

edited

Loading