Infrastructure/ghost
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2025-01-20 22:42:53 -05:00
Code Issues Activity

Added full SafeString handling to match helper

Browse source 
refs: https://github.com/TryGhost/Team/issues/759

- No matter what, a handlebars helper outputs a string. So if you return true, you'll always get 'true'.
- SafeStrings are handlebars's way of passing around a string whilst also maintaining a record of the original value e.g. new SafeString(true) results in {string: true}
- We need this for the match helper, so that we know when doing a comparison that we're meant to be comparing against a boolean true, not a string true
- Therefore, we need to putput SafeStrings, but also process them when passed in

The logic
- Figuring out the correct logic here has been a little tricky but essentially:
  - {{match safestring}} with a single arg, will return true for any truthy value
  - {{match safestring "=" true}} does a direct comparison with the original value of the safe string, so if it was a boolean true, the match will be true else false
  - {{match (match something) "=" true}} will therefore work for any level of nesting
  - this can result in slightly inconsistent results, but feels correct and documentable

This is documented extensively through the test cases
This commit is contained in:
Hannah Wolfe 2021-10-14 11:04:44 +01:00
parent 565ced555c
commit fbc23459fc
No known key found for this signature in database
GPG key ID: 9F8C7532D0A6BA55
2 changed files with 104 additions and 11 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

17
core/frontend/helpers/match.js
View file

@ -29,10 +29,6 @@ const handleConditional = (conditional, options) => {
conditional = conditional.call(this);
}
if (conditional instanceof SafeString) {
conditional = conditional.string;
}
// Default behavior is to render the positive path if the value is truthy and not empty.
// The `includeZero` option may be set to treat the condtional as purely not empty based on the
// behavior of isEmpty. Effectively this determines if 0 is handled by the positive path or negative.
@ -71,11 +67,20 @@ function match(...attrs) {
return;
}
// If any of the attributes are safe strings, change them back to their original value
attrs = attrs.map((attr) => {
if (attr instanceof SafeString) {
return attr.string;
}
return attr;
});
if (attrs.length === 1) {
// If we only have one attribute, treat it as simple true/false (like the if helper)
result = handleConditional(attrs[0], options);
} else if (attrs.length === 3) {
result = handleMatch(attrs[0], attrs[1], attrs[2], options);
result = handleMatch(attrs[0], attrs[1], attrs[2]);
} else {
logging.warn(tpl(messages.invalidAttribute));
return;
@ -90,7 +95,7 @@ function match(...attrs) {
return options.inverse(this);
}
// Else return the result as a string
// Else return the result as a SafeString Eg.{string: false} || {string: true}
return new SafeString(result);
}

98
test/unit/frontend/helpers/match.test.js
View file

@ -1,13 +1,16 @@
const should = require('should');
const sinon = require('sinon');
const _ = require('lodash');
const match = require('../../../../core/frontend/helpers/match');
const matchHelper = require('../../../../core/frontend/helpers/match');
const titleHelper = require('../../../../core/frontend/helpers/title');
const labs = require('../../../../core/shared/labs');
const handlebars = require('../../../../core/frontend/services/theme-engine/engine').handlebars;
const {SafeString} = require('express-hbs');
describe('Match helper', function () {
before(function () {
handlebars.registerHelper('match', match);
handlebars.registerHelper('match', matchHelper);
handlebars.registerHelper('title', titleHelper);
});
afterEach(function () {
@ -49,6 +52,13 @@ describe('Match helper', function () {
zero: 0,
one: 1,
string: 'Hello world',
title: 'The Title',
string_true: 'true',
string_false: 'false',
safestring_string_true: new SafeString('true'),
safestring_string_false: new SafeString('false'),
safestring_bool_true: new SafeString(true),
safestring_bool_false: new SafeString(false),
five: 5,
string_five: '5',
empty: '',
@ -59,7 +69,6 @@ describe('Match helper', function () {
}
};
// @TODO: Fix this!
describe('Basic values', function () {
runTests({
'{{match truthy_bool}}': 'true',
@ -67,12 +76,20 @@ describe('Match helper', function () {
'{{match one}}': 'true',
'{{match zero}}': 'false',
'{{match string}}': 'true',
'{{match string_true}}': 'true',
'{{match string_false}}': 'true',
'{{match safestring_string_true}}': 'true',
'{{match safestring_string_false}}': 'true',
'{{match safestring_bool_true}}': 'true',
'{{match safestring_bool_false}}': 'false',
'{{match empty}}': 'false',
'{{match null}}': 'false',
'{{match undefined}}': 'false',
'{{match unknown}}': 'false',
'{{match object}}': 'true',
'{{match (title)}}': 'true',
// Zero works if includeZero is set
'{{match zero includeZero=true}}': 'true',
@ -94,6 +111,18 @@ describe('Match helper', function () {
runTests({
'{{match string "=" "Hello world"}}': 'true',
'{{match string "=" "Hello world!"}}': 'false',
'{{match string_true "=" "true"}}': 'true',
'{{match string_true "=" true}}': 'false',
'{{match string_false "=" "false"}}': 'true',
'{{match string_false "=" false}}': 'false',
'{{match safestring_string_true "=" "true"}}': 'true',
'{{match safestring_string_true "=" true}}': 'false',
'{{match safestring_string_false "=" "false"}}': 'true',
'{{match safestring_string_false "=" false}}': 'false',
'{{match safestring_bool_true "=" "true"}}': 'false',
'{{match safestring_bool_true "=" true}}': 'true',
'{{match safestring_bool_false "=" "false"}}': 'false',
'{{match safestring_bool_false "=" false}}': 'true',
'{{match truthy_bool "=" true}}': 'true',
'{{match truthy_bool "=" false}}': 'false',
'{{match falsy_bool "=" false}}': 'true',
@ -101,7 +130,10 @@ describe('Match helper', function () {
'{{match one "=" 1}}': 'true',
'{{match one "=" "1"}}': 'false',
'{{match zero "=" 0}}': 'true',
'{{match zero "=" "0"}}': 'false'
'{{match zero "=" "0"}}': 'false',
'{{match (title) "=" "The Title"}}': 'true',
'{{match (title) "=" "The Title!"}}': 'false'
}, hash);
});
@ -109,6 +141,18 @@ describe('Match helper', function () {
runTests({
'{{match string "!=" "Hello world"}}': 'false',
'{{match string "!=" "Hello world!"}}': 'true',
'{{match string_true "!=" true}}': 'true',
'{{match string_true "!=" "true"}}': 'false',
'{{match string_false "!=" false}}': 'true',
'{{match string_false "!=" "false"}}': 'false',
'{{match safestring_string_true "!=" "true"}}': 'false',
'{{match safestring_string_true "!=" true}}': 'true',
'{{match safestring_string_false "!=" "false"}}': 'false',
'{{match safestring_string_false "!=" false}}': 'true',
'{{match safestring_bool_true "!=" "true"}}': 'true',
'{{match safestring_bool_true "!=" true}}': 'false',
'{{match safestring_bool_false "!=" "false"}}': 'true',
'{{match safestring_bool_false "!=" false}}': 'false',
'{{match truthy_bool "!=" true}}': 'false',
'{{match truthy_bool "!=" false}}': 'true',
'{{match falsy_bool "!=" false}}': 'false',
@ -116,9 +160,53 @@ describe('Match helper', function () {
'{{match one "!=" 1}}': 'false',
'{{match one "!=" "1"}}': 'true',
'{{match zero "!=" 0}}': 'false',
'{{match zero "!=" "0"}}': 'true'
'{{match zero "!=" "0"}}': 'true',
'{{match (title) "!=" "The Title"}}': 'false',
'{{match (title) "!=" "The Title!"}}': 'true'
}, hash);
});
// SafeStrings represent the original value as an object for example:
// SafeString { string: true } vs SafeString { string: 'true' }
// allows us to know if the original value was a boolean or a string
// These tests make sure that we can compare to the _originaL_ value
// But that we don't start allowing weird things like boolean true being equal to string true
describe('SafeString behaviour makes sense(ish)', function () {
runTests({
// Title equals true value = true
'{{match (match title "=" "The Title") "=" "true"}}': 'false',
'{{match (match title "=" "The Title") "=" true}}': 'true',
'{{match (match title "=" "The Title")}}': 'true',
// With title as a helper that also outputs a SafeString
'{{match (match (title) "=" "The Title") "=" "true"}}': 'false',
'{{match (match (title) "=" "The Title") "=" true}}': 'true',
'{{match (match (title) "=" "The Title")}}': 'true',
// Title equals false value = true
'{{match (match title "=" "The Title!") "=" "false"}}': 'false',
'{{match (match title "=" "The Title!") "=" false}}': 'true',
'{{match (match title "=" "The Title!")}}': 'false',
// With title as a helper that also outputs a SafeString
'{{match (match (title) "=" "The Title!") "=" "false"}}': 'false',
'{{match (match (title) "=" "The Title!") "=" false}}': 'true',
'{{match (match (title) "=" "The Title!")}}': 'false',
// // Reverse, reverse!
// // Title not equals true value = false
'{{match (match title "!=" "The Title") "=" "false"}}': 'false',
'{{match (match title "!=" "The Title") "=" false}}': 'true',
'{{match (match title "!=" "The Title")}}': 'false',
// With title as a helper that also outputs a SafeString
'{{match (match (title) "!=" "The Title") "=" "false"}}': 'false',
'{{match (match (title) "!=" "The Title") "=" false}}': 'true',
'{{match (match (title) "!=" "The Title")}}': 'false',
// Yoda a complex example or two to prove this works
'{{match false "=" (match title "!=" "The Title")}}': 'true',
'{{match "false" "=" (match (title) "!=" "The Title")}}': 'false'
}, {title: 'The Title'});
});
});
// By using match as a block helper, instead of returning true or false, the matching template is executed