tpl: Refactor and fix substr logic

Fix miscalculations when start is negative.  Results should now match
PHP substr.

Fixes #7993

2 files changed, 45 insertions, 33 deletions

diff --git a/tpl/strings/strings.go b/tpl/strings/strings.go
index c87b532d3..9427d8e8a 100644
--- a/tpl/strings/strings.go
+++ b/tpl/strings/strings.go
@@ -279,64 +279,73 @@ func (ns *Namespace) Split(a interface{}, delimiter string) ([]string, error) {
 // if length is given and is negative, then that many characters will be omitted from
 // the end of string.
 func (ns *Namespace) Substr(a interface{}, nums ...interface{}) (string, error) {
-	aStr, err := cast.ToStringE(a)
+	s, err := cast.ToStringE(a)
 	if err != nil {
 		return "", err
 	}
 
-	var start, length int
+	asRunes := []rune(s)
+	rlen := len(asRunes)
 
-	asRunes := []rune(aStr)
+	var start, length int
 
 	switch len(nums) {
 	case 0:
-		return "", errors.New("too less arguments")
+		return "", errors.New("too few arguments")
 	case 1:
 		if start, err = cast.ToIntE(nums[0]); err != nil {
-			return "", errors.New("start argument must be integer")
+			return "", errors.New("start argument must be an integer")
 		}
-		length = len(asRunes)
+		length = rlen
 	case 2:
 		if start, err = cast.ToIntE(nums[0]); err != nil {
-			return "", errors.New("start argument must be integer")
+			return "", errors.New("start argument must be an integer")
 		}
 		if length, err = cast.ToIntE(nums[1]); err != nil {
-			return "", errors.New("length argument must be integer")
+			return "", errors.New("length argument must be an integer")
 		}
 	default:
 		return "", errors.New("too many arguments")
 	}
 
-	if start < -len(asRunes) {
+	if rlen == 0 {
+		return "", nil
+	}
+
+	if start < 0 {
+		start += rlen
+	}
+
+	// start was originally negative beyond rlen
+	if start < 0 {
 		start = 0
 	}
-	if start > len(asRunes) {
-		return "", fmt.Errorf("start position out of bounds for %d-byte string", len(aStr))
+
+	if start > rlen-1 {
+		return "", fmt.Errorf("start position out of bounds for %d-byte string", rlen)
 	}
 
-	var s, e int
-	if start >= 0 && length >= 0 {
-		s = start
-		e = start + length
-	} else if start < 0 && length >= 0 {
-		s = len(asRunes) + start - length + 1
-		e = len(asRunes) + start + 1
-	} else if start >= 0 && length < 0 {
-		s = start
-		e = len(asRunes) + length
-	} else {
-		s = len(asRunes) + start
-		e = len(asRunes) + length
+	end := rlen
+
+	if length < 0 {
+		end += length
+	} else if length > 0 {
+		end = start + length
+	}
+
+	if start >= end {
+		return "", fmt.Errorf("calculated start position greater than end position: %d > %d", start, end)
 	}
 
-	if s > e {
-		return "", fmt.Errorf("calculated start position greater than end position: %d > %d", s, e)
+	if end < 0 {
+		return "", nil
 	}
-	if e > len(asRunes) {
-		e = len(asRunes)
+
+	if end > rlen {
+		end = rlen
 	}
 
-	return string(asRunes[s:e]), nil
+	return string(asRunes[start:end]), nil
 }
 
 // Title returns a copy of the input s with all Unicode letters that begin words
diff --git a/tpl/strings/strings_test.go b/tpl/strings/strings_test.go
index 6a14acd78..bb90200c0 100644
--- a/tpl/strings/strings_test.go
+++ b/tpl/strings/strings_test.go
@@ -441,8 +441,11 @@ func TestSubstr(t *testing.T) {
 	}{
 		{"abc", 1, 2, "bc"},
 		{"abc", 0, 1, "a"},
-		{"abcdef", -1, 2, "ef"},
-		{"abcdef", -3, 3, "bcd"},
+		{"abcdef", -1, 2, "f"},
+		{"abcdef", -3, 3, "def"},
+		{"abcdef", -1, nil, "f"},
+		{"abcdef", -2, nil, "ef"},
+		{"abcdef", -3, 1, "d"},
 		{"abcdef", 0, -1, "abcde"},
 		{"abcdef", 2, -1, "cde"},
 		{"abcdef", 4, -4, false},
@@ -480,12 +483,12 @@ func TestSubstr(t *testing.T) {
 		}
 
 		if b, ok := test.expect.(bool); ok && !b {
-			c.Assert(err, qt.Not(qt.IsNil))
+			c.Assert(err, qt.Not(qt.IsNil), qt.Commentf("%v", test))
 			continue
 		}
 
 		c.Assert(err, qt.IsNil)
-		c.Assert(result, qt.Equals, test.expect)
+		c.Assert(result, qt.Equals, test.expect, qt.Commentf("%v", test))
 	}
 
 	_, err = ns.Substr("abcdef")