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

TASK: Pass ContentStreamId to NodeFactory #4941

Merged
merged 2 commits into from
Mar 15, 2024

Conversation

dlubitz
Copy link
Contributor

@dlubitz dlubitz commented Mar 14, 2024

No description provided.

@dlubitz dlubitz requested a review from mhsdesign March 14, 2024 08:59
@dlubitz dlubitz self-assigned this Mar 14, 2024
Copy link
Member

@ahaeslich ahaeslich left a comment

Choose a reason for hiding this comment

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

by reading 👀

@kitsunet
Copy link
Member

Why is this necessary?

@dlubitz
Copy link
Contributor Author

dlubitz commented Mar 14, 2024

@mhsdesign requested that as preparation for the node identity.

@kitsunet
Copy link
Member

Mmmm, @mhsdesign gimme some more background. Shouldn't we resolve this some other place before hitting this?

@mhsdesign
Copy link
Member

@kitsunet Thats what we discussed in the weekly from the 01-03-2024.

  1. NodeFactory::mapNodeRowToNode must accept WorkspaceName and ContentStreamId from outside.
  2. returned array $nodeRow does not need to contain $csId as this is part of the query (optional)

@mhsdesign
Copy link
Member

And its good to see that all tests pass, because that means we never fetch foreign nodes in a subgraph.
The question is if we should do also 2, to remove the returned csStreamId field from the node data to reduce transferred things?

@mhsdesign mhsdesign marked this pull request as draft March 15, 2024 14:05
@mhsdesign
Copy link
Member

Bastians subtree tag pr goes first.

And yes we discussed that we can remove the csStreamId from the selects, which might even be noticeable for recursive queries.

@mhsdesign mhsdesign marked this pull request as ready for review March 15, 2024 14:47
@mhsdesign mhsdesign force-pushed the task/pass-contentstreamid-to-nodefactory branch from 08794f0 to 0deca11 Compare March 15, 2024 15:12
Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

+1 by reading

@ahaeslich ahaeslich merged commit 7d7a44b into neos:9.0 Mar 15, 2024
9 checks passed
@mhsdesign mhsdesign mentioned this pull request Mar 19, 2024
35 tasks
@dlubitz dlubitz deleted the task/pass-contentstreamid-to-nodefactory branch May 10, 2024 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants