Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Translation layer for Node read-model in Fusion to restore 8.3 syntax #5022

Open
mhsdesign opened this issue Apr 30, 2024 · 1 comment
Open
Labels

Comments

@mhsdesign
Copy link
Member

mhsdesign commented Apr 30, 2024

intro

For simplicity we should add a translation layer which will allow the well known Neos 8.3 syntax of working with Nodes in EEL/Fusion. Among some other missed properties are:

  • ${node.identifier}
  • ${node.nodeType}
  • ${node.label}

While we have rector migrations for nearly every use-case it will be hard to teach the new often too verbose and hard to understand and find out syntax.

This was discussed and endorsed together in todays weekly (30.04) with @ahaeslich @Sebobo @skurfuerst and me

problems

strict (non string-able) value objects in eel

With #4156 we made value objects stricter and removed their ability to be auto casted to strings. That means instead of writing ${node.nodeAggregateId} one needs to write ${node.nodeAggregateId.value}

This caused a big controversy which is only partly documented here but was also discussed in several meetings:
#4156 (comment)

We concur that having to try out .value only sometimes in certain scenarios will be super frustrating and hard to use. EEL was made in times where people were not using php like this and is best used with primitives / objects that have getters with primitives.

enums in eel

Enums cannot be easily compared except by accessing its value like node.classification.value == 'tethered'.
Thats why we added helper is* methods on the enum:


but ideally this would not be necessary.

abbreviations, id instead of identifier in some places

With Neos 9 we deliberately changed Node::nodeAggregateIdentifier to Node::nodeAggregateId (see #5024)

The rule to consistently use abbreviations originated from when there was not such good code autocompletion and linting in php. Modern day IDE's like phpstorm make it a breeze to work with either of them as you just check whats available.

But this is not true for Fusion and EEL. Here time dials back and many user use the try and error approach. And here it is generally easier to work with if you have a few rules in your head which you know they apply like: No abbreviations.

solutions

introduce a high level API around Nodes

An abstraction on the php side which is more capable and god like. Could, if done consistently, be rewarding. But this comes with a huge overhead. The idea around a TraversableNodeInterface or a NodeAccessor was discarded: #3861
There is just a lot of overhead having to deal possibly with two abstractions in EEL helpers and its hard to stay consistent.

introduce EEL helpers to fetch the information by Node

While EEL helpers can solve the technical problem of getting a NodeType or label for a Node the code is rather verbose and not as easily to remember than to just check which properties a node has.
Additionally the complexity increases when the variable is possible null and cannot be passed to an helper without risking a type error:

${node.nodeType.properties || []}

vs

${node ? Neos.Node.nodeType(nodeType).properties : []}

introduce custom read model layer for eel

As only partially drafted out here neos/flow-development-collection#3153 and discussed in slack we could add a global special translation layer when Nodes are encountered. That will make it possible to add virtual properties accessible like node.nodeType and node.label without the corresponding implementation in the actual php model.

For the current user-base it will be easy (because the will now wonder why this works) but new users might not be able to easily understand what is accessible on a node. By providing a __dir utility which transparently debugs accessible properties we could circumvent this (optional).

summary

It often comes back to that we dont want to design the cr core so it will be best usable in EEL according to ObjectAccess. We need a way to influence and improve this.

@mhsdesign mhsdesign changed the title WIP: Translation layer for Node read-model in Fusion to restore 8.3 sz WIP: Translation layer for Node read-model in Fusion to restore 8.3 syntax Apr 30, 2024
@ahaeslich ahaeslich added the 9.0 label May 3, 2024
@mhsdesign mhsdesign changed the title WIP: Translation layer for Node read-model in Fusion to restore 8.3 syntax Translation layer for Node read-model in Fusion to restore 8.3 syntax May 3, 2024
@mhsdesign
Copy link
Member Author

mhsdesign commented May 10, 2024

Notes from our weekly 2024-05-03

  • Fluid templates using Nodes might certainly become impossible out of the box with Neos 9
  • we might consider renaming node.nodeAggregateId to node.id on the node read model in php
  • idea: use translation layer mechanism mostly to detect legacy
  • idea: if we add a translation layer it should be used only with great care, maybe it can be used to avoid misleading exceptions when rendering VOs, for example: <div id={node.nodeAggregateId}> (alternative <div id={q(node).id()}>)
  • we want to add flow-queries to fetch important value objects that would otherwise need to be manually strigified via .value
    • q(node).id() and or q(node).identifier()
    • q(node).nodeTypeName()
    • → also use in rector
    • possible backport to 8.3?
    • idea: q(node).nodeType() - maybe with special NodeType object return for Fusion use, but rather not as the NodeType is not intended for fusion

Notes from our weekly 2024-05-10

  • we dont want a compatibility layer, but want to throw exceptions when legacy syntax is used.
    • node.nodeType and node.identifier etc. should throw and tell in the exception how to user the alternative.
    • we should make sure that throwing in production is harmless by returning null: node.identifier == null
      • node.identifier in Fusion should also throw in DEV Context (in Prod it should probably render null)
    • implementing by adding a getter like Node::getNodeType has the downside of polluting the core and getting dummy completions via phpstorm. Also it cannot be DEV context aware.
    • Can be implemented as magic in the eel core
    • Discuss if we also want to throw if used wrongly in Fluid and YAML EEL
  • idea: how to make a missing .value cast throw?
    • or discuss magic stringify?
    • → no: not needed because we will have q(node).id() see last weekly, and value object will not be used much

Idea:
Make magic property access _identifier also throw - possibly already backported to Neos with a strict flag.

mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this issue May 12, 2024
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this issue May 12, 2024
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this issue May 12, 2024
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this issue May 12, 2024
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this issue May 12, 2024
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this issue May 13, 2024
neos-bot pushed a commit to neos/contentrepository-nodeaccess that referenced this issue May 13, 2024
neos-bot pushed a commit to neos/contentrepositoryregistry that referenced this issue May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

No branches or pull requests

2 participants