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

Remove tetheredDescendantNodeAggregateIds from CreateNodeAggregateWithNode constructor #4375

Closed
bwaidelich opened this issue Jun 30, 2023 · 4 comments
Assignees
Labels

Comments

@bwaidelich
Copy link
Member

The $tetheredDescendantNodeAggregateIds argument is currently used in order to create deterministic tethered node ids (mostly for tests).

With #4313 ids of tethered nodes will be deterministic so that we can remove that argument for good.
But for this, quite some tests have to be adjusted.

For Behat tests that rely on the child node ids, we could introduce some "makro" in the form tetheredNodeId(<parentNodeId>, <name>) and change tests from

    When the command ChangeNodeAggregateType was published with payload:
      | Key                                | Value                                      |
      | contentStreamId                    | "cs-identifier"                            |
      | nodeAggregateId                    | "nodea-identifier-de"                      |
      | newNodeTypeName                    | "Neos.ContentRepository.Testing:NodeTypeB" |
      | strategy                           | "happypath"                                |
      | tetheredDescendantNodeAggregateIds | { "child-of-type-b": "child-of-type-b-id"} |
    # ...
    And I expect this node to have the following child nodes:
      | Name            | NodeDiscriminator                                   |
      | child-of-type-a | cs-identifier;child-of-type-a-id;{"language":"gsw"} |

to

    When the command ChangeNodeAggregateType was published with payload:
      | Key                                | Value                                      |
      | contentStreamId                    | "cs-identifier"                            |
      | nodeAggregateId                    | "nodea-identifier-de"                      |
      | newNodeTypeName                    | "Neos.ContentRepository.Testing:NodeTypeB" |
      | strategy                           | "happypath"                                |
    # ...
    And I expect this node to have the following child nodes:
      | Name            | NodeDiscriminator                                   |
      | child-of-type-a | cs-identifier;tetheredNodeId("nodea-identifier-de", "child-of-type-b");{"language":"gsw"} |

As a first step we should at least remove the tetheredDescendantNodeAggregateIds argument from the constructor to communicate that this is purely there for internal use.

@nezaniel
Copy link
Member

nezaniel commented Jul 3, 2023

Mmh, I think I'd like to use explicit tethered IDs also in my userland code. I'll check whether the deterministic ones are enough first, though.

@bwaidelich
Copy link
Member Author

We decided, that we first want to make it more explicit that this parameter is internal, e.g. by replacing

    public function __construct(
        // ...
        ?NodeAggregateIdsByNodePaths $tetheredDescendantNodeAggregateIds = null
    ) {

with

    public function __construct(
        // ...
    ) {

    /** @internal bla bla */
    public function  withTetheredDescendantNodeAggregateIds(NodeAggregateIdsByNodePaths $tetheredDescendantNodeAggregateIds): self {
        // ...
    }

because a lot of tests currently rely on the custom tethered node ids.
In the long run, I would still like to get rid of this ambiguity altogether and introduce some helper syntax for Behat tests, so that instead of:

    And the subtree for node aggregate "sir-david-nodenborough" with node types "" and 2 levels deep should be:
      | Level | nodeAggregateId        |
      | 0     | sir-david-nodenborough |
      | 1     | nodewyn-tetherton      |
      | 2     | nodimer-tetherton      |
      | 1     | nody-mc-nodeface       |

one could write something like

    And the subtree for node aggregate "sir-david-nodenborough" with node types "" and 2 levels deep should be:
      | Level | nodeAggregateId                                                                    |
      | 0     | sir-david-nodenborough                                                             |
      | 1     | tetheredNodeId{sir-david-nodenborough.tethered-node}                               |
      | 2     | tetheredNodeId{tetheredNodeId{sir-david-nodenborough.tethered-node}.tethered-leaf} |
      | 1     | nody-mc-nodeface                                                                   |

But we'll have to wait for #4313 anyways

@mhsdesign
Copy link
Member

As discussed in the weekly @nezaniel will always prefer to have an option to set the nodeIds so we should definitely keep a way like withTetheredDescendantNodeAggregateIds (and also possibly make it api?)

@mhsdesign mhsdesign moved this from Todo to Prioritized 🔥 in Neos 9.0 Release Board Sep 1, 2023
bwaidelich added a commit that referenced this issue Sep 8, 2023
@bwaidelich
Copy link
Member Author

Resolved with #4489

@github-project-automation github-project-automation bot moved this from Prioritized 🔥 to Done ✅ in Neos 9.0 Release Board Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

No branches or pull requests

3 participants