!!! FEATURE: Bind `ContentGraph` to workspace

kitsunet commented 5 months ago

The ContentGraph is now always bound to a WorkspaceName and ContentStreamId combination. The internal! ProjectionState of the ContentGraph is now the ContentGraphFinder which allows you to get an instance of the ContentGraph bound to a combination (usually by giving only the WorkspaceName).

The userland way to access the content graph is to ask the content repository:

ContentRepository::getContentGraph(WorkspaceName)

Upgrade instructions

The WorkspaceName must be passed along to get a subgraph instead of the internal ContentStreamId:

- $subgraph = $contentRepository->getContentGraph()->getSubgraph(
-     $workspace->currentContentStreamId,
-     $dimensionSpacePoint,
-     VisibilityConstraints::withoutRestrictions()
- );              
+ $subgraph = $contentRepository->getContentGraph($workspace->workspaceName)->getSubgraph(
+     $dimensionSpacePoint,
+     VisibilityConstraints::withoutRestrictions()
+ );

This pr presents an in between step. Its for example currently not easily possible to get the WorkspaceName of a Node, but that will be solved in a following pr see also https://github.com/neos/neos-development-collection/issues/4564. In this intermediate step, one must ask the workspace finder first to resolve a workspace for content stream which then can be passed to getContentGraph (see how ContentRepositoryRegistry::subgraphForNode handles it internally)

Review instructions

Followups: https://github.com/neos/neos-development-collection/issues/5038 Neos Ui adjustments: https://github.com/neos/neos-ui/pull/3775

kitsunet commented 5 months ago

Ok legacy migration tests fail, I guess changes are needed there, I had done the obvious code adaptions, but probably that's not enough

kitsunet commented 5 months ago

@kitsunet Thanks so much for putting a tremendous amount of time into this. I think it looks great already. I just left some mostly nitpicky comments after a first quick pass mostly to dump my thoughts – no need to fix those (now).

One more important architectural choice is the stateful ContentGraphFinder. If I get it right, it now needs a getInstances() method such that we can access (and reset the state of) all previously created CG instances in order to enable or disable their inMemory cache. And then we need a reset() method to reset the instances for tests.

This is quite a lot of "burden" to an interface (and custom implementations) for some really low-level goals. Maybe together we can come up with a slightly different mechanism to work with those runtime caches. But this is certainly no reason to block this one, we could always tweak it in a follow-up.

Great work!

<3 Thank you!

Yes lets definitely have a look at those low level things, they were really an afterthought since I realized that definitely the tests do not like having the instances around. We could completely skip the cache, but then any cache within the CG or subgraph wouldn't be much use either because you get a fresh instance every time.

bwaidelich commented 4 months ago

@kitsunet I took the freedom to push our changes from yesterday (24cfa06d01200ca7f19d7243b4fab59cb6ebca0a) to this PR. I don't know if tests are still green because there is a conflict that I wasn't able to resolve