plush icon indicating copy to clipboard operation
plush copied to clipboard

Can't evaluated if conditions properly with an undefined variable

Open Mido-sys opened this issue 3 years ago • 1 comments

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 avatar Jun 08 '22 14:06 Mido-sys

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<%} %>`

Mido-sys avatar Jun 08 '22 15:06 Mido-sys

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.

github-actions[bot] avatar Sep 27 '22 03:09 github-actions[bot]

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.

mattbnz avatar Sep 27 '22 08:09 mattbnz

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.

sio4 avatar Sep 27 '22 11:09 sio4

@sio4, I pushed a fix for this.

Thanks, @mattbnz, for the reminder.

Mido-sys avatar Sep 27 '22 14:09 Mido-sys

@sio4, I pushed a fix for this.

Thanks, @mattbnz, for the reminder.

Thank! Will check!

sio4 avatar Sep 27 '22 14:09 sio4

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.

Mido-sys avatar Sep 27 '22 14:09 Mido-sys