Skip to content

Conversation

nezaniel
Copy link
Member

@nezaniel nezaniel commented Jun 17, 2023

This cleans up the handling of root nodes and absolute node paths.

Root nodes don't have names but are to be resolved by type (if not identifier).
Thus, the following adjustments have been made:

  • Uniqueness of node type name for root nodes is now enforced on the write side (threw an exception on the read side until now)
  • The ContentSubgraphInterface has a new method "findRootNodeByType" similar to the ContentGraphInterface
  • Node paths can now be absolute by having a root node type name defined. Absolute paths can be serialized as '/<Neos.ContentRepository.Root>/my/path' with the first path segment being the root node type name
  • Tests for node paths have been written
  • ContentSubgraphInterface::findNodeByPath has been adjusted to accept either the path's root node aggregate type or the starting node aggregate ID
  • ContentSubgraphInterface::retrieveNodePath has been adjusted to return absolute paths with root node aggregate type ContentSubgraph::retrieveNodePath() returns invalid path #4323
  • ContentSubgraphInterface::retrieveNodePath also has been adjusted to fill up "missing" node names with node aggregate IDs ContentSubgraph::retrieveNodePath() when segment has no node name #4331
  • Adjusted the Neos endpoint API to support serialized absolute node paths for the reference editor

Review instructions

Besides the tests, try out a reference editor starting point like '/<Neos.ContentRepository.Root>/neosdemo'

Checklist

  • Code follows the PSR-12 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the 9.0 branch

@github-actions github-actions bot added the 9.0 label Jun 17, 2023
@mhsdesign
Copy link
Member

Should we provide a migration for people using „sites“ and rewrite this to Neos.Neos:Sites?

@nezaniel
Copy link
Member Author

Sure

Should we provide a migration for people using „sites“ and rewrite this to Neos.Neos:Sites?

Sure thing, feel free to do it :)

@nezaniel nezaniel requested review from bwaidelich, skurfuerst, mhsdesign, mficzel and grebaldi and removed request for bwaidelich June 18, 2023 15:25
@nezaniel
Copy link
Member Author

Still missing: UI integration, will follow in a bit

@nezaniel nezaniel changed the title WIP: Serious root nodes 9.0: Serious root nodes Jun 18, 2023
@nezaniel nezaniel requested a review from kitsunet June 18, 2023 19:46
Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some early thoughts ;)

@nezaniel
Copy link
Member Author

I think I covered everything, if the tests pass and there are no more important concerns, I'd like to merge

1 similar comment
@nezaniel
Copy link
Member Author

I think I covered everything, if the tests pass and there are no more important concerns, I'd like to merge

Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets do it ;)

@mficzel
Copy link
Member

mficzel commented Jul 2, 2023

Just tested this and the last commit 61b4d48 introduces an error in the ui whenever a reference error is opened
Could not convert target type "string": Warning: Array to string conversion in /tmp/neos/Development/SubContextddev/Cache/Code/Flow_Object_Classes/Neos_Flow_Property_TypeConverter_StringConverter.php line 114 - Check the logs for details

With the commit before 5b7e06c i could verify:

  • referencing of nodes in the same site (+ rendering links)
  • linking of nodes nodes in other sites (+ rendering links)
  • linking to nodes in totally different root-nodes (taxonomies)

otherwise flow will get confused and throw exceptions that type conversion failed
@mficzel
Copy link
Member

mficzel commented Jul 2, 2023

Node: I could fix the problem with adjusting bringing the parameter comments into the same order as the function parameters. Pushed this change to the branch.

Copy link
Member

@mficzel mficzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by testing .... what i tested:

  • referencing documents with configured starting point in the same site
  • referencing documents with configured starting point in another site
  • referencing other nodes (taxonomies) with a startingpoint other than /<Neos.Neos:Sites>

All three cases above worked and the nodes that were chosen by references editor could be used for traversal and link generation.

What i could not test is linking nodes in another cr which is uncharted territory even with this pr !!!

I personally cannot say that much about coding principles in the new cr other than that the code makes sense to me. So maybe count this as half an approval.

Copy link
Member

@mficzel mficzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed a linting error in ci that obviously should be addressed. Seems to originate from changes made elsewhere since it also appeared in the last commit after running the test again.

@nezaniel
Copy link
Member Author

nezaniel commented Jul 5, 2023

I don't quite understand the linting error and since I cannot reproduce it locally, I'd just ignore it.
As for the test failure, I don't understand it either, and it should be resolved where it was introduced.

Go?

@nezaniel nezaniel requested a review from mficzel July 5, 2023 20:00
@nezaniel nezaniel merged commit e9f3c6d into 9.0 Jul 6, 2023
@nezaniel nezaniel deleted the seriousRootNodes branch July 6, 2023 08:25
@bwaidelich
Copy link
Member

bwaidelich commented Jul 7, 2023

Thanks a lot for this.
Unfortunately this seems to break Neos and neos/neos-ui#3530 does not seem to cover everything
Nevermind, I think it does fix the issue :)

@bwaidelich
Copy link
Member

e.g. I immediately get

Warning: Undefined property: Neos\ContentRepository\Core\Projection\ContentGraph\AbsoluteNodePath::$value in Cache/Code/Flow_Object_Classes/Neos_Neos_Ui_FlowQueryOperations_NeosUiDefaultNodesOperation.php line 88

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ContentSubgraph::retrieveNodePath() when segment has no node name ContentSubgraph::retrieveNodePath() returns invalid path
5 participants