TASK Deprecate overhauled temporary FlowQueries and use node field access instead #5277

mhsdesign · 2024-10-08T15:14:25Z

with #5241 value object were made string-able, so they are simply usable in fusion after all.

Previously flowquery operations were introduced to not have to do the manual string cast via .value in node.nodeTypeName.value and similar.

The label flowquery operation was introduced in the same run but now we reconsidered that flowquery should mostly be used for traversal and the only real final operation is get().

For this reason we also discourage the property operation now. The negative performance aspect of 8.3 of needlesly resolving all reference nodes by calling node.properties.title is dated as now only real properties are ever in the result set.

Upgrade instructions

Review instructions

Checklist

Code follows the PSR-2 coding style
Tests have been created, run and adjusted as needed
The PR is created against the lowest maintained branch
Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
Reviewer - The first section explains the change briefly for change-logs
Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

This reverts commit 2a26722

…query operations with neos#5241 value object were made string-able, so they are simply usable in fusion after all. Previously flowquery operations were introduced to not have to do the manual string cast via `.value` in `node.nodeTypeName.value` and similar. The label flowquery operation was introduced in the same run but now we reconsidered that flowquery should mostly be used for traversal and the only real final operation is get(). For this reason we also discourage the property operation now. The negative performance aspect of 8.3 of needlesly resolving all reference nodes by calling `node.properties.title` is dated as now only real properties are ever in the result set.

bwaidelich

+1 by reading, thanks for taking care.

Just some comment regarding the deprecated Flow operations:
Will the @deprecated annotations in the PHP implementation really help to prevent developers/integrators from using them?
What is our phasing out strategy?

If we keep the @deprecated annotations, we should maybe add the version, i.e. @deprecated with 9.0.0-beta14...

bwaidelich · 2024-10-09T08:05:29Z

Neos.ContentRepository.NodeAccess/Classes/FlowQueryOperations/IdOperation.php

@@ -21,6 +21,7 @@

 /**
 * Used to access the Node's identifier of a ContentRepository Node.
+ * @deprecated please use ${node.aggregateId} instead


How do we deal with those deprecations – AFAIS the user won't notice them.
Wouldn't it make more sense to just remove the operation so they do?
Or what are the plans for a removal?

haha thing is this was praised by us to BE THE way in beta10 https://discuss.neos.io/t/neos-9-beta-10-release/6578 and beta11 again https://discuss.neos.io/t/neos-9-beta-11-release/6618 so i dont know if we can just remove them again 😅 . This is a classic EIGENTOR. And for what i think we dont need to remove i before the beta as a dead fq doesnt hurt.

Neos.Neos/Classes/Fusion/Helper/NodeHelper.php

…-access-instead-of-flowquery

mhsdesign · 2024-10-13T08:02:03Z

Neos.Neos/Classes/Fusion/Helper/NodeHelper.php

+ /**
+ * Renders the actual node label based on the NodeType definition in Fusion.
+ */
+ public function getLabel(Node $node): string


it might be unlucky that the Neos.Node is also used as helper to generate the actual label in yaml. See NodeHelper::labelForNode above. Like:

'Vendor.Site:MyContent': label: "${Neos.Node.labelForNode(node).prefix('foo')}"

so its unfortunate that the naming is not distinct and one could easily create a recursion here. Though that was also possible in 8.3 by just writing label: "${node.label}"

As an outlook i would propose to deprecate Neos.Node for the yaml case at some point and then introduce a more specialised Label.createForNode helper or something that is only to be used in YAML. Having the capabilities to access the "NodeLabelToken" through fusion is not the desired usecase as far as i can tell.

Thats why i named the method getLabel in the first place and not just label as the ambiguity would be even bigger. Though Neos.Node.label(node) would fit better with the semantics Neos.Node.depth(node) and path so we have to decide on that!

Separating the helper for YAML makes sense to me. I would probably start that transition ASAP by creating the new helper and leaving labelForNode here deprecated just calling the other one. And then really just using label(node) as method name for consistency.

So you want to unify them into one?

I want to split them as they are separate usecases: f22d740

The labelForNode feature was 'just' introduced in #3328 and i consider it safe that it was not really used in fusion so i have no b/c layer in place.

well as its Neos.Neos.label i might have messed up the Neos.Node.getNodeType earlier so i created a sister pr #5287

kitsunet

See one comment otherwise this could go in.

Sebobo

Looks good by reading and I love the change ❤️

Sebobo · 2024-10-14T09:35:31Z

Neos.Neos/Configuration/Settings.yaml

@@ -422,7 +422,7 @@ Neos:
 Json: Neos\Eel\Helper\JsonHelper
 I18n: Neos\Flow\I18n\EelHelper\TranslationHelper
 q: Neos\Eel\FlowQuery\FlowQuery::q
- Neos.Node: Neos\Neos\Fusion\Helper\NodeHelper
+ Neos.Node: Neos\Neos\Fusion\Helper\NodeLabelHelper


This doesn't look right to me, depending on the context, the same helper name are two different classes, that do similar stuff?

jup its to say a litte dirty at least. The NodeHelper contained in Neos 8.3 just 3 methods but in Neos 9 now a whole lot more (and also the label method which would lead to a recursion)
Alternatively we could introduce a new namespace like Neos.NodeLabel.labelForNode or labelForNode(node) as function, or NodeLabel.labelForNode while probably still keeping the proposed Neos.Node: Neos\Neos\Fusion\Helper\NodeLabelHelper as legacy layer.

But i thought its not really worth the effort. Thats why the documentation in NodeLabelHelper also explains its not the same as the other. Whats your idea.

No this is confusing as hell, please lets not do this. Also what if I want to actually use one of the Neos.Node functions in my label

Jup agreed. Hmm especially here:

neos-development-collection/Neos.Neos/Classes/Domain/NodeLabel/ExpressionBasedNodeLabelGenerator.php

Line 44 in 6bbc133

${(Neos.Node.getNodeType(node).label || node.nodeTypeName.value) + (node.nodeName ? ' (' + node.nodeName.value + ')' : '')}

We actually need the node type in the yaml expression ...

so i guess there are two ways from here:

introduce a new NodeLabelHelper an register it under Neos.NodeLabel or something
keep the Neos.Node helper registered in yaml but deprecate its method labelForNode

Keep everything in NodeHelper and just rename labelForNode

3

Keep labelForNode and label side by side which is a little confusing but maybe okay?

we decided on 3

This reverts commit f22d740.

…-access-instead-of-flowquery

mhsdesign added 3 commits October 8, 2024 11:05

TASK: Replace q(node).label() with Neos.Node.getLabel

e688caa

This reverts commit 2a26722

TASK: Remove needless .value cast in Fusion

77f6c62

github-actions bot added the 9.0 label Oct 8, 2024

mhsdesign changed the title ~~TASK Prefer node field access instead of flowquery~~ TASK Deprecate overhauled temporary FlowQueries and use node field access instead Oct 8, 2024

bwaidelich approved these changes Oct 9, 2024

View reviewed changes

mhsdesign added 3 commits October 11, 2024 13:16

Merge remote-tracking branch 'origin/9.0' into task/prefer-node-field…

357d151

…-access-instead-of-flowquery

TASK: Adjust deprecation notice

5744d43

Merge remote-tracking branch 'origin/9.0' into task/prefer-node-field…

20df63d

…-access-instead-of-flowquery

mhsdesign commented Oct 13, 2024

View reviewed changes

mhsdesign requested review from kitsunet and Sebobo October 13, 2024 15:31

kitsunet approved these changes Oct 14, 2024

View reviewed changes

Sebobo approved these changes Oct 14, 2024

View reviewed changes

mhsdesign added 2 commits October 14, 2024 09:40

TASK: Split labelForNode out of main Neos.Node helper

f22d740

TASK: Rename Neos.Node.getLabel to Neos.Node.label for consistency

9584511

mhsdesign mentioned this pull request Oct 14, 2024

TASK: Replace node.nodeTypeName.value with q(node).nodeTypeName() #5171

Closed

6 tasks

Sebobo reviewed Oct 14, 2024

View reviewed changes

mhsdesign added 4 commits October 14, 2024 13:43

Revert "TASK: Split labelForNode out of main Neos.Node helper"

69506ca

This reverts commit f22d740.

Merge remote-tracking branch 'origin/9.0' into task/prefer-node-field…

e033f1a

…-access-instead-of-flowquery

TASK: Describe that labelForNode is not confused with label

bd8f62f

TASK: Adjust to use Neos.Node.nodeType(node)

2f0cdb3

mhsdesign merged commit 41de09a into neos:9.0 Oct 14, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TASK Deprecate overhauled temporary FlowQueries and use node field access instead #5277

TASK Deprecate overhauled temporary FlowQueries and use node field access instead #5277

mhsdesign commented Oct 8, 2024 •

edited

Loading

bwaidelich left a comment

bwaidelich Oct 9, 2024

mhsdesign Oct 9, 2024

mhsdesign Oct 13, 2024

kitsunet Oct 14, 2024

Sebobo Oct 14, 2024

mhsdesign Oct 14, 2024

mhsdesign Oct 14, 2024

kitsunet left a comment

Sebobo left a comment

Sebobo Oct 14, 2024 •

edited

Loading

mhsdesign Oct 14, 2024

kitsunet Oct 14, 2024 •

edited

Loading

mhsdesign Oct 14, 2024

mhsdesign Oct 14, 2024

TASK Deprecate overhauled temporary FlowQueries and use node field access instead #5277

TASK Deprecate overhauled temporary FlowQueries and use node field access instead #5277

Conversation

mhsdesign commented Oct 8, 2024 • edited Loading

bwaidelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitsunet left a comment

Choose a reason for hiding this comment

Sebobo left a comment

Choose a reason for hiding this comment

Sebobo Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitsunet Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsdesign commented Oct 8, 2024 •

edited

Loading

Sebobo Oct 14, 2024 •

edited

Loading

kitsunet Oct 14, 2024 •

edited

Loading