💡Changed static router - throw 400 for missing tpl

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 "/".'
This commit is contained in:
Hannah Wolfe 2019-08-05 19:34:12 +01:00
parent b875cc339d
commit 623c65c509
5 changed files with 70 additions and 59 deletions

View File

@ -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

View File

@ -130,6 +130,8 @@ _private.pickTemplate = function pickTemplate(templateList, fallback) {
if (!template) {
if (!fallback) {
template = 'index';
} else if (_.isFunction(fallback)) {
fallback();
} else {
template = fallback;
}

View File

@ -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'

View File

@ -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);
});
});

View File

@ -1 +1,8 @@
<div class="slug">{{tag.slug}}</div>
<div class="slug">{{tag.slug}}</div>
{{#foreach posts}}
{{!-- The tag below includes the markup for each post - partials/post-card.hbs --}}
<article class="post-card {{post_class}}{{#unless feature_image}} no-image{{/unless}}">{{title}}</article>
{{/foreach}}