FEATURE: Deprecate `WorkspaceFinder` and introduce new API #5279

mhsdesign · 2024-10-08T16:02:16Z

introduces

ContentRepository::findWorkspaces()
ContentRepository::findWorkspaceByName()

as well as find and filter methods on the Workspaces collection to replace all functionality of the WorkspaceFinder

This is done as preparation for #5096 which will remove the Workspace projection and thus the finder.

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

mhsdesign · 2024-10-08T16:12:41Z

Neos.ContentRepository.Core/Classes/ContentRepository.php

+ return $this->getWorkspaceFinder()->findOneByName($workspaceName);
+ }
+
+ public function getWorkspaces(): Workspaces


todo discuss find vs get here. The philosophy to use get was we dont find - eg filter - for anything.
But when we want to use find to signal the the implementation is probably slow (at least uncached) then we should go with that.

I would suggest not to use find = slow and get = fast as a general concept, because it tries to change the meaning of those words.
In this case we don't "find" any workspaces, we just get them all – at least that was my original reasoning :)

However we decide, some doc comment would be nice

Suggested change

public function getWorkspaces(): Workspaces

/**

* Returns all workspaces of this content repository. To limit the set, {@see Workspaces::find()} and {@see Workspaces::filter()} can be used

*/

public function getWorkspaces(): Workspaces

iirc our convention is

get for what is already there (like in-memory computations, property access)

find for everything that has to be fetched from somewhere

Or in short: yes, we decided to go for get* for fast and find* for slow stuff and I'd rather stick to that.

Im torn in between. getContentGraph for example also does one SQL query to get the currentContentStreamId, though its "get" there ... but im fine with either way.

I couldnt find discuss post to our find* vs get* philosophy but i found a few old artefacts regarding why Node::getParent was renamed to Node::findParent in the old Neos world:

TODO: rename to findParent() because it is a DB operation

and here #2202

adjust TraversableNodeInterface to have proper method namings. Everything which queries something should be called find*; as opposed to get*.

or here #2202 (comment)

$node->findNodePath() sounds weird to me. Why not just getNodePath()?

Because we say: "all operations which are slow should be named find*", vs. "all operations which operate on already-loaded data should be named get*". And fetching a node path is a potentially costly operation.

And in findParent we also definitely dont filter anything still it continues to be named Subgraph::findParentNode.

Though there is one exception Subgraph::findNodePath was eventually renamed to Subgraph::retrieveNodePath with #4090
So it seems retrieveParent or retrieveWorkspaces would also be possibly valid when starting out fresh.

But for now for consistency i guess its a good idea to stick the 6year old philosophy and name it findWorkspaces instead (especially we could consider adding a filter later:)).

Just to avoid confusion: My point was that this kind of "magic meaning" makes sense for a single API, Content(Sub)Graph in this case. I would suggest not to adapt in in general if it leads to weird method names.
But I'm fine with either solution in this case!!

I really find the distinction between find* and get* hard to grasp, and keep track of. Internals might and can change such that the meaning of find/get is no longer correct, and we cannot change the public API according to our internals. I think this warrants discussion, as I struggled with this naming before as well...

bwaidelich

Thanks! +1 by reading

bwaidelich · 2024-10-09T07:57:27Z

Neos.ContentRepository.Core/Classes/ContentRepository.php

@@ -247,6 +244,19 @@ public function getContentGraph(WorkspaceName $workspaceName): ContentGraphInter
 return $this->projectionState(ContentGraphFinder::class)->getByWorkspaceName($workspaceName);
 }

+ public function findWorkspaceByName(WorkspaceName $workspaceName): ?Workspace


Some doc comment would be nice, e.g:

Suggested change

public function findWorkspaceByName(WorkspaceName $workspaceName): ?Workspace

/**

* Returns the workspace with the given name, or NULL if it does not exist in this content repository

*/

public function findWorkspaceByName(WorkspaceName $workspaceName): ?Workspace

bwaidelich · 2024-10-09T08:01:50Z

Neos.ContentRepository.Core/Classes/ContentRepository.php

+ return $this->getWorkspaceFinder()->findOneByName($workspaceName);
+ }
+
+ public function getWorkspaces(): Workspaces


I would suggest not to use find = slow and get = fast as a general concept, because it tries to change the meaning of those words.
In this case we don't "find" any workspaces, we just get them all – at least that was my original reasoning :)

However we decide, some doc comment would be nice

Suggested change

public function getWorkspaces(): Workspaces

/**

* Returns all workspaces of this content repository. To limit the set, {@see Workspaces::find()} and {@see Workspaces::filter()} can be used

*/

public function getWorkspaces(): Workspaces

dlubitz · 2024-10-09T09:41:23Z

Looks good to me, but tests are failing.

Based on neos#5146 Required to ease migration to neos#5096

neos@db7861f

> "all operations which are slow should be named find*", vs. "all operations which operate on already-loaded data should be named get*".

The review comments were not applied.

mhsdesign · 2024-10-10T17:33:34Z

Pushed the comment suggestions on 9.0

> "all operations which are slow should be named find*", vs. "all operations which operate on already-loaded data should be named get*". see #5279 (comment)

mhsdesign changed the base branch from 9.0 to feature/4726-extract-workspace-metadata-and-user-assignment-to-neos-wip October 8, 2024 16:02

mhsdesign changed the base branch from feature/4726-extract-workspace-metadata-and-user-assignment-to-neos-wip to feature/4726-extract-workspace-metadata-and-user-assignment-to-neos October 8, 2024 16:02

github-actions bot added Feature 9.0 and removed 9.0 labels Oct 8, 2024

mhsdesign commented Oct 8, 2024

View reviewed changes

bwaidelich approved these changes Oct 9, 2024

View reviewed changes

mhsdesign deleted the branch neos:9.0 October 9, 2024 14:24

mhsdesign closed this Oct 9, 2024

mhsdesign reopened this Oct 9, 2024

mhsdesign changed the base branch from feature/4726-extract-workspace-metadata-and-user-assignment-to-neos to 9.0 October 9, 2024 14:30

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

TASK: Deprecate WorkspaceFinder and introduce forward-compatible API

f15eabc

Based on neos#5146 Required to ease migration to neos#5096

mhsdesign force-pushed the task/deprecate--WorkspaceFinder branch from 37734dc to f15eabc Compare October 10, 2024 08:27

mhsdesign added 2 commits October 10, 2024 10:28

TASK: Update Workspaces utilities according to neos#5096

c0528e7

neos@db7861f

TASK: Adjust to find* vs get* convention

6a84332

> "all operations which are slow should be named find*", vs. "all operations which operate on already-loaded data should be named get*".

kitsunet approved these changes Oct 10, 2024

View reviewed changes

kitsunet merged commit bc25ec5 into neos:9.0 Oct 10, 2024
9 checks passed

mhsdesign deleted the task/deprecate--WorkspaceFinder branch October 10, 2024 17:29

mhsdesign added a commit that referenced this pull request Oct 10, 2024

TASK: Add missing doc comments #5279

98a0fb2

The review comments were not applied.

mhsdesign added a commit that referenced this pull request Oct 10, 2024

TASK: Adjust to find* vs get* convention

c34456b

> "all operations which are slow should be named find*", vs. "all operations which operate on already-loaded data should be named get*". see #5279 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEATURE: Deprecate `WorkspaceFinder` and introduce new API #5279

FEATURE: Deprecate `WorkspaceFinder` and introduce new API #5279

mhsdesign commented Oct 8, 2024 •

edited

Loading

mhsdesign Oct 8, 2024

bwaidelich Oct 9, 2024

nezaniel Oct 9, 2024

mhsdesign Oct 9, 2024

mhsdesign Oct 10, 2024

bwaidelich Oct 10, 2024 •

edited

Loading

kitsunet Oct 10, 2024

bwaidelich left a comment

bwaidelich Oct 9, 2024

bwaidelich Oct 9, 2024

dlubitz commented Oct 9, 2024

mhsdesign commented Oct 10, 2024

FEATURE: Deprecate WorkspaceFinder and introduce new API #5279

FEATURE: Deprecate WorkspaceFinder and introduce new API #5279

Conversation

mhsdesign commented Oct 8, 2024 • edited Loading

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

bwaidelich Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

dlubitz commented Oct 9, 2024

mhsdesign commented Oct 10, 2024

FEATURE: Deprecate `WorkspaceFinder` and introduce new API #5279

FEATURE: Deprecate `WorkspaceFinder` and introduce new API #5279

mhsdesign commented Oct 8, 2024 •

edited

Loading

bwaidelich Oct 10, 2024 •

edited

Loading