gobuffalo / plush

The powerful template system that Go needs
MIT License
895 stars 57 forks source link

Can't evaluate if conditions properly with an undefined variable #157

Closed Mido-sys closed 1 year ago

Mido-sys commented 2 years ago

Plush evaluates undefined variables in if conditions without throwing an error; however, it becomes an issue with this example.

    r := require.New(t)
    type page struct {
        PageTitle string
    }
    g := &page{"cafe"}
    ctx := NewContext()
    ctx.Set("pages", g)
    ctx.Set("path", "home")

    input := `<%= if ( path != "pagePath" || (page && page.PageTitle != "cafe") ) { %>hi<%} %>`

    s, err := Render(input, ctx)
    r.NoError(err)
    r.Equal("hi", s)

Currently, this should return "hi" as one of the conditions path != "pagePath" is true; however, it returns an empty string because the if condition values on the right of the or return an error, so it bubbles back up and results in evaluating the condition as false.

This behaviour is inconsistent because this condition evaluates as true <%= if ( path != "pagePath" || !page) { %>hi<%} %>

Mido-sys commented 2 years ago

A quick fix will be adding this :

if node.Operator == "||" {
    if c.isTruthy(lres) {
        return true, nil
    }
}

Here but it will still fail if the "true" condition is on the right of the operator and not left like this one:

    ctx.Set("path", "cart")
    ctx.Set("paths", "cart")
    input := `<%= if ( path == "pagePath" || (page && page.PageTitle != "cafe") || paths == "cart") { %>hi<%} %>`
github-actions[bot] commented 1 year ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment. Otherwise, this will be closed in 7 days.

mattbnz commented 1 year ago

I've hit this issue too so I'd like this to remain open rather than be auto-closed as stale. It's definitely a pain-point with plush.

sio4 commented 1 year ago

I've hit this issue too so I'd like this to remain open rather than be auto-closed as stale. It's definitely a pain-point with plush.

Sure, absolutely! I configured auto close action across all modules to make it easy to maintain, but if any issue is still meaningful, it should remain as open. The main purpose of this autoclose action is actually not to close issues but make it possible to get an insight into the issue. Also, the more information gathered, the better the solution will be.

Mido-sys commented 1 year ago

@sio4, I pushed a fix for this.

Thanks, @mattbnz, for the reminder.

sio4 commented 1 year ago

@sio4, I pushed a fix for this.

Thanks, @mattbnz, for the reminder.

Thank! Will check!

Mido-sys commented 1 year ago

I pushed an improved version. I realized my first commit would have ignored syntax errors and returned true if one of the nodes was true. I also added more tests.