From 6a97873f983af623e22bd006877a446f5949de01 Mon Sep 17 00:00:00 2001 From: kirrg001 Date: Tue, 13 Sep 2016 22:24:57 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=A8=20=20=F0=9F=94=A6=20=20refactor=20?= =?UTF-8?q?content=20paths=20(images,=20apps,=20themes,=20storage,=20sched?= =?UTF-8?q?uling)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refs #6982 - create config util fn: getContentPath - we can later let the user change the folder names in contentPath - get rid of custom/default storage paths [ci skip] --- core/server/api/themes.js | 10 ++++---- core/server/apps/loader.js | 2 +- core/server/config/defaults.json | 9 +------ core/server/config/index.js | 1 + core/server/config/overrides.json | 9 ++++--- core/server/config/utils.js | 20 ++++++++++++++++ core/server/data/importer/handlers/image.js | 4 ++-- core/server/data/validation/index.js | 2 +- core/server/index.js | 9 +++---- core/server/middleware/static-theme.js | 2 +- core/server/middleware/theme-handler.js | 6 ++--- core/server/scheduling/utils.js | 24 ++++++++++++++++--- core/server/storage/index.js | 8 +++---- core/server/storage/local-file-store.js | 17 ++++++++----- .../test/functional/routes/api/themes_spec.js | 10 ++++---- core/test/unit/config_spec.js | 22 ++++------------- core/test/unit/scheduling/utils_spec.js | 3 ++- core/test/unit/storage/index_spec.js | 10 ++++---- .../unit/storage/local-file-store_spec.js | 14 +++++------ 19 files changed, 103 insertions(+), 79 deletions(-) diff --git a/core/server/api/themes.js b/core/server/api/themes.js index 5fcef52c95..c587c2cb9c 100644 --- a/core/server/api/themes.js +++ b/core/server/api/themes.js @@ -21,7 +21,7 @@ var Promise = require('bluebird'), */ themes = { loadThemes: function () { - return utils.readThemes(config.get('paths').themePath) + return utils.readThemes(config.getContentPath('themes')) .then(function (result) { config.set('paths:availableThemes', result); }); @@ -63,12 +63,12 @@ themes = { ); }) .then(function () { - return storageAdapter.exists(config.get('paths').themePath + '/' + zip.shortName); + return storageAdapter.exists(config.getContentPath('themes') + '/' + zip.shortName); }) .then(function (themeExists) { // delete existing theme if (themeExists) { - return storageAdapter.delete(zip.shortName, config.get('paths').themePath); + return storageAdapter.delete(zip.shortName, config.getContentPath('themes')); } }) .then(function () { @@ -77,7 +77,7 @@ themes = { return storageAdapter.save({ name: zip.shortName, path: theme.path - }, config.get('paths').themePath); + }, config.getContentPath('themes')); }) .then(function () { // force reload of availableThemes @@ -155,7 +155,7 @@ themes = { } events.emit('theme.deleted', name); - return storageAdapter.delete(name, config.get('paths').themePath); + return storageAdapter.delete(name, config.getContentPath('themes')); }) .then(function () { return themes.loadThemes(); diff --git a/core/server/apps/loader.js b/core/server/apps/loader.js index ef526bbeb5..eafb446d86 100644 --- a/core/server/apps/loader.js +++ b/core/server/apps/loader.js @@ -20,7 +20,7 @@ function getAppAbsolutePath(name) { return path.join(config.get('paths').internalAppPath, name); } - return path.join(config.get('paths').appPath, name); + return path.join(config.getContentPath('apps'), name); } // Get a relative path to the given apps root, defaults diff --git a/core/server/config/defaults.json b/core/server/config/defaults.json index 801b375474..208657d5ae 100644 --- a/core/server/config/defaults.json +++ b/core/server/config/defaults.json @@ -15,14 +15,7 @@ }, "privacy": false, "paths": { - "contentPath": "content/", - "themePath": "content/themes/", - "imagesPath": "content/images/", - "appPath": "content/apps/", - "storagePath": { - "custom": "content/storage/" - }, - "schedulingPath": "core/server/scheduling/" + "contentPath": "content/" }, "storage": { "active": { diff --git a/core/server/config/index.js b/core/server/config/index.js index c17e47207c..f5248eee40 100644 --- a/core/server/config/index.js +++ b/core/server/config/index.js @@ -37,3 +37,4 @@ nconf.set('ghostVersion', packageInfo.version); module.exports = nconf; module.exports.isPrivacyDisabled = localUtils.isPrivacyDisabled.bind(nconf); +module.exports.getContentPath = localUtils.getContentPath.bind(nconf); diff --git a/core/server/config/overrides.json b/core/server/config/overrides.json index b890a22377..ea30318c38 100644 --- a/core/server/config/overrides.json +++ b/core/server/config/overrides.json @@ -2,14 +2,13 @@ "paths": { "appRoot": ".", "corePath": "core/", - "internalAppPath": "core/server/apps/", - "storagePath": { - "default": "core/server/storage/" - }, "clientAssets": "core/built/assets", "imagesRelPath": "content/images", "helperTemplates": "core/server/helpers/tpl/", - "adminViews": "core/server/views/" + "adminViews": "core/server/views/", + "internalAppPath": "core/server/apps/", + "internalStoragePath": "core/server/storage/", + "internalSchedulingPath": "core/server/scheduling/" }, "internalApps": [ "private-blogging", diff --git a/core/server/config/utils.js b/core/server/config/utils.js index 6895b5f56b..0d332ca051 100644 --- a/core/server/config/utils.js +++ b/core/server/config/utils.js @@ -35,3 +35,23 @@ exports.makePathsAbsolute = function makePathsAbsolute(paths, parent) { } }); }; + +/** + * we can later support setting folder names via custom config values + */ +exports.getContentPath = function getContentPath(type) { + switch (type) { + case 'storage': + return path.join(this.get('paths:contentPath'), 'storage/'); + case 'images': + return path.join(this.get('paths:contentPath'), 'images/'); + case 'apps': + return path.join(this.get('paths:contentPath'), 'apps/'); + case 'themes': + return path.join(this.get('paths:contentPath'), 'themes/'); + case 'scheduling': + return path.join(this.get('paths:contentPath'), 'scheduling/'); + default: + throw new Error('getContentPath was called with: ' + type); + } +}; diff --git a/core/server/data/importer/handlers/image.js b/core/server/data/importer/handlers/image.js index 2e48f153b0..781d85539d 100644 --- a/core/server/data/importer/handlers/image.js +++ b/core/server/data/importer/handlers/image.js @@ -31,14 +31,14 @@ ImageHandler = { file.originalPath = noBaseDir; file.name = noGhostDirs; - file.targetDir = path.join(config.get('paths').imagesPath, path.dirname(noGhostDirs)); + file.targetDir = path.join(config.getContentPath('images'), path.dirname(noGhostDirs)); return file; }); return Promise.map(files, function (image) { return store.getUniqueFileName(store, image, image.targetDir).then(function (targetFilename) { image.newPath = (utils.url.getSubdir() + '/' + - config.get('paths').imagesRelPath + '/' + path.relative(config.get('paths').imagesPath, targetFilename)) + config.get('paths').imagesRelPath + '/' + path.relative(config.getContentPath('images'), targetFilename)) .replace(new RegExp('\\' + path.sep, 'g'), '/'); return image; diff --git a/core/server/data/validation/index.js b/core/server/data/validation/index.js index d19243ff32..4f4c4452ba 100644 --- a/core/server/data/validation/index.js +++ b/core/server/data/validation/index.js @@ -141,7 +141,7 @@ validateActiveTheme = function validateActiveTheme(themeName) { // A Promise that will resolve to an object with a property for each installed theme. // This is necessary because certain configuration data is only available while Ghost // is running and at times the validations are used when it's not (e.g. tests) - availableThemes = readThemes(config.get('paths').themePath); + availableThemes = readThemes(config.getContentPath('themes')); } return availableThemes.then(function then(themes) { diff --git a/core/server/index.js b/core/server/index.js index ffc458bd35..38015c6b1f 100644 --- a/core/server/index.js +++ b/core/server/index.js @@ -72,7 +72,7 @@ function init(options) { // Initialize Internationalization i18n.init(); - return readDirectory(config.get('paths').appPath).then(function loadThemes(result) { + return readDirectory(config.getContentPath('apps')).then(function loadThemes(result) { config.set('paths:availableApps', result); return api.themes.loadThemes(); }).then(function () { @@ -143,7 +143,7 @@ function init(options) { middleware(parentApp); // Log all theme errors and warnings - validateThemes(config.get('paths').themePath) + validateThemes(config.getContentPath('themes')) .catch(function (result) { // TODO: change `result` to something better result.errors.forEach(function (err) { @@ -163,8 +163,9 @@ function init(options) { // scheduling module can create x schedulers with different adapters return scheduling.init({ active: config.get('scheduling').active, - path: config.get('paths').schedulingPath, - apiUrl: utils.url.apiUrl() + apiUrl: utils.url.apiUrl(), + internalPath: config.get('paths').internalSchedulingPath, + contentPath: config.getContentPath('scheduling') }); }).then(function () { return ghostServer; diff --git a/core/server/middleware/static-theme.js b/core/server/middleware/static-theme.js index 4be5cb58c7..e79d33d381 100644 --- a/core/server/middleware/static-theme.js +++ b/core/server/middleware/static-theme.js @@ -21,7 +21,7 @@ function forwardToExpressStatic(req, res, next) { next(); } else { express.static( - path.join(config.get('paths').themePath, req.app.get('activeTheme')), + path.join(config.getContentPath('themes'), req.app.get('activeTheme')), {maxAge: utils.ONE_YEAR_MS} )(req, res, next); } diff --git a/core/server/middleware/theme-handler.js b/core/server/middleware/theme-handler.js index 7af82fe33b..5e97259141 100644 --- a/core/server/middleware/theme-handler.js +++ b/core/server/middleware/theme-handler.js @@ -41,8 +41,8 @@ themeHandler = { hbs.updateTemplateOptions({data: {blog: themeData, labs: labsData}}); - if (config.get('paths').themePath && blogApp.get('activeTheme')) { - blogApp.set('views', path.join(config.get('paths').themePath, blogApp.get('activeTheme'))); + if (config.getContentPath('themes') && blogApp.get('activeTheme')) { + blogApp.set('views', path.join(config.getContentPath('themes'), blogApp.get('activeTheme'))); } // Pass 'secure' flag to the view engine @@ -56,7 +56,7 @@ themeHandler = { // Helper for updateActiveTheme activateTheme: function activateTheme(blogApp, activeTheme) { var hbsOptions, - themePartials = path.join(config.get('paths').themePath, activeTheme, 'partials'); + themePartials = path.join(config.getContentPath('themes'), activeTheme, 'partials'); // clear the view cache blogApp.cache = {}; diff --git a/core/server/scheduling/utils.js b/core/server/scheduling/utils.js index 91b08d3bbd..7e2bff57a8 100644 --- a/core/server/scheduling/utils.js +++ b/core/server/scheduling/utils.js @@ -8,7 +8,8 @@ exports.createAdapter = function (options) { var adapter = null, activeAdapter = options.active, - path = options.path; + internalPath = options.internalPath, + contentPath = options.contentPath; if (!activeAdapter) { return Promise.reject(new errors.IncorrectUsage('Please provide an active adapter.')); @@ -29,10 +30,27 @@ exports.createAdapter = function (options) { * CASE: active adapter is located in specific ghost path */ try { - adapter = adapter || new (require(path + activeAdapter))(options); + adapter = adapter || new (require(contentPath + activeAdapter))(options); } catch (err) { + // CASE: only throw error if module does exist + if (err.code !== 'MODULE_NOT_FOUND') { + return Promise.reject(new errors.IncorrectUsage(err.message)); + } + // CASE: if module not found it can be an error within the adapter (cannot find bluebird for example) + else if (err.code === 'MODULE_NOT_FOUND' && err.message.indexOf(contentPath + activeAdapter) === -1) { + return Promise.reject(new errors.IncorrectUsage(err.message)); + } + } + + /** + * CASE: active adapter is located in internal ghost path + */ + try { + adapter = adapter || new (require(internalPath + activeAdapter))(options); + } catch (err) { + // CASE: only throw error if module does exist if (err.code === 'MODULE_NOT_FOUND') { - return Promise.reject(new errors.IncorrectUsage('MODULE_NOT_FOUND', activeAdapter)); + return Promise.reject(new errors.IncorrectUsage('We cannot find your adapter in: ' + contentPath + ' or: ' + internalPath)); } return Promise.reject(new errors.IncorrectUsage(err.message)); diff --git a/core/server/storage/index.js b/core/server/storage/index.js index 2fa85d9177..cb0743958c 100644 --- a/core/server/storage/index.js +++ b/core/server/storage/index.js @@ -33,24 +33,24 @@ function getStorage(type) { // CASE: load adapter from custom path (.../content/storage) try { - storage[storageChoice] = require(config.get('paths').storagePath.custom + storageChoice); + storage[storageChoice] = require(config.getContentPath('storage') + storageChoice); } catch (err) { // CASE: only throw error if module does exist if (err.code !== 'MODULE_NOT_FOUND') { throw new errors.IncorrectUsage(err.message); } // CASE: if module not found it can be an error within the adapter (cannot find bluebird for example) - else if (err.code === 'MODULE_NOT_FOUND' && err.message.indexOf(config.get('paths').storagePath.custom + storageChoice) === -1) { + else if (err.code === 'MODULE_NOT_FOUND' && err.message.indexOf(config.getContentPath('storage') + storageChoice) === -1) { throw new errors.IncorrectUsage(err.message); } } // CASE: either storage[storageChoice] is already set or why check for in the default storage path try { - storage[storageChoice] = storage[storageChoice] || require(config.get('paths').storagePath.default + storageChoice); + storage[storageChoice] = storage[storageChoice] || require(config.get('paths').internalStoragePath + storageChoice); } catch (err) { if (err.code === 'MODULE_NOT_FOUND') { - throw new errors.IncorrectUsage('We cannot find your adpter in: ' + config.get('paths').storagePath.custom + ' or: ' + config.get('paths').storagePath.default); + throw new errors.IncorrectUsage('We cannot find your adapter in: ' + config.getContentPath('storage') + ' or: ' + config.get('paths').internalStoragePath); } else { throw new errors.IncorrectUsage(err.message); } diff --git a/core/server/storage/local-file-store.js b/core/server/storage/local-file-store.js index 13146b554d..44def9423c 100644 --- a/core/server/storage/local-file-store.js +++ b/core/server/storage/local-file-store.js @@ -24,7 +24,7 @@ util.inherits(LocalFileStore, BaseStore); // - image is the express image object // - returns a promise which ultimately returns the full url to the uploaded image LocalFileStore.prototype.save = function (image, targetDir) { - targetDir = targetDir || this.getTargetDir(config.get('paths').imagesPath); + targetDir = targetDir || this.getTargetDir(config.getContentPath('images')); var targetFilename; return this.getUniqueFileName(this, image, targetDir).then(function (filename) { @@ -35,8 +35,13 @@ LocalFileStore.prototype.save = function (image, targetDir) { }).then(function () { // The src for the image must be in URI format, not a file system path, which in Windows uses \ // For local file system storage can use relative path so add a slash - var fullUrl = (utils.url.getSubdir() + '/' + config.get('paths').imagesRelPath + '/' + - path.relative(config.get('paths').imagesPath, targetFilename)).replace(new RegExp('\\' + path.sep, 'g'), '/'); + var fullUrl = ( + utils.url.getSubdir() + '/' + + config.get('paths').imagesRelPath + + '/' + + path.relative(config.getContentPath('images'), targetFilename) + ).replace(new RegExp('\\' + path.sep, 'g'), '/'); + return fullUrl; }).catch(function (e) { errors.logError(e); @@ -63,7 +68,7 @@ LocalFileStore.prototype.serve = function (options) { if (options.isTheme) { return function downloadTheme(req, res, next) { var themeName = options.name, - themePath = path.join(config.get('paths').themePath, themeName), + themePath = path.join(config.getContentPath('themes'), themeName), zipName = themeName + '.zip', // store this in a unique temporary folder zipBasePath = path.join(os.tmpdir(), utils.uid(10)), @@ -95,12 +100,12 @@ LocalFileStore.prototype.serve = function (options) { // CASE: serve images // For some reason send divides the max age number by 1000 // Fallthrough: false ensures that if an image isn't found, it automatically 404s - return serveStatic(config.get('paths').imagesPath, {maxAge: utils.ONE_YEAR_MS, fallthrough: false}); + return serveStatic(config.getContentPath('images'), {maxAge: utils.ONE_YEAR_MS, fallthrough: false}); } }; LocalFileStore.prototype.delete = function (fileName, targetDir) { - targetDir = targetDir || this.getTargetDir(config.get('paths').imagesPath); + targetDir = targetDir || this.getTargetDir(config.getContentPath('images')); var pathToDelete = path.join(targetDir, fileName); return remove(pathToDelete); diff --git a/core/test/functional/routes/api/themes_spec.js b/core/test/functional/routes/api/themes_spec.js index 77d7479d68..a3157b3fc6 100644 --- a/core/test/functional/routes/api/themes_spec.js +++ b/core/test/functional/routes/api/themes_spec.js @@ -42,8 +42,8 @@ describe('Themes API', function () { after(function (done) { // clean successful uploaded themes - fs.removeSync(config.get('paths').themePath + '/valid'); - fs.removeSync(config.get('paths').themePath + '/casper.zip'); + fs.removeSync(config.getContentPath('themes') + '/valid'); + fs.removeSync(config.getContentPath('themes') + '/casper.zip'); // gscan creates /test/tmp in test mode fs.removeSync(config.get('paths').appRoot + '/test'); @@ -92,7 +92,7 @@ describe('Themes API', function () { } // ensure contains two files (zip and extracted theme) - fs.readdirSync(config.get('paths').themePath).join().match(/valid/gi).length.should.eql(1); + fs.readdirSync(config.getContentPath('themes')).join().match(/valid/gi).length.should.eql(1); done(); }); }); @@ -139,8 +139,8 @@ describe('Themes API', function () { return done(err); } - fs.existsSync(config.get('paths').themePath + '/valid').should.eql(false); - fs.existsSync(config.get('paths').themePath + '/valid.zip').should.eql(false); + fs.existsSync(config.getContentPath('themes') + '/valid').should.eql(false); + fs.existsSync(config.getContentPath('themes') + '/valid.zip').should.eql(false); done(); }); }); diff --git a/core/test/unit/config_spec.js b/core/test/unit/config_spec.js index fb5c2b815d..ac5d6f42c9 100644 --- a/core/test/unit/config_spec.js +++ b/core/test/unit/config_spec.js @@ -94,13 +94,11 @@ describe('Config', function () { // This will fail if there are any extra keys pathConfig.should.have.keys( 'appRoot', - 'storagePath', + 'internalStoragePath', + 'internalSchedulingPath', 'contentPath', 'corePath', - 'themePath', - 'appPath', 'internalAppPath', - 'imagesPath', 'imagesRelPath', 'adminViews', 'helperTemplates', @@ -118,25 +116,15 @@ describe('Config', function () { it('should allow specific properties to be user defined', function () { var contentPath = path.join(config.get('paths').appRoot, 'otherContent', '/'); - configUtils.set({ - paths: { - contentPath: contentPath - } - }); - + configUtils.set('paths:contentPath', contentPath); config.get('paths').should.have.property('contentPath', contentPath); - config.get('paths').should.have.property('themePath', contentPath + 'themes'); - config.get('paths').should.have.property('appPath', contentPath + 'apps'); - config.get('paths').should.have.property('imagesPath', contentPath + 'images'); + config.getContentPath('images').should.eql(contentPath + 'images/'); }); }); describe('Storage', function () { it('should default to local-file-store', function () { - config.get('paths').should.have.property('storagePath', { - default: path.join(config.get('paths').corePath, '/server/storage/'), - custom: path.join(config.get('paths').contentPath, 'storage/') - }); + config.get('paths').should.have.property('internalStoragePath', path.join(config.get('paths').corePath, '/server/storage/')); config.get('storage').should.have.property('active', { images: 'local-file-store', diff --git a/core/test/unit/scheduling/utils_spec.js b/core/test/unit/scheduling/utils_spec.js index 9b4c88e94b..5220d98de2 100644 --- a/core/test/unit/scheduling/utils_spec.js +++ b/core/test/unit/scheduling/utils_spec.js @@ -29,9 +29,10 @@ describe('Scheduling: utils', function () { 'module.exports = AnotherAdapter'; fs.writeFileSync(__dirname + '/another-scheduler.js', jsFile); + schedulingUtils.createAdapter({ active: 'another-scheduler', - path: __dirname + '/' + contentPath: __dirname + '/' }).then(function (adapter) { should.exist(adapter); done(); diff --git a/core/test/unit/storage/index_spec.js b/core/test/unit/storage/index_spec.js index a27a5cecdf..274f0b2f2e 100644 --- a/core/test/unit/storage/index_spec.js +++ b/core/test/unit/storage/index_spec.js @@ -12,8 +12,8 @@ describe('storage: index_spec', function () { var scope = {adapter: null}; before(function () { - if (!fs.existsSync(configUtils.config.get('paths').storagePath.custom)) { - fs.mkdirSync(configUtils.config.get('paths').storagePath.custom); + if (!fs.existsSync(configUtils.config.getContentPath('storage'))) { + fs.mkdirSync(configUtils.config.getContentPath('storage')); } }); @@ -48,7 +48,7 @@ describe('storage: index_spec', function () { describe('custom ghost storage config', function () { it('images storage adapter is custom, themes is default', function () { - scope.adapter = configUtils.config.get('paths').storagePath.custom + 'custom-adapter.js'; + scope.adapter = configUtils.config.getContentPath('storage') + 'custom-adapter.js'; configUtils.set({ storage: { @@ -81,7 +81,7 @@ describe('storage: index_spec', function () { describe('adapter validation', function () { it('create good adapter', function () { - scope.adapter = configUtils.config.get('paths').storagePath.custom + 'another-storage.js'; + scope.adapter = configUtils.config.getContentPath('storage') + 'another-storage.js'; configUtils.set({ storage: { @@ -113,7 +113,7 @@ describe('storage: index_spec', function () { }); it('create bad adapter: exists fn is missing', function () { - scope.adapter = configUtils.config.get('paths').storagePath.custom + 'broken-storage.js'; + scope.adapter = configUtils.config.getContentPath('storage') + 'broken-storage.js'; configUtils.set({ storage: { diff --git a/core/test/unit/storage/local-file-store_spec.js b/core/test/unit/storage/local-file-store_spec.js index 88829ad9fd..ad069c8552 100644 --- a/core/test/unit/storage/local-file-store_spec.js +++ b/core/test/unit/storage/local-file-store_spec.js @@ -3,7 +3,6 @@ var fs = require('fs-extra'), path = require('path'), should = require('should'), sinon = require('sinon'), - _ = require('lodash'), LocalFileStore = require('../../../server/storage/local-file-store'), localFileStore, @@ -181,13 +180,7 @@ describe('Local File System Storage', function () { describe('when a custom content path is used', function () { beforeEach(function () { var configPaths = configUtils.defaultConfig.paths; - - configUtils.set({ - paths: _.merge({}, configPaths, { - contentPath: configPaths.appRoot + '/var/ghostcms', - imagesPath: configPaths.appRoot + '/var/ghostcms/' + configPaths.imagesRelPath - }) - }); + configUtils.set('paths:contentPath', configPaths.appRoot + '/var/ghostcms'); }); it('should send the correct path to image', function (done) { @@ -199,21 +192,26 @@ describe('Local File System Storage', function () { }); }); + // @TODO: remove path.join mock... describe('on Windows', function () { var truePathSep = path.sep; beforeEach(function () { sinon.stub(path, 'join'); + sinon.stub(configUtils.config, 'getContentPath').returns('content/images/'); }); afterEach(function () { path.join.restore(); + configUtils.config.getContentPath.restore(); + path.sep = truePathSep; }); it('should return url in proper format for windows', function (done) { path.sep = '\\'; path.join.returns('content\\images\\2013\\09\\IMAGE.jpg'); + localFileStore.save(image).then(function (url) { if (truePathSep === '\\') { url.should.equal('/content/images/2013/09/IMAGE.jpg');