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.

Sign up for GitHub

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

FEATURE: Deprecate WorkspaceFinder and introduce new API #5279

Merged
merged 3 commits into from
Oct 10, 2024
Merged

FEATURE: Deprecate WorkspaceFinder and introduce new API #5279

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f15eabc
TASK: Deprecate `WorkspaceFinder` and introduce forward-compatible API
mhsdesign Oct 8, 2024
c0528e7
TASK: Update `Workspaces` utilities according to #5096
mhsdesign Oct 10, 2024
6a84332
TASK: Adjust to `find*` vs `get*` convention
mhsdesign Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions Neos.ContentRepository.Core/Classes/ContentRepository.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@
use Neos\ContentRepository\Core\Projection\ProjectionStateInterface;
use Neos\ContentRepository\Core\Projection\ProjectionStatuses;
use Neos\ContentRepository\Core\Projection\WithMarkStaleInterface;
use Neos\ContentRepository\Core\Projection\Workspace\Workspace;
use Neos\ContentRepository\Core\Projection\Workspace\WorkspaceFinder;
use Neos\ContentRepository\Core\Projection\Workspace\Workspaces;
use Neos\ContentRepository\Core\SharedModel\ContentRepository\ContentRepositoryId;
use Neos\ContentRepository\Core\SharedModel\ContentRepository\ContentRepositoryStatus;
use Neos\ContentRepository\Core\SharedModel\Exception\WorkspaceDoesNotExist;
Expand Down Expand Up @@ -234,11 +236,6 @@ public function resetProjectionState(string $projectionClassName): void
$projection->reset();
}

public function getNodeTypeManager(): NodeTypeManager
{
return $this->nodeTypeManager;
}

/**
* @throws WorkspaceDoesNotExist if the workspace does not exist
*/
Expand All @@ -247,6 +244,19 @@ public function getContentGraph(WorkspaceName $workspaceName): ContentGraphInter
return $this->projectionState(ContentGraphFinder::class)->getByWorkspaceName($workspaceName);
}

public function findWorkspaceByName(WorkspaceName $workspaceName): ?Workspace
mhsdesign marked this conversation as resolved.
Show resolved Hide resolved
{
return $this->getWorkspaceFinder()->findOneByName($workspaceName);
}

public function getWorkspaces(): Workspaces
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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:)).

Copy link
Member

@bwaidelich bwaidelich Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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!!

Copy link
Member

Choose a reason for hiding this comment

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

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()->findAll();
}

/**
* @deprecated with 9.0.0-beta14 please use {@see ContentRepository::getWorkspaces()} and {@see ContentRepository::findWorkspaceByName()} instead.
*/
public function getWorkspaceFinder(): WorkspaceFinder
{
return $this->projectionState(WorkspaceFinder::class);
Expand All @@ -257,6 +267,11 @@ public function getContentStreamFinder(): ContentStreamFinder
return $this->projectionState(ContentStreamFinder::class);
}

public function getNodeTypeManager(): NodeTypeManager
{
return $this->nodeTypeManager;
}

public function getVariationGraph(): InterDimensionalVariationGraph
{
return $this->variationGraph;
Expand Down
5 changes: 5 additions & 0 deletions Neos.ContentRepository.Core/Classes/Projection/Workspace/Workspace.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,4 +129,9 @@ public function isPublicWorkspace(): bool
{
return $this->baseWorkspaceName === null && $this->workspaceOwner === null;
}

public function isRootWorkspace(): bool
{
return $this->baseWorkspaceName !== null;
}
}
31 changes: 28 additions & 3 deletions Neos.ContentRepository.Core/Classes/Projection/Workspace/WorkspaceFinder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,10 @@
use Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceTitle;

/**
* Workspace Finder
* The legacy Workspace Finder
*
* @api
* @deprecated with 9.0.0-beta14 please use {@see ContentRepository::getWorkspaces()} and {@see ContentRepository::findWorkspaceByName()} instead.
* @internal
*/
final class WorkspaceFinder implements ProjectionStateInterface
{
Expand All @@ -36,7 +37,9 @@ public function __construct(
) {
}


/**
* @deprecated with 9.0.0-beta14 please use {@see ContentRepository::findWorkspaceByName()} instead
*/
public function findOneByName(WorkspaceName $name): ?Workspace
{
$workspace = $this->workspaceRuntimeCache->getWorkspaceByName($name);
Expand All @@ -63,6 +66,15 @@ public function findOneByName(WorkspaceName $name): ?Workspace
return $workspace;
}

/**
* @deprecated with 9.0.0-beta14 discouraged. You should just operate on workspace names instead.
* To still archive the functionality please use {@see ContentRepository::getWorkspaces()} instead and filter the result:
*
* $this->contentRepository->getWorkspaces()->find(
* fn (Workspace $workspace) => $workspace->currentContentStreamId->equals($contentStreamId)
* )
*
*/
public function findOneByCurrentContentStreamId(
ContentStreamId $contentStreamId
): ?Workspace {
Expand Down Expand Up @@ -91,6 +103,7 @@ public function findOneByCurrentContentStreamId(
}

/**
* @deprecated with 9.0.0-beta14 please use {@see ContentRepository::getWorkspaces()} and {@see Workspaces::getBaseWorkspaces()} instead.
* @return array<string,Workspace>
* @throws DBALException
*/
Expand All @@ -116,6 +129,9 @@ public function findByBaseWorkspace(WorkspaceName $baseWorkspace): array
return $result;
}

/**
* @deprecated with 9.0.0-beta14 owners/collaborators should be assigned to workspaces outside the Content Repository core
*/
public function findOneByWorkspaceOwner(string $owner): ?Workspace
{
$workspaceRow = $this->dbal->executeQuery(
Expand All @@ -135,6 +151,9 @@ public function findOneByWorkspaceOwner(string $owner): ?Workspace
return $this->createWorkspaceFromDatabaseRow($workspaceRow);
}

/**
* @deprecated with 9.0.0-beta14 please use {@see ContentRepository::getWorkspaces()} instead
*/
public function findAll(): Workspaces
{
$result = [];
Expand All @@ -154,6 +173,12 @@ public function findAll(): Workspaces
}

/**
* @deprecated with 9.0.0-beta14 please use {@see ContentRepository::getWorkspaces()} instead and filter the result:
*
* $this->contentRepository->getWorkspaces()->filter(
* fn (Workspace $workspace) => $workspace->status === WorkspaceStatus::OUTDATED
* )
*
* @return array<string,Workspace>
* @throws \Doctrine\DBAL\Driver\Exception
* @throws DBALException
Expand Down
26 changes: 26 additions & 0 deletions Neos.ContentRepository.Core/Classes/Projection/Workspace/Workspaces.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,34 @@ public function getIterator(): \Traversable
yield from array_values($this->workspaces);
}

/**
* @param \Closure(Workspace): bool $callback
*/
public function filter(\Closure $callback): self
{
return new self(array_filter($this->workspaces, $callback));
}

/**
* @param \Closure(Workspace): bool $callback
*/
public function find(\Closure $callback): ?Workspace
{
foreach ($this->workspaces as $workspace) {
if ($callback($workspace)) {
return $workspace;
}
}
return null;
}

public function count(): int
{
return count($this->workspaces);
}

public function isEmpty(): bool
{
return $this->workspaces === [];
}
}
Loading