statamic / v2-hub

Statamic 2 - Feature Requests and Bug Reports
https://statamic.com
95 stars 5 forks source link

Logical OR (`||`) used with *some* parens broken? #1432

Open iDGS opened 7 years ago

iDGS commented 7 years ago

(Pasted/edited from Slack:)

{{ if ( true && true ) }} Hello World!{{ /if }}

works, but

{{ if ( true || true ) }} Hello World!{{ /if }}

does not.

Any number of parens by themselves, and it works OK

{{ if ((((( true ))))) }} Hello World!{{ /if }}

but

{{ if ( true ) || true }} Hello World!{{ /if }}

Goes "Whoops, looks like something went wrong."

{{ if ( true ) || ( true ) }} Hello World!{{ /if }}

Fails to work (but should)

{{ if true || ( true ) }} Hello World!{{ /if }}

works as expected.

Substitute && for || and they all work. (edited)

Methinks something's wrong with ||.

Possibly.

jackmcdade commented 7 years ago

Parens are used when you need to group multiple statements and/or modifiers inside one side of a condition. Almost all of those examples are unsupported.

Example:

{{ if segment_1 || (title | lower == "happy") }} do something {{ /if }}
iDGS commented 7 years ago

Might you reconsider?

Here's my real use-case: I have a global state variable for whether or not to show notes from the developer (me) to a site admin—or super user—since in v2 those two are no longer synonymous.

In site/content/globals/state.yaml I have show_dev_notes: 1. But when I run this [1]

{{ user:profile }}
<p>Um... {{ if ( super || is_admin ) && state:show_dev_notes }} Hello World!{{ /if }}</p>
{{ /user:profile }}

it fails silently, and just prints "Um..." (when it should print the whole (demo) expression including "Hello World!")

However, if I flip the conditional expression on the && axis, like so [2],

{{ user:profile }}
<p>Um... {{ if state:show_dev_notes && ( super || is_admin )  }} Hello World!{{ /if }}</p>
{{ /user:profile }}

it works the way it should, and prints "Um... Hello World!"

As it happens, all such conditional expressions on my site are like the above [1] flavor, so I now have to go flop them all around to flavor [2]. And I (and others) now have to beware lest we ever forget that {{S}}'s conditional && expressions are not commutative.

*sigh*

jasonvarga commented 7 years ago

Reopened.

In the future, it would be more helpful if you lead with your actual usage. From your initial issue's contents, it just looked like you were trying to break things for the sake of it.

iDGS commented 7 years ago

Ah—No. Point taken. Sorry for the misunderstanding. I was trying to be "helpful" by reducing what I discovered to it's lowest common denominator.

jackmcdade commented 7 years ago

Understood. We do best with the absolute minimum, real world/actual use case information necessary to communicate the issue. We're pretty good about extending the scenario to its edge cases. We're quite intimate with all the nooks and crannies with issues slip through. Like this.

jackmcdade commented 6 years ago

For Future Jack™ with more brain power: the problem happens around line 729. When the condition triggers looseVariableRegex, which allows spaces so you can run modifiers, but ignores double pipes. This is tricky. Might still take a while to fix because it requires a higher-level approach to extracting all sets of parens, vs trying to spot parse them inline w/ RegEx.