Repository: harttle/liquidjs
Pull request #671 (2024-02-16 02:45:55)

fix: in conditionals, don't render anything after an else branch
This PR enforces a simple rule: don't render anything after an `else` branch.

There are several bugs currently where `if`, `unless` and `case` conditionals can have branches that occur after an `else` render, or that branches after an `else` can actually cause the `else` branch itself not to render.

Fixes: #670 and #672

## Example
```liquid
<p>If</p>
{% if false %}
  don't show
  {%- else -%}
  show
  {%- else -%}
  don't show
{% endif %}

<p>If with Elsif</p>
{% if false %}
  don't show
  {%- else -%}
  show
  {%- elsif true -%}
  don't show
{% endif %}

<p>Unless</p>
{% unless true %}
  don't show
  {%- else -%}
  show
  {%- else -%}
  don't show
{% endunless %}

<p>Case</p>
{% case true %}
  {%- when false- %}
    don't show
  {%- else -%}
    show
  {%- else -%}
    don't show
{% endcase %}

<p>Case with When</p>
{% case true -%}
  {% when false -%}
    don't show (false)
  {%- else -%}
    show
  {%- else -%}
    don't show (second else)
  {%- when true -%}
    don't show (truthy when condition after else)
  {%- when true -%}
    don't show (truthy when condition after else)
{%- endcase %}
```

**`master` Behaviour**:
```html
<p>If</p>
showdon't show

<p>If with Elsif</p>
don't show

<p>Unless</p>
showdon't show

<p>Case</p>
showdon't show

<p>Case with When</p>
don't show (truthy when condition after else)don't show (truthy when condition after else)
```

**PR Behaviour**:
```html
<p>If</p>
show

<p>If with Elsif</p>
show

<p>Unless</p>
show

<p>Case</p>
show
 
<p>Case with When</p>
show
```

coveralls: ## Pull Request Test Coverage Report for [Build 7925339311](https://coveralls.io/builds/65735228)


### Details

* **0** of **8**   **(100.0%)**  changed or added relevant lines in **3** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.003%**) to **99.471%**

---



|  Totals | [![Coverage Status](https://coveralls.io/builds/65735228/badge)](https://coveralls.io/builds/65735228) |
| :-- | --: |
| Change from base [Build 7895611436](https://coveralls.io/builds/65683580): |  0.003% |
| Covered Lines: | 2249 |
| Relevant Lines: | 2256 |

---
##### 💛  - [Coveralls](https://coveralls.io)


Patch:
diff --git a/src/tags/case.ts b/src/tags/case.ts
index 23564d3a8..de0d2d651 100644
--- a/src/tags/case.ts
+++ b/src/tags/case.ts
@@ -10,8 +10,13 @@ export default class extends Tag {
     this.elseTemplates = []
 
     let p: Template[] = []
+    let elseCount = 0
     const stream: ParseStream = this.liquid.parser.parseStream(remainTokens)
       .on('tag:when', (token: TagToken) => {
+        if (elseCount > 0) {
+          return
+        }
+
         p = []
 
         const values: ValueToken[] = []
@@ -29,9 +34,16 @@ export default class extends Tag {
           templates: p
         })
       })
-      .on('tag:else', () => (p = this.elseTemplates))
+      .on('tag:else', () => {
+        elseCount++
+        p = this.elseTemplates
+      })
       .on('tag:endcase', () => stream.stop())
-      .on('template', (tpl: Template) => p.push(tpl))
+      .on('template', (tpl: Template) => {
+        if (p !== this.elseTemplates || elseCount === 1) {
+          p.push(tpl)
+        }
+      })
       .on('end', () => {
         throw new Error(`tag ${tagToken.getText()} not closed`)
       })
diff --git a/src/tags/if.ts b/src/tags/if.ts
index 8e6651793..f563c8060 100644
--- a/src/tags/if.ts
+++ b/src/tags/if.ts
@@ -6,19 +6,33 @@ export default class extends Tag {
 
   constructor (tagToken: TagToken, remainTokens: TopLevelToken[], liquid: Liquid) {
     super(tagToken, remainTokens, liquid)
-    let p
+    let p: Template[] = []
+    let elseCount = 0
     liquid.parser.parseStream(remainTokens)
       .on('start', () => this.branches.push({
         value: new Value(tagToken.args, this.liquid),
         templates: (p = [])
       }))
-      .on('tag:elsif', (token: TagToken) => this.branches.push({
-        value: new Value(token.args, this.liquid),
-        templates: (p = [])
-      }))
-      .on('tag:else', () => (p = this.elseTemplates))
+      .on('tag:elsif', (token: TagToken) => {
+        if (elseCount > 0) {
+          p = []
+          return
+        }
+        this.branches.push({
+          value: new Value(token.args, this.liquid),
+          templates: (p = [])
+        })
+      })
+      .on('tag:else', () => {
+        elseCount++
+        p = this.elseTemplates
+      })
       .on('tag:endif', function () { this.stop() })
-      .on('template', (tpl: Template) => p.push(tpl))
+      .on('template', (tpl: Template) => {
+        if (p !== this.elseTemplates || elseCount === 1) {
+          p.push(tpl)
+        }
+      })
       .on('end', () => { throw new Error(`tag ${tagToken.getText()} not closed`) })
       .start()
   }
diff --git a/src/tags/unless.ts b/src/tags/unless.ts
index 390d7931b..56216aa5c 100644
--- a/src/tags/unless.ts
+++ b/src/tags/unless.ts
@@ -5,21 +5,35 @@ export default class extends Tag {
   elseTemplates: Template[] = []
   constructor (tagToken: TagToken, remainTokens: TopLevelToken[], liquid: Liquid) {
     super(tagToken, remainTokens, liquid)
-    let p
+    let p: Template[] = []
+    let elseCount = 0
     this.liquid.parser.parseStream(remainTokens)
       .on('start', () => this.branches.push({
         value: new Value(tagToken.args, this.liquid),
         test: isFalsy,
         templates: (p = [])
       }))
-      .on('tag:elsif', (token: TagToken) => this.branches.push({
-        value: new Value(token.args, this.liquid),
-        test: isTruthy,
-        templates: (p = [])
-      }))
-      .on('tag:else', () => (p = this.elseTemplates))
+      .on('tag:elsif', (token: TagToken) => {
+        if (elseCount > 0) {
+          p = []
+          return
+        }
+        this.branches.push({
+          value: new Value(token.args, this.liquid),
+          test: isTruthy,
+          templates: (p = [])
+        })
+      })
+      .on('tag:else', () => {
+        elseCount++
+        p = this.elseTemplates
+      })
       .on('tag:endunless', function () { this.stop() })
-      .on('template', (tpl: Template) => p.push(tpl))
+      .on('template', (tpl: Template) => {
+        if (p !== this.elseTemplates || elseCount === 1) {
+          p.push(tpl)
+        }
+      })
       .on('end', () => { throw new Error(`tag ${tagToken.getText()} not closed`) })
       .start()
   }


Test patch:
diff --git a/test/e2e/issues.spec.ts b/test/e2e/issues.spec.ts
index a195b4919..a73257c99 100644
--- a/test/e2e/issues.spec.ts
+++ b/test/e2e/issues.spec.ts
@@ -469,4 +469,20 @@ describe('Issues', function () {
     const result = engine.parseAndRenderSync('{{ÜLKE}}', { ÜLKE: 'Türkiye' })
     expect(result).toEqual('Türkiye')
   })
+  it('#670 Should not render anything after an else branch', () => {
+    const engine = new Liquid()
+    const result = engine.parseAndRenderSync('{% assign value = "this" %}' +
+      '{% if false %}don\'t show' +
+      '{% else %}show {{ value }}' +
+    '{% else %}don\'t show{% endif %}', {})
+    expect(result).toEqual('show this')
+  })
+  it('#672 Should not render an elseif after an else branch', () => {
+    const engine = new Liquid()
+    const result = engine.parseAndRenderSync('{% if false %}don\'t show' +
+      '{% else %}show' +
+      '{% elsif true %}don\'t show' +
+    '{% endif %}', {})
+    expect(result).toEqual('show')
+  })
 })
diff --git a/test/integration/tags/case.spec.ts b/test/integration/tags/case.spec.ts
index 3312eb800..ce0d8f86f 100644
--- a/test/integration/tags/case.spec.ts
+++ b/test/integration/tags/case.spec.ts
@@ -91,4 +91,32 @@ describe('tags/case', function () {
     const html = await liquid.parseAndRender(src)
     return expect(html).toBe('and or or')
   })
+  it('should not render anything after an else branch', async function () {
+    const html = await liquid.parseAndRenderSync('{% assign value = "this" %}' +
+    '{% case true %}' +
+      '{% when false %}don\'t show' +
+      '{% else %}show {{ value }}' +
+      '{% else %}don\'t show' +
+    '{% endcase %}', {})
+    expect(html).toEqual('show this')
+  })
+  it('should not render anything after an else branch even when first else branch is empty', async function () {
+    const html = await liquid.parseAndRenderSync('{% case true %}' +
+      '{% when false %}don\'t show' +
+      '{% else %}' +
+      '{% else %}don\'t show' +
+    '{% endcase %}', {})
+    expect(html).toEqual('')
+  })
+  it('should not render anything after an else branch even when there are \'when\' conditions', () => {
+    const engine = new Liquid()
+    const result = engine.parseAndRenderSync('{% assign value = "this" %}' +
+    '{% case true -%}' +
+      '{% when false -%}don\'t show' +
+      '{% else %}show {{ value }}' +
+      '{% else %}don\'t show' +
+      '{%- when true -%}don\'t show' +
+    '{%- endcase %}', {})
+    expect(result).toEqual('show this')
+  })
 })
diff --git a/test/integration/tags/if.spec.ts b/test/integration/tags/if.spec.ts
index ba6c35fb1..2086b0360 100644
--- a/test/integration/tags/if.spec.ts
+++ b/test/integration/tags/if.spec.ts
@@ -143,4 +143,12 @@ describe('tags/if', function () {
     const html = await liquid.parseAndRender(src, scope)
     return expect(html).toBe('success')
   })
+  it('should not render anything after an else branch even when first else branch is empty', () => {
+    const engine = new Liquid()
+    const result = engine.parseAndRenderSync('{% if false %}don\'t show' +
+      '{% else %}' +
+      '{% else %}don\'t show' +
+    '%{% endif %}', {})
+    expect(result).toEqual('')
+  })
 })
diff --git a/test/integration/tags/unless.spec.ts b/test/integration/tags/unless.spec.ts
index f31175051..5ca0446a6 100644
--- a/test/integration/tags/unless.spec.ts
+++ b/test/integration/tags/unless.spec.ts
@@ -47,6 +47,29 @@ describe('tags/unless', function () {
       Inside wonderland
       After wonderland`)
   })
+  it('should not render anything after an else branch', async function () {
+    const html = await liquid.parseAndRenderSync('{% assign value = "this" %}' +
+      '{% unless true %}don\'t show' +
+      '{% else %}show {{ value }}' +
+      '{% else %}don\'t show' +
+    '{% endunless %}', {})
+    expect(html).toEqual('show this')
+  })
+  it('should not render anything after an else branch even when first else branch is empty', async function () {
+    const html = await liquid.parseAndRenderSync('{% unless true %}don\'t show' +
+      '{% else %}' +
+      '{% else %}don\'t show' +
+    '{% endunless %}', {})
+    expect(html).toEqual('')
+  })
+  it('should not render an elseif after an else branch', () => {
+    const engine = new Liquid()
+    const result = engine.parseAndRenderSync('{% unless true %}don\'t show' +
+      '{% else %}show' +
+      '{% elsif true %}don\'t show' +
+    '{% endunless %}', {})
+    expect(result).toEqual('show')
+  })
 
   describe('sync support', function () {
     it('should render else when predicate yields true', function () {