Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added creation of custom image sizes on request #10184

Merged
merged 22 commits into from
Dec 13, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
26d2417
Implemented saveRaw method on local file storage
allouis Nov 19, 2018
5f3e346
Added initial handleImageSizes middleware
allouis Nov 19, 2018
20c78da
Wired up handleImageSizes middleware
allouis Nov 26, 2018
52fd709
Implemented delete for LocalFileStorage
allouis Nov 26, 2018
274fc7e
Removed delete method from theme Storage class
allouis Nov 26, 2018
2b23205
Deleted sizes directory when theme is activated
allouis Nov 26, 2018
80d5fb8
Ensured that smaller images are not enlarged
allouis Nov 26, 2018
79f2371
Renamed sizes -> size
allouis Dec 3, 2018
9d3cdce
Exited middleware as early as possible
allouis Dec 3, 2018
ddbc78d
Called getStorage as late as possible
allouis Dec 3, 2018
6adbb2d
Updated image sizes middleware to handle dimension paths
allouis Dec 3, 2018
7e791ed
Revert "Deleted sizes directory when theme is activated"
allouis Dec 3, 2018
515e23f
Revert "Removed delete method from theme Storage class"
allouis Dec 3, 2018
e9d67e7
Revert "Implemented delete for LocalFileStorage"
allouis Dec 3, 2018
c2ee4ea
Fixed typo
naz Dec 3, 2018
9186c3e
Redirected to original image if no image_sizes config
allouis Dec 3, 2018
3a4e3f0
Refactored redirection because rule of three
allouis Dec 3, 2018
ad6d41a
Updated comments
allouis Dec 3, 2018
10884ee
Added rubbish tests
allouis Dec 3, 2018
008a387
Added @TODO comment for handleImageSizes tests
allouis Dec 3, 2018
58b3d8a
Added safeResizeImage method to image manipulator
allouis Dec 13, 2018
46bddcd
Used image manipulator lib in image_size middleware
allouis Dec 13, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions core/server/adapters/storage/LocalFileStorage.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,31 @@ class LocalFileStore extends StorageBase {
this.storagePath = config.getContentPath('images');
}

/**
* Saves a buffer in the targetPath
* - buffer is an instance of Buffer
* - returns a Promise which returns the full URL to retrieve the data
*/
saveRaw(buffer, targetPath) {
const storagePath = path.join(this.storagePath, targetPath);
const targetDir = path.dirname(storagePath);

return fs.mkdirs(targetDir)
.then(() => {
return fs.writeFile(storagePath, buffer);
})
.then(() => {
// For local file system storage can use relative path so add a slash
const fullUrl = (
urlService.utils.urlJoin('/', urlService.utils.getSubdir(),
urlService.utils.STATIC_IMAGE_URL_PREFIX,
targetPath)
).replace(new RegExp(`\\${path.sep}`, 'g'), '/');

return fullUrl;
});
}

/**
* Saves the image to storage (the file system)
* - image is the express image object
Expand Down
23 changes: 23 additions & 0 deletions core/server/lib/image/manipulator.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,4 +61,27 @@ const process = (options = {}) => {
});
};

const resizeImage = (originalBuffer, {width, height} = {}) => {
const sharp = require('sharp');
return sharp(originalBuffer)
.resize(width, height, {
// CASE: dont make the image bigger than it was
withoutEnlargement: true
})
// CASE: Automatically remove metadata and rotate based on the orientation.
.rotate()
.toBuffer()
.then((resizedBuffer) => {
return resizedBuffer.length < originalBuffer.length ? resizedBuffer : originalBuffer;
});
};

module.exports.process = process;
module.exports.safeResizeImage = (buffer, options) => {
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ☺️

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
};
62 changes: 62 additions & 0 deletions core/server/web/shared/middlewares/image/handle-image-sizes.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
const path = require('path');
const image = require('../../../../lib/image');
const storage = require('../../../../adapters/storage');
const activeTheme = require('../../../../services/themes/active');

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

This comment was marked as abuse.


module.exports = function (req, res, next) {
if (!SIZE_PATH_REGEX.test(req.url)) {
return next();
}

const [sizeImageDir, requestedDimension] = req.url.match(SIZE_PATH_REGEX);
const redirectToOriginal = () => {
const url = req.originalUrl.replace(`/size/${requestedDimension}`, '');
return res.redirect(url);
};

const imageSizes = activeTheme.get().config('image_sizes');
// CASE: no image_sizes config
if (!imageSizes) {
return redirectToOriginal();
}

const imageDimensions = Object.keys(imageSizes).reduce((dimensions, size) => {
const {width, height} = imageSizes[size];
const dimension = (width ? 'w' + width : '') + (height ? 'h' + height : '');
return Object.assign({
[dimension]: imageSizes[size]
}, dimensions);
}, {});

const imageDimensionConfig = imageDimensions[requestedDimension];
// CASE: unknown dimension
if (!imageDimensionConfig || (!imageDimensionConfig.width && !imageDimensionConfig.height)) {

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

return redirectToOriginal();
}

const storageInstance = storage.getStorage();
// CASE: unsupported storage adapter but theme is using custom image_sizes
if (typeof storageInstance.saveRaw !== 'function') {
return redirectToOriginal();
}

storageInstance.exists(req.url).then((exists) => {
if (exists) {
return;
}

const originalImagePath = path.relative(sizeImageDir, req.url);

return storageInstance.read({path: originalImagePath})
.then((originalImageBuffer) => {
return image.manipulator.safeResizeImage(originalImageBuffer, imageDimensionConfig);
})
.then((resizedImageBuffer) => {
return storageInstance.saveRaw(resizedImageBuffer, req.url);
});
}).then(() => {
next();
}).catch(next);
};
3 changes: 3 additions & 0 deletions core/server/web/shared/middlewares/image/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
module.exports = {
get normalize() {
return require('./normalize');
},
get handleImageSizes() {
return require('./handle-image-sizes');
}
};
4 changes: 3 additions & 1 deletion core/server/web/site/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ const themeMiddleware = require('../../services/themes').middleware;
const siteRoutes = require('./routes');
const shared = require('../shared');

const STATIC_IMAGE_URL_PREFIX = `/${urlService.utils.STATIC_IMAGE_URL_PREFIX}`;

let router;

function SiteRouter(req, res, next) {
Expand Down Expand Up @@ -58,7 +60,7 @@ module.exports = function setupSiteApp(options = {}) {
siteApp.use(shared.middlewares.servePublicFile('public/404-ghost.png', 'png', constants.ONE_HOUR_S));

// Serve blog images using the storage adapter
siteApp.use('/' + urlService.utils.STATIC_IMAGE_URL_PREFIX, storage.getStorage().serve());
siteApp.use(STATIC_IMAGE_URL_PREFIX, shared.middlewares.image.handleImageSizes, storage.getStorage().serve());

// @TODO find this a better home
// We do this here, at the top level, because helpers require so much stuff.
Expand Down
44 changes: 44 additions & 0 deletions core/test/unit/web/middleware/image/handle-image-sizes_spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
const should = require('should');
const handleImageSizes = require('../../../../../server/web/shared/middlewares/image/handle-image-sizes.js');

// @TODO make these tests lovely and non specific to implementation
describe('handleImageSizes middleware', function () {
it('calls next immediately if the url does not match /size/something/', function (done) {
const fakeReq = {
url: '/size/something'
};
// CASE: second thing middleware does is try to match to a regex
fakeReq.url.match = function () {
throw new Error('Should have exited immediately');
};
handleImageSizes(fakeReq, {}, function next() {
done();
});
});

it('calls next immediately if the url does not match /size/something/', function (done) {
const fakeReq = {
url: '/url/whatever/'
};
// CASE: second thing middleware does is try to match to a regex
fakeReq.url.match = function () {
throw new Error('Should have exited immediately');
};
handleImageSizes(fakeReq, {}, function next() {
done();
});
});

it('calls next immediately if the url does not match /size/something/', function (done) {
const fakeReq = {
url: '/size//'
};
// CASE: second thing middleware does is try to match to a regex
fakeReq.url.match = function () {
throw new Error('Should have exited immediately');
};
handleImageSizes(fakeReq, {}, function next() {
done();
});
});
});