From 623c65c50957ee29ba25b468def5da60b30ff3e9 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Mon, 5 Aug 2019 19:34:12 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=92=A1Changed=20static=20router=20-=20thr?= =?UTF-8?q?ow=20400=20for=20missing=20tpl?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fixes #10990 - Changed the static router to throw a 400 error for a missing template file, rather than falling back to using the default.hbs file - Falling back is weird and hard to understand, but throwing an error makes it clear that the user has to provide the matching template - The new error reads 'Missing template [filename].hbs for route "[route]".' Assume you have a route.yaml file something like: ``` routes: /: home ``` - In Ghost v2, if you don't have a home.hbs template, Ghost falls back to using the default.hbs file if it's available - Most themes have a default.hbs, however this file is a layout file, depended on by other templates, not a template file itself - In production mode, using the default.hbs as a template causes weird, intermittent layout issues depending on which order pages are loaded - This is due to this issue: https://github.com/barc/express-hbs/issues/161 - In Ghost v3, we will throw a 400 error for missing template files instead of having a fallback - In the example above, navigating to '/' would throw the error 'Missing template home.hbs for route "/".' --- .../services/routing/StaticRoutesRouter.js | 6 +- .../services/routing/helpers/templates.js | 2 + core/test/regression/site/site_spec.js | 79 +++++++++---------- .../routing/StaticRoutesRouter_spec.js | 33 ++++---- .../fixtures/themes/test-theme/index.hbs | 9 ++- 5 files changed, 70 insertions(+), 59 deletions(-) diff --git a/core/frontend/services/routing/StaticRoutesRouter.js b/core/frontend/services/routing/StaticRoutesRouter.js index 54d360d54d..1862983fc5 100644 --- a/core/frontend/services/routing/StaticRoutesRouter.js +++ b/core/frontend/services/routing/StaticRoutesRouter.js @@ -111,7 +111,11 @@ class StaticRoutesRouter extends ParentRouter { res.routerOptions = { type: 'custom', templates: this.templates, - defaultTemplate: 'default', + defaultTemplate: () => { + throw new common.errors.IncorrectUsageError({ + message: `Missing template ${res.routerOptions.templates.map(x => `${x}.hbs`).join(', ')} for route "${req.originalUrl}".` + }); + }, data: this.data.query, context: [this.routerName], contentType: this.contentType diff --git a/core/frontend/services/routing/helpers/templates.js b/core/frontend/services/routing/helpers/templates.js index 84ad13ffcc..60f0410e80 100644 --- a/core/frontend/services/routing/helpers/templates.js +++ b/core/frontend/services/routing/helpers/templates.js @@ -130,6 +130,8 @@ _private.pickTemplate = function pickTemplate(templateList, fallback) { if (!template) { if (!fallback) { template = 'index'; + } else if (_.isFunction(fallback)) { + fallback(); } else { template = fallback; } diff --git a/core/test/regression/site/site_spec.js b/core/test/regression/site/site_spec.js index 52a84ce597..b0a690b266 100644 --- a/core/test/regression/site/site_spec.js +++ b/core/test/regression/site/site_spec.js @@ -396,7 +396,9 @@ describe('Integration - Web - Site', function () { before(function () { sinon.stub(frontendSettingsService, 'get').returns({ routes: { - '/': 'home' + '/': { + templates: ['home'] + } }, collections: { @@ -418,7 +420,7 @@ describe('Integration - Web - Site', function () { }); testUtils.integrationTesting.urlService.resetGenerators(); - testUtils.integrationTesting.defaultMocks(sinon); + testUtils.integrationTesting.defaultMocks(sinon, {theme: 'test-theme'}); return testUtils.integrationTesting.initGhost() .then(function () { @@ -454,7 +456,7 @@ describe('Integration - Web - Site', function () { return testUtils.mocks.express.invoke(app, req) .then(function (response) { response.statusCode.should.eql(200); - response.template.should.eql('default'); + response.template.should.eql('home'); }); }); @@ -487,7 +489,7 @@ describe('Integration - Web - Site', function () { }); }); - it('serve collection: podcast', function () { + it('serve collection: podcast with default template', function () { const req = { secure: true, method: 'GET', @@ -506,7 +508,7 @@ describe('Integration - Web - Site', function () { }); }); - it('serve collection: something', function () { + it('serve collection: something with custom template', function () { const req = { secure: true, method: 'GET', @@ -519,9 +521,7 @@ describe('Integration - Web - Site', function () { const $ = cheerio.load(response.body); response.statusCode.should.eql(200); - response.template.should.eql('index'); - - $('.post-card').length.should.equal(2); + response.template.should.eql('something'); }); }); }); @@ -530,14 +530,14 @@ describe('Integration - Web - Site', function () { before(function () { sinon.stub(frontendSettingsService, 'get').returns({ routes: { - '/test/': 'test' + '/something/': {templates: ['something']} }, collections: {}, taxonomies: {} }); testUtils.integrationTesting.urlService.resetGenerators(); - testUtils.integrationTesting.defaultMocks(sinon); + testUtils.integrationTesting.defaultMocks(sinon, {theme: 'test-theme'}); return testUtils.integrationTesting.initGhost() .then(function () { @@ -566,14 +566,14 @@ describe('Integration - Web - Site', function () { const req = { secure: true, method: 'GET', - url: '/test/', + url: '/something/', host: 'example.com' }; return testUtils.mocks.express.invoke(app, req) .then(function (response) { response.statusCode.should.eql(200); - response.template.should.eql('default'); + response.template.should.eql('something'); }); }); }); @@ -1614,7 +1614,6 @@ describe('Integration - Web - Site', function () { before(function () { sinon.stub(frontendSettingsService, 'get').returns({ routes: { - '/about/': 'about', '/podcast/rss/': { templates: ['podcast/rss'], content_type: 'text/xml' @@ -2147,7 +2146,7 @@ describe('Integration - Web - Site', function () { before(function () { sinon.stub(frontendSettingsService, 'get').returns({ routes: { - '/': 'home' + '/': {templates: ['home']} }, collections: { @@ -2169,7 +2168,7 @@ describe('Integration - Web - Site', function () { }); testUtils.integrationTesting.urlService.resetGenerators(); - testUtils.integrationTesting.defaultMocks(sinon); + testUtils.integrationTesting.defaultMocks(sinon, {theme: 'test-theme'}); return testUtils.integrationTesting.initGhost() .then(function () { @@ -2205,7 +2204,7 @@ describe('Integration - Web - Site', function () { return testUtils.mocks.express.invoke(app, req) .then(function (response) { response.statusCode.should.eql(200); - response.template.should.eql('default'); + response.template.should.eql('home'); }); }); @@ -2238,7 +2237,7 @@ describe('Integration - Web - Site', function () { }); }); - it('serve collection: podcast', function () { + it('serve collection: podcast with default template', function () { const req = { secure: true, method: 'GET', @@ -2257,7 +2256,7 @@ describe('Integration - Web - Site', function () { }); }); - it('serve collection: something', function () { + it('serve collection: something with custom template', function () { const req = { secure: true, method: 'GET', @@ -2270,9 +2269,7 @@ describe('Integration - Web - Site', function () { const $ = cheerio.load(response.body); response.statusCode.should.eql(200); - response.template.should.eql('index'); - - $('.post-card').length.should.equal(2); + response.template.should.eql('something'); }); }); }); @@ -2281,14 +2278,14 @@ describe('Integration - Web - Site', function () { before(function () { sinon.stub(frontendSettingsService, 'get').returns({ routes: { - '/test/': 'test' + '/something/': {templates: ['something']} }, collections: {}, taxonomies: {} }); testUtils.integrationTesting.urlService.resetGenerators(); - testUtils.integrationTesting.defaultMocks(sinon); + testUtils.integrationTesting.defaultMocks(sinon, {theme: 'test-theme'}); return testUtils.integrationTesting.initGhost() .then(function () { @@ -2317,14 +2314,14 @@ describe('Integration - Web - Site', function () { const req = { secure: true, method: 'GET', - url: '/test/', + url: '/something/', host: 'example.com' }; return testUtils.mocks.express.invoke(app, req) .then(function (response) { response.statusCode.should.eql(200); - response.template.should.eql('default'); + response.template.should.eql('something'); }); }); }); @@ -2642,7 +2639,8 @@ describe('Integration - Web - Site', function () { router: { pages: [{redirect: true, slug: 'static-page-test'}] } - } + }, + templates: ['page'] } }, @@ -3343,7 +3341,6 @@ describe('Integration - Web - Site', function () { before(function () { sinon.stub(frontendSettingsService, 'get').returns({ routes: { - '/about/': 'about', '/podcast/rss/': { templates: ['podcast/rss'], content_type: 'text/xml' @@ -3876,7 +3873,7 @@ describe('Integration - Web - Site', function () { before(function () { sinon.stub(frontendSettingsService, 'get').returns({ routes: { - '/': 'home' + '/': {templates: ['home']} }, collections: { @@ -3898,7 +3895,7 @@ describe('Integration - Web - Site', function () { }); testUtils.integrationTesting.urlService.resetGenerators(); - testUtils.integrationTesting.defaultMocks(sinon); + testUtils.integrationTesting.defaultMocks(sinon, {theme: 'test-theme'}); return testUtils.integrationTesting.initGhost() .then(function () { @@ -3934,7 +3931,7 @@ describe('Integration - Web - Site', function () { return testUtils.mocks.express.invoke(app, req) .then(function (response) { response.statusCode.should.eql(200); - response.template.should.eql('default'); + response.template.should.eql('home'); }); }); @@ -3967,7 +3964,7 @@ describe('Integration - Web - Site', function () { }); }); - it('serve collection: podcast', function () { + it('serve collection: podcast with default template', function () { const req = { secure: true, method: 'GET', @@ -3986,7 +3983,7 @@ describe('Integration - Web - Site', function () { }); }); - it('serve collection: something', function () { + it('serve collection: something with custom template', function () { const req = { secure: true, method: 'GET', @@ -3999,9 +3996,7 @@ describe('Integration - Web - Site', function () { const $ = cheerio.load(response.body); response.statusCode.should.eql(200); - response.template.should.eql('index'); - - $('.post-card').length.should.equal(2); + response.template.should.eql('something'); }); }); }); @@ -4010,14 +4005,16 @@ describe('Integration - Web - Site', function () { before(function () { sinon.stub(frontendSettingsService, 'get').returns({ routes: { - '/test/': 'test' + '/something/': { + templates: ['something'] + } }, collections: {}, taxonomies: {} }); testUtils.integrationTesting.urlService.resetGenerators(); - testUtils.integrationTesting.defaultMocks(sinon); + testUtils.integrationTesting.defaultMocks(sinon, {theme: 'test-theme'}); return testUtils.integrationTesting.initGhost() .then(function () { @@ -4046,14 +4043,14 @@ describe('Integration - Web - Site', function () { const req = { secure: true, method: 'GET', - url: '/test/', + url: '/something/', host: 'example.com' }; return testUtils.mocks.express.invoke(app, req) .then(function (response) { response.statusCode.should.eql(200); - response.template.should.eql('default'); + response.template.should.eql('something'); }); }); }); @@ -4371,7 +4368,8 @@ describe('Integration - Web - Site', function () { router: { pages: [{redirect: true, slug: 'static-page-test'}] } - } + }, + templates: ['page'] } }, @@ -5072,7 +5070,6 @@ describe('Integration - Web - Site', function () { before(function () { sinon.stub(frontendSettingsService, 'get').returns({ routes: { - '/about/': 'about', '/podcast/rss/': { templates: ['podcast/rss'], content_type: 'text/xml' diff --git a/core/test/unit/services/routing/StaticRoutesRouter_spec.js b/core/test/unit/services/routing/StaticRoutesRouter_spec.js index 6836b78f39..dbfd037a0e 100644 --- a/core/test/unit/services/routing/StaticRoutesRouter_spec.js +++ b/core/test/unit/services/routing/StaticRoutesRouter_spec.js @@ -77,14 +77,15 @@ describe('UNIT - services/routing/StaticRoutesRouter', function () { staticRoutesRouter._prepareStaticRouteContext(req, res, next); next.called.should.be.true(); - res.routerOptions.should.eql({ - type: 'custom', - templates: [], - defaultTemplate: 'default', - context: ['about'], - data: {}, - contentType: undefined - }); + + res.routerOptions.should.have.properties('type', 'templates', 'defaultTemplate', 'context', 'data', 'contentType'); + res.routerOptions.type.should.eql('custom'); + res.routerOptions.templates.should.eql([]); + res.routerOptions.defaultTemplate.should.be.a.Function(); + res.routerOptions.context.should.eql(['about']); + res.routerOptions.data.should.eql({}); + + should(res.routerOptions.contentType).be.undefined(); should.not.exist(res.locals.slug); }); @@ -93,14 +94,14 @@ describe('UNIT - services/routing/StaticRoutesRouter', function () { staticRoutesRouter._prepareStaticRouteContext(req, res, next); next.called.should.be.true(); - res.routerOptions.should.eql({ - type: 'custom', - templates: [], - defaultTemplate: 'default', - context: ['index'], - data: {}, - contentType: undefined - }); + + res.routerOptions.should.have.properties('type', 'templates', 'defaultTemplate', 'context', 'data', 'contentType'); + res.routerOptions.type.should.eql('custom'); + res.routerOptions.templates.should.eql([]); + res.routerOptions.defaultTemplate.should.be.a.Function(); + res.routerOptions.context.should.eql(['index']); + res.routerOptions.data.should.eql({}); + should.not.exist(res.locals.slug); }); }); diff --git a/core/test/utils/fixtures/themes/test-theme/index.hbs b/core/test/utils/fixtures/themes/test-theme/index.hbs index e9194a5a61..d73bab62b8 100644 --- a/core/test/utils/fixtures/themes/test-theme/index.hbs +++ b/core/test/utils/fixtures/themes/test-theme/index.hbs @@ -1 +1,8 @@ -
{{tag.slug}}
\ No newline at end of file +
{{tag.slug}}
+ +{{#foreach posts}} + +{{!-- The tag below includes the markup for each post - partials/post-card.hbs --}} +
{{title}}
+ +{{/foreach}} \ No newline at end of file