fix: #7444 Re-factor handling of og:image tags (#7463)

* fix: display proper site logo or og-image with sizes in head

* fix: refactor og:image logic, #7444

- Updated logic to set additional og:image tags based on more
  factors
- logo.png fallback

* feat: save image sizes on post upload, re: #7444

* fix: awaiting addTags in topic controller

* fix: pass strings to meta tags object

* fix: sending absolute image url to meta tag

* fix: removed unneeded async and requiring sync db

* feat: upgrade to calculate image sizes for all post uploads tracked

* fix: tests

src/controllers/admin/uploads.js

@@ -290,6 +290,20 @@ function uploadImage(filename, folder, uploadedFile, req, res, next) {
 						height: 50,
 					}),
 					async.apply(meta.configs.set, 'brand:emailLogo', path.join(nconf.get('upload_url'), 'system/site-logo-x50.png')),
+					function (next) {
+						image.size(imageData.path, function (err, size) {
+							if (err) {
+								return next(err);
+							}
+
+							meta.configs.setMultiple({
+								'brand:logo:width': size.width,
+								'brand:logo:height': size.height,
+							}, function (err) {
+								next(err);
+							});
+						});
+					},
 				], function (err) {
 					next(err, imageData);
 				});

src/controllers/topics.js

@@ -8,7 +8,7 @@ var winston = require('winston');
 var user = require('../user');
 var meta = require('../meta');
 var topics = require('../topics');
-var posts = require('../posts');
+var posts = require('../posts').async;
 var privileges = require('../privileges');
 var plugins = require('../plugins');
 var helpers = require('./helpers');
@@ -137,6 +137,10 @@ topicsController.get = function getTopic(req, res, callback) {
 		function (data, next) {
 			buildBreadcrumbs(data.topicData, next);
 		},
+		async function (topicData) {
+			await addTags(topicData, req, res);
+			return topicData;
+		},
 		function (topicData) {
 			topicData.privileges = userPrivileges;
 			topicData.topicStaleDays = meta.config.topicStaleDays;
@@ -153,8 +157,6 @@ topicsController.get = function getTopic(req, res, callback) {
 				topicData.rssFeedUrl += '?uid=' + req.uid + '&token=' + rssToken;
 			}
 
-			addTags(topicData, req, res);
-
 			topicData.postIndex = req.params.post_index;
 			topicData.pagination = pagination.create(currentPage, pageCount, req.query);
 			topicData.pagination.rel.forEach(function (rel) {
@@ -211,7 +213,7 @@ function buildBreadcrumbs(topicData, callback) {
 	], callback);
 }
 
-function addTags(topicData, req, res) {
+async function addTags(topicData, req, res) {
 	var postAtIndex = topicData.posts.find(function (postData) {
 		return parseInt(postData.index, 10) === parseInt(Math.max(0, req.params.post_index - 1), 10);
 	});
@@ -261,7 +263,7 @@ function addTags(topicData, req, res) {
 		},
 	];
 
-	addOGImageTags(res, topicData, postAtIndex);
+	await addOGImageTags(res, topicData, postAtIndex);
 
 	res.locals.linkTags = [
 		{
@@ -286,58 +288,64 @@ function addTags(topicData, req, res) {
 	}
 }
 
-function addOGImageTags(res, topicData, postAtIndex) {
+async function addOGImageTags(res, topicData, postAtIndex) {
-	var ogImageUrl = '';
+	const images = [];
-	if (topicData.thumb) {
-		ogImageUrl = topicData.thumb;
-	} else if (topicData.category.backgroundImage && (!postAtIndex || !postAtIndex.index)) {
-		ogImageUrl = topicData.category.backgroundImage;
-	} else if (postAtIndex && postAtIndex.user && postAtIndex.user.picture) {
-		ogImageUrl = postAtIndex.user.picture;
-	} else if (meta.config['og:image']) {
-		ogImageUrl = meta.config['og:image'];
-	} else if (meta.config['brand:logo']) {
-		ogImageUrl = meta.config['brand:logo'];
-	} else {
-		ogImageUrl = '/logo.png';
-	}
 
-	addOGImageTag(res, ogImageUrl);
+	async.series([
-	addOGImageTagsForPosts(res, topicData.posts);
+		async function () {
-}
+			const uploads = await posts.uploads.listWithSizes(postAtIndex.pid);
-
+			uploads.forEach((upload) => {
-function addOGImageTagsForPosts(res, posts) {
+				upload.name = nconf.get('url') + nconf.get('upload_url') + '/files/' + upload.name;
-	posts.forEach(function (postData) {
+				images.push(upload);
-		var regex = /src\s*=\s*"(.+?)"/g;
+			});
-		var match = regex.exec(postData.content);
+		},
-		while (match !== null) {
+		function (next) {
-			var image = match[1];
+			if (topicData.thumb) {
-
+				images.push(topicData.thumb);
-			if (image.startsWith(nconf.get('url') + '/plugins')) {
+			}
-				return;
+			if (topicData.category.backgroundImage && (!postAtIndex || !postAtIndex.index)) {
+				images.push(topicData.category.backgroundImage);
+			}
+			if (postAtIndex && postAtIndex.user && postAtIndex.user.picture) {
+				images.push(postAtIndex.user.picture);
 			}
 
-			addOGImageTag(res, image);
+			process.nextTick(next);
-
+		},
-			match = regex.exec(postData.content);
+	], function () {
-		}
+		images.forEach(path => addOGImageTag(res, path));
 	});
 }
 
-function addOGImageTag(res, imageUrl) {
+function addOGImageTag(res, image) {
-	if (typeof imageUrl === 'string' && !imageUrl.startsWith('http')) {
+	let imageUrl;
-		imageUrl = nconf.get('url') + imageUrl.replace(new RegExp('^' + nconf.get('relative_path')), '');
+	if (typeof image === 'string' && !image.startsWith('http')) {
+		imageUrl = nconf.get('url') + image.replace(new RegExp('^' + nconf.get('relative_path')), '');
+	} else if (typeof image === 'object') {
+		imageUrl = image.name;
+	} else {
+		imageUrl = image;
 	}
+
 	res.locals.metaTags.push({
 		property: 'og:image',
 		content: imageUrl,
 		noEscape: true,
-	});
+	}, {
-	res.locals.metaTags.push({
 		property: 'og:image:url',
 		content: imageUrl,
 		noEscape: true,
 	});
+
+	if (typeof image === 'object' && image.width && image.height) {
+		res.locals.metaTags.push({
+			property: 'og:image:width',
+			content: String(image.width),
+		}, {
+			property: 'og:image:height',
+			content: String(image.height),
+		});
+	}
 }
 
 topicsController.teaser = function (req, res, next) {

src/meta/tags.js

@@ -104,7 +104,7 @@ Tags.parse = function (req, data, meta, link, callback) {
 			}
 			plugins.fireHook('filter:meta.getLinkTags', { req: req, data: data, links: defaultLinks }, next);
 		},
-	}, function (err, results) {
+	}, async function (err, results) {
 		if (err) {
 			return callback(err);
 		}
@@ -122,24 +122,14 @@ Tags.parse = function (req, data, meta, link, callback) {
 			return tag;
 		});
 
-		addIfNotExists(meta, 'property', 'og:title', Meta.config.title || 'NodeBB');
+		addSiteOGImage(meta);
 
+		addIfNotExists(meta, 'property', 'og:title', Meta.config.title || 'NodeBB');
 		var ogUrl = nconf.get('url') + (req.originalUrl !== '/' ? stripRelativePath(req.originalUrl) : '');
 		addIfNotExists(meta, 'property', 'og:url', ogUrl);
-
 		addIfNotExists(meta, 'name', 'description', Meta.config.description);
 		addIfNotExists(meta, 'property', 'og:description', Meta.config.description);
 
-		var ogImage = stripRelativePath(Meta.config['og:image'] || Meta.config['brand:logo'] || '');
-		if (ogImage && !ogImage.startsWith('http')) {
-			ogImage = nconf.get('url') + ogImage;
-		}
-		addIfNotExists(meta, 'property', 'og:image', ogImage);
-		if (ogImage) {
-			addIfNotExists(meta, 'property', 'og:image:width', Meta.config['og:image:width'] || 200);
-			addIfNotExists(meta, 'property', 'og:image:height', Meta.config['og:image:height'] || 200);
-		}
-
 		link = results.links.links.concat(link || []);
 
 		callback(null, {
@@ -173,3 +163,50 @@ function stripRelativePath(url) {
 
 	return url;
 }
+
+function addSiteOGImage(meta) {
+	const key = Meta.config['og:image'] ? 'og:image' : 'brand:logo';
+	var ogImage = stripRelativePath(Meta.config[key] || '');
+	if (ogImage && !ogImage.startsWith('http')) {
+		ogImage = nconf.get('url') + ogImage;
+	}
+
+	if (ogImage) {
+		meta.push({
+			property: 'og:image',
+			content: ogImage,
+			noEscape: true,
+		}, {
+			property: 'og:image:url',
+			content: ogImage,
+			noEscape: true,
+		});
+
+		if (Meta.config[key + ':width'] && Meta.config[key + ':height']) {
+			meta.push({
+				property: 'og:image:width',
+				content: String(Meta.config[key + ':width']),
+			}, {
+				property: 'og:image:height',
+				content: String(Meta.config[key + ':height']),
+			});
+		}
+	} else {
+		// Push fallback logo
+		meta.push({
+			property: 'og:image',
+			content: nconf.get('url') + '/assets/logo.png',
+			noEscape: true,
+		}, {
+			property: 'og:image:url',
+			content: nconf.get('url') + '/assets/logo.png',
+			noEscape: true,
+		}, {
+			property: 'og:image:width',
+			content: '128',
+		}, {
+			property: 'og:image:height',
+			content: '128',
+		});
+	}
+}

src/posts/uploads.js

@@ -1,17 +1,21 @@
 'use strict';
 
 var async = require('async');
+var nconf = require('nconf');
 var crypto = require('crypto');
 var fs = require('fs');
 var path = require('path');
+var util = require('util');
+var winston = require('winston');
 
 var db = require('../database');
+const image = require('../image');
 
 module.exports = function (Posts) {
 	Posts.uploads = {};
 
 	const md5 = filename => crypto.createHash('md5').update(filename).digest('hex');
-	const pathPrefix = path.join(__dirname, '../../public/uploads/files');
+	const pathPrefix = path.join(nconf.get('upload_path'), 'files');
 	const searchRegex = /\/assets\/uploads\/files\/([^\s")]+\.?[\w]*)/g;
 
 	Posts.uploads.sync = function (pid, callback) {
@@ -52,6 +56,16 @@ module.exports = function (Posts) {
 		db.getSortedSetRange('post:' + pid + ':uploads', 0, -1, callback);
 	};
 
+	Posts.uploads.listWithSizes = async function (pid) {
+		const paths = await Posts.async.uploads.list(pid);
+		const sizes = await db.async.getObjects(paths.map(path => 'upload:' + md5(path)));
+
+		return sizes.map((sizeObj, idx) => {
+			sizeObj.name = paths[idx];
+			return sizeObj;
+		});
+	};
+
 	Posts.uploads.isOrphan = function (filePath, callback) {
 		// Returns bool indicating whether a file is still CURRENTLY included in any posts
 		db.sortedSetCard('upload:' + md5(filePath) + ':pids', function (err, length) {
@@ -91,6 +105,9 @@ module.exports = function (Posts) {
 			let methods = [async.apply(db.sortedSetAdd.bind(db), 'post:' + pid + ':uploads', scores, filePaths)];
 
 			methods = methods.concat(filePaths.map(path => async.apply(db.sortedSetAdd.bind(db), 'upload:' + md5(path) + ':pids', now, pid)));
+			methods = methods.concat(async function () {
+				await Posts.uploads.saveSize(filePaths);
+			});
 			async.parallel(methods, function (err) {
 				// Strictly return only err
 				callback(err);
@@ -112,4 +129,35 @@ module.exports = function (Posts) {
 			callback(err);
 		});
 	};
+
+	Posts.uploads.saveSize = async (filePaths) => {
+		const getSize = util.promisify(image.size);
+		const sizes = await Promise.all(filePaths.map(async function (fileName) {
+			try {
+				return await getSize(path.join(pathPrefix, fileName));
+			} catch (e) {
+				// Error returned by getSize, do not save size in database
+				return null;
+			}
+		}));
+
+		const methods = filePaths.map((filePath, idx) => {
+			if (!sizes[idx]) {
+				return null;
+			}
+
+			winston.verbose('[posts/uploads/' + filePath + '] Saving size');
+			return async.apply(db.setObject, 'upload:' + md5(filePath), {
+				width: sizes[idx].width,
+				height: sizes[idx].height,
+			});
+		}).filter(Boolean);
+		async.parallel(methods, function (err) {
+			if (err) {
+				winston.error('[posts/uploads] Error while saving post upload sizes: ', err.message);
+			} else {
+				winston.verbose('[posts/uploads] Finished saving post upload sizes.');
+			}
+		});
+	};
 };

src/upgrades/1.12.1/post_upload_sizes.js

@@ -0,0 +1,24 @@
+'use strict';
+
+var async = require('async');
+
+const batch = require('../../batch');
+const posts = require('../../posts').async;
+
+module.exports = {
+	name: 'Calculate image sizes of all uploaded images',
+	timestamp: Date.UTC(2019, 2, 16),
+	method: function (callback) {
+		const progress = this.progress;
+
+		batch.processSortedSet('posts:pid', function (postData, next) {
+			async.eachSeries(postData, async function (pid) {
+				const uploads = await posts.uploads.list(pid);
+				await posts.uploads.saveSize(uploads);
+				progress.incr();
+			}, next);
+		}, {
+			progress: progress,
+		}, callback);
+	},
+};

src/upgrades/TEMPLATE

@@ -10,7 +10,7 @@ module.exports = {
 	// the underscores are there so you can double click to select the whole thing
 	name: 'User_friendly_upgrade_script_name',
 	// remember, month is zero-indexed (so January is 0, December is 11)
-	timestamp: Date.UTC(2017, 0, 1),
+	timestamp: Date.UTC(2019, 0, 1),
 	method: function (callback) {
 		// Do stuff here...
 	},

test/posts.js

@@ -936,7 +936,7 @@ describe('Post\'s', function () {
 		before(function (done) {
 			// Create stub files for testing
 			['abracadabra.png', 'shazam.jpg', 'whoa.gif', 'amazeballs.jpg', 'wut.txt', 'test.bmp']
-				.forEach(filename => fs.closeSync(fs.openSync(path.join(__dirname, '../public/uploads/files', filename), 'w')));
+				.forEach(filename => fs.closeSync(fs.openSync(path.join(nconf.get('upload_path'), 'files', filename), 'w')));
 
 			topics.post({
 				uid: 1,
@@ -957,7 +957,7 @@ describe('Post\'s', function () {
 
 					db.sortedSetCard('post:' + pid + ':uploads', function (err, length) {
 						assert.ifError(err);
-						assert.strictEqual(2, length);
+						assert.strictEqual(length, 2);
 						done();
 					});
 				});