FEATURE: Deprecate WorkspaceFinder and introduce new API
#5279
Conversation
mhsdesign
changed the base branch from
9.0
to
feature/4726-extract-workspace-metadata-and-user-assignment-to-neos-wip
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
| return $this->getWorkspaceFinder()->findOneByName($workspaceName); | ||
| } | ||
|
|
||
| public function getWorkspaces(): Workspaces |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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 justgetNodePath()?
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:)).
There was a problem hiding this comment.
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!!
There was a problem hiding this comment.
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...
| return $this->getWorkspaceFinder()->findOneByName($workspaceName); | ||
| } | ||
|
|
||
| public function getWorkspaces(): Workspaces |
There was a problem hiding this comment.
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
| 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 |
|
Looks good to me, but tests are failing. |
mhsdesign
changed the base branch from
feature/4726-extract-workspace-metadata-and-user-assignment-to-neos
to
9.0
mhsdesign
force-pushed
the
task/deprecate--WorkspaceFinder
branch
from
37734dc to
f15eabc
Compare
> "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.
|
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)
> "all operations which are slow should be named find*", vs. "all operations which operate on already-loaded data should be named get*". see neos/neos-development-collection#5279 (comment)
> "all operations which are slow should be named find*", vs. "all operations which operate on already-loaded data should be named get*". see neos/neos-development-collection#5279 (comment)
> "all operations which are slow should be named find*", vs. "all operations which operate on already-loaded data should be named get*". see neos/neos-development-collection#5279 (comment)
> "all operations which are slow should be named find*", vs. "all operations which operate on already-loaded data should be named get*". see neos/neos-development-collection#5279 (comment)
introduces
ContentRepository::findWorkspaces()ContentRepository::findWorkspaceByName()as well as
findandfiltermethods on the Workspaces collection to replace all functionality of theWorkspaceFinderThis is done as preparation for #5096 which will remove the Workspace projection and thus the finder.
Upgrade instructions
Review instructions
Checklist
FEATURE|TASK|BUGFIX!!!and have upgrade-instructions