neos / neos-development-collection

The unified repository containing the Neos core packages, used for Neos development.
https://www.neos.io/
GNU General Public License v3.0
260 stars 222 forks source link

Feature: warn when `@if` and `@process` in fusion are used directly without subkeys #3703

Open mhsdesign opened 2 years ago

mhsdesign commented 2 years ago

The initial idea

Feature: simplify @if and @process in fusion (no needed subkeys)

turned out to be a bad idea. See https://github.com/neos/neos-development-collection/issues/3703#issuecomment-1292167671 for what is the actual reason for this issue.

original pr description:

This feature was requested once for AFX and is since 7.2 merged: https://github.com/neos/fusion-afx/issues/29

While implementing this, there was also a discussion if this could be implemented directly in fusion. https://github.com/neos/neos-development-collection/pull/3428#issuecomment-951218360

currently this is the way to go:

@if.1 = ...
@if.2 = ...

while this is working fine and perfectly overrideable its not super beginner friendly, as when one forgets the '.1' it will just be silently ignored. Also adding those subkeys makes the code more verbose.

id like to be able to write:

@if = ...

but the question would be, what happens then to multiple @ifs ...

@if = ...
@if = ...

Option A: the latter if will override the first if as usual and the runtime will be used to handle this case

Option B: the parser (merged array tree ast node visitor) will treat them as @if.if_1 and @if.if_2

B would mean they could never be overriden, what seems fine as overriding non named ifs seems like a worse idea than overriding in general ^^ But B would also mean that the parser has too much knowledge about the distiction between an @if and @position (as the position meta key only allows direct assigns)

mhsdesign commented 2 years ago

@mficzel what do you say?

mhsdesign commented 1 year ago

the easiest option is to handle it in the merged array tree ast node visitor (after the parsing process) and generate a fake subpath @if.if_root__


@if = ${value}

would become

{
  "__meta": {
    "if": {
      "if_root__": {
        "__eelExpression": "value"
      }
    }
  }
}

there is a catch though: currently using this: @if >

would unset all ifs applied.

in the future we would need to decide between still unsetting all ifs (inconsistent) or only unset the fake if_root__ (breaking)

kitsunet commented 1 year ago

I would not implement it in Fusion. In AFX you cannot amend existing code, you have to write the whole AFX again, so no problems there. But in Fusion you would have to check previously existing protoype code for solo/multi conditions/processors otherwise very weird things will happen to you. Will lead to many questions and confusion especially with beginners, IMHO totally not worth.

mhsdesign commented 1 year ago

Okay i agree. I have a hard time myself defining a spec in my head with all these edge cases ^^

I had the idea for this feature because sometime ago i tried to present Fusion to someone and missed the subkey of @process in my live demo. ^^ That was very frustrating as neos didnt tell me that i was doing anything wrong ^^

And i want to fix this in some way.

One way is described here: just make it work.

But i was also thinking about a fusion linter #3766 in this issue @cvette said

However, for your example, I would actually prefer a warning or even an exception from the Fusion runtime. Better to fail early, than to discover some weird behavior later.

i agree on that too - the question is should the runtime handle that or the fusion parsing process?

the runtime could look if @if or @process have directly a value assigned (see canEvaluate) and throw an exception in DEV context (to be non-breaking and later every time?) ;)