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: Refactor Node property mapper to use new NodeAddress #5068

Merged
merged 5 commits into from
Jul 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions Neos.Neos/Classes/Controller/Backend/ContentController.php
Original file line number Diff line number Diff line change
Expand Up @@ -150,14 +150,15 @@ public function uploadAssetAction(Asset $asset, string $metadata, string $node,
$contentRepositoryId = SiteDetectionResult::fromRequest($this->request->getHttpRequest())
->contentRepositoryId;
$contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId);
$nodeAddress = NodeAddressFactory::create($contentRepository)->createFromUriString($nodeAddressString);
// todo legacy uri node address notation used. Should be refactored to use json encoded NodeAddress
Copy link
Member

Choose a reason for hiding this comment

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

The TODO is related to line 149, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

jip a followup will change the whole neos ui and neos.neos to work on the new format we agreed upon.

$nodeAddress = NodeAddressFactory::create($contentRepository)->createCoreNodeAddressFromLegacyUriString($nodeAddressString);

$node = $contentRepository->getContentGraph($nodeAddress->workspaceName)
->getSubgraph(
$nodeAddress->dimensionSpacePoint,
VisibilityConstraints::withoutRestrictions()
)
->findNodeById($nodeAddress->nodeAggregateId);
->findNodeById($nodeAddress->aggregateId);


$this->response->setContentType('application/json');
Expand Down
5 changes: 3 additions & 2 deletions Neos.Neos/Classes/Controller/Service/NodesController.php
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,9 @@ public function indexAction(
: null;
$nodeAddress = null;
if (!$nodePath) {
// todo legacy uri node address notation used. Should be refactored to use json encoded NodeAddress
$nodeAddress = $contextNode
? NodeAddressFactory::create($contentRepository)->createFromUriString($contextNode)
? NodeAddressFactory::create($contentRepository)->createCoreNodeAddressFromLegacyUriString($contextNode)
: null;
}

Expand All @@ -141,7 +142,7 @@ public function indexAction(

if ($nodeIds === [] && (!is_null($nodeAddress) || !is_null($nodePath))) {
if (!is_null($nodeAddress)) {
$entryNode = $subgraph->findNodeById($nodeAddress->nodeAggregateId);
$entryNode = $subgraph->findNodeById($nodeAddress->aggregateId);
} else {
/** @var AbsoluteNodePath $nodePath */
$entryNode = $subgraph->findNodeByAbsolutePath($nodePath);
Expand Down
21 changes: 3 additions & 18 deletions Neos.Neos/Classes/FrontendRouting/NodeAddress.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,6 @@
use Neos\Flow\Annotations as Flow;

/**
* A persistent, external "address" of a node; used to link to it.
*
* Describes the intention of the user making the current request:
* Show me
* node $nodeAggregateId
* in dimensions $dimensionSpacePoint
* in contentStreamId $contentStreamId
*
* It is used in Neos Routing to build a URI to a node.
*
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove those?

Copy link
Member Author

Choose a reason for hiding this comment

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

because its just @deprecated and will be removed soon.
The statements are not true anymore: It is used in Neos Routing to build a URI to a node.

Copy link
Member

Choose a reason for hiding this comment

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

Right, thanks for clarifying, I briefly confused the two NodeAddresses.. Good that we are getting rid of one :)

* @deprecated will be removed before Final 9.0
* The NodeAddress was added 6 years ago without the concept of multiple crs
* Its usages will be replaced by the new node attached node address
Expand All @@ -42,8 +32,9 @@
/**
* @internal use NodeAddressFactory, if you want to create a NodeAddress
*/
/** @phpstan-ignore-next-line its all just temporary */
Copy link
Member

Choose a reason for hiding this comment

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

Could you elaborate the comment a bit? Why do we need it?

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 dindt want to remove the param yet as it would break the neos ui now (we construct it over there) this is just intermediate

public function __construct(
public ContentStreamId $contentStreamId,
?ContentStreamId $_contentStreamId,
Copy link
Member

Choose a reason for hiding this comment

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

I assume, you just kept this to keep the PR smaller? But is it necessary to rename the parameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

its just a dummy stub, and _ indicates its unused. the class will be obliterated into nothing.

public DimensionSpacePoint $dimensionSpacePoint,
public NodeAggregateId $nodeAggregateId,
public WorkspaceName $workspaceName
Expand All @@ -59,16 +50,10 @@ public function serializeForUri(): string
. '__' . $this->nodeAggregateId->value;
}

public function isInLiveWorkspace(): bool
{
return $this->workspaceName->isLive();
}

public function __toString(): string
{
return sprintf(
'NodeAddress[contentStream=%s, dimensionSpacePoint=%s, nodeAggregateId=%s, workspaceName=%s]',
$this->contentStreamId->value,
'NodeAddress[dimensionSpacePoint=%s, nodeAggregateId=%s, workspaceName=%s]',
Copy link
Member

Choose a reason for hiding this comment

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

I hope that we don' relay on this string representation (=> do we really need it, what about using __debugInfo() instead?) – and if we keep it: Why did you decide to move the workspace name to the end?

Copy link
Member Author

Choose a reason for hiding this comment

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

we probably dont even need the __toString anymore at this time ... but i kept it ... it will all go away. Its legacy.

$this->dimensionSpacePoint->toJson(),
$this->nodeAggregateId->value,
$this->workspaceName->value
Expand Down
40 changes: 4 additions & 36 deletions Neos.Neos/Classes/FrontendRouting/NodeAddressFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
use Neos\ContentRepository\Core\Projection\ContentGraph\Node;
use Neos\ContentRepository\Core\SharedModel\Node\NodeAggregateId;
use Neos\ContentRepository\Core\SharedModel\Node\NodeAddress;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId;
use Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceName;

/**
Expand All @@ -38,35 +37,13 @@ public static function create(ContentRepository $contentRepository): self
return new self($contentRepository);
}

public function createFromContentStreamIdAndDimensionSpacePointAndNodeAggregateId(
ContentStreamId $contentStreamId,
DimensionSpacePoint $dimensionSpacePoint,
NodeAggregateId $nodeAggregateId
): LegacyNodeAddress {
$workspace = $this->contentRepository->getWorkspaceFinder()->findOneByCurrentContentStreamId(
$contentStreamId
);
if ($workspace === null) {
throw new \RuntimeException(
'Cannot build a NodeAddress for traversable node of aggregate ' . $nodeAggregateId->value
. ', because the content stream ' . $contentStreamId->value
. ' is not assigned to a workspace.'
);
}
return new LegacyNodeAddress(
$contentStreamId,
$dimensionSpacePoint,
$nodeAggregateId,
$workspace->workspaceName,
);
}

public function createFromNode(Node $node): LegacyNodeAddress
{
return $this->createFromContentStreamIdAndDimensionSpacePointAndNodeAggregateId(
$node->subgraphIdentity->contentStreamId,
return new LegacyNodeAddress(
null,
$node->dimensionSpacePoint,
$node->aggregateId,
$node->workspaceName,
);
}

Expand All @@ -93,17 +70,8 @@ public function createFromUriString(string $serializedNodeAddress): LegacyNodeAd
$dimensionSpacePoint = DimensionSpacePoint::fromArray(json_decode(base64_decode($dimensionSpacePointSerialized), true));
$nodeAggregateId = NodeAggregateId::fromString($nodeAggregateIdSerialized);

$contentStreamId = $this->contentRepository->getWorkspaceFinder()->findOneByName($workspaceName)
?->currentContentStreamId;
if (is_null($contentStreamId)) {
throw new \InvalidArgumentException(
'Could not resolve content stream identifier for node address ' . $serializedNodeAddress,
1645363784
);
}

return new LegacyNodeAddress(
$contentStreamId,
null,
$dimensionSpacePoint,
$nodeAggregateId,
$workspaceName
Expand Down
9 changes: 2 additions & 7 deletions Neos.Neos/Classes/Fusion/Helper/NodeHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,12 @@
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\FindAncestorNodesFilter;
use Neos\ContentRepository\Core\Projection\ContentGraph\Node;
use Neos\ContentRepository\Core\Projection\ContentGraph\NodePath;
use Neos\ContentRepository\Core\Projection\NodeHiddenState\NodeHiddenStateFinder;
use Neos\ContentRepository\Core\SharedModel\Node\NodeAddress;
use Neos\ContentRepositoryRegistry\ContentRepositoryRegistry;
use Neos\Eel\ProtectedContextAwareInterface;
use Neos\Flow\Annotations as Flow;
use Neos\Neos\Domain\Exception;
use Neos\Neos\Domain\Service\NodeTypeNameFactory;
use Neos\Neos\FrontendRouting\NodeAddressFactory;
use Neos\Neos\Presentation\VisualNodePath;
use Neos\Neos\Utility\NodeTypeWithFallbackProvider;

Expand Down Expand Up @@ -155,11 +154,7 @@ public function isNodeTypeExistent(Node $node): bool

public function serializedNodeAddress(Node $node): string
{
$contentRepository = $this->contentRepositoryRegistry->get(
$node->contentRepositoryId
);
$nodeAddressFactory = NodeAddressFactory::create($contentRepository);
return $nodeAddressFactory->createFromNode($node)->serializeForUri();
return NodeAddress::fromNode($node)->toJson();
}

public function subgraphForNode(Node $node): ContentSubgraphInterface
Expand Down
31 changes: 28 additions & 3 deletions Neos.Neos/Classes/Service/Controller/DataSourceController.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,14 @@
namespace Neos\Neos\Service\Controller;

use Neos\ContentRepository\Core\Projection\ContentGraph\Node;
use Neos\ContentRepository\Core\Projection\ContentGraph\VisibilityConstraints;
use Neos\ContentRepository\Core\SharedModel\Node\NodeAddress;
use Neos\ContentRepositoryRegistry\ContentRepositoryRegistry;
use Neos\Flow\Annotations as Flow;
use Neos\Flow\Mvc\View\JsonView;
use Neos\Flow\ObjectManagement\ObjectManagerInterface;
use Neos\Neos\FrontendRouting\NodeAddressFactory;
use Neos\Neos\FrontendRouting\SiteDetection\SiteDetectionResult;
use Neos\Utility\ObjectAccess;
use Neos\Flow\Reflection\ReflectionService;
use Neos\Neos\Exception as NeosException;
Expand All @@ -30,6 +35,9 @@
*/
class DataSourceController extends AbstractServiceController
{
#[Flow\Inject]
protected ContentRepositoryRegistry $contentRepositoryRegistry;

/**
* @var array<string,class-string>
*/
Expand All @@ -39,10 +47,9 @@ class DataSourceController extends AbstractServiceController

/**
* @param string $dataSourceIdentifier
* @param Node $node
* @throws NeosException
*/
public function indexAction($dataSourceIdentifier, Node $node = null): void
public function indexAction($dataSourceIdentifier, string $node = null): void
Copy link
Member

Choose a reason for hiding this comment

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

you removed the param annotation – I would instead suggest to keep it and add some docs instead – what is this $node string expected to contain?

Copy link
Member Author

Choose a reason for hiding this comment

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

as per my main description:

where previously Node $node was used to trigger property mapping (DataSourceController and WorkspaceController) we now pass the node as string and use the legacy node address factory to handle the legacy format manually. A followup will adjust everything to work with the new node address.

Copy link
Member

Choose a reason for hiding this comment

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

OK, sorry, missed that one!

{
$dataSources = static::getDataSources($this->objectManager);

Expand All @@ -63,11 +70,29 @@ public function indexAction($dataSourceIdentifier, Node $node = null): void
unset($arguments['dataSourceIdentifier']);
unset($arguments['node']);

$values = $dataSource->getData($node, $arguments);
$values = $dataSource->getData($this->deserializeNodeFromLegacyAddress($node), $arguments);

$this->view->assign('value', $values);
}

private function deserializeNodeFromLegacyAddress(?string $stringFormattedNodeAddress): ?Node
{
if (!$stringFormattedNodeAddress) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!$stringFormattedNodeAddress) {
if ($stringFormattedNodeAddress === null) {

return null;
}

$contentRepositoryId = SiteDetectionResult::fromRequest($this->request->getHttpRequest())
->contentRepositoryId;
$contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId);
// todo legacy uri node address notation used. Should be refactored to use json encoded NodeAddress
$nodeAddress = NodeAddressFactory::create($contentRepository)->createCoreNodeAddressFromLegacyUriString($stringFormattedNodeAddress);

return $contentRepository->getContentGraph($nodeAddress->workspaceName)->getSubgraph(
$nodeAddress->dimensionSpacePoint,
VisibilityConstraints::withoutRestrictions()
)->findNodeById($nodeAddress->aggregateId);
}

/**
* Get available data source implementations
*
Expand Down
5 changes: 2 additions & 3 deletions Neos.Neos/Classes/Service/LinkingService.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
use Neos\Neos\Domain\Model\Site;
use Neos\Neos\Domain\Repository\SiteRepository;
use Neos\Neos\Exception as NeosException;
use Neos\Neos\FrontendRouting\NodeAddressFactory;
use Neos\Neos\FrontendRouting\SiteDetection\SiteDetectionResult;
use Neos\Utility\Arrays;
use Psr\Http\Message\UriInterface;
Expand Down Expand Up @@ -309,15 +308,15 @@ public function createNodeUri(
$controllerContext->getRequest()->getHttpRequest()
)->contentRepositoryId;
$contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId);
$nodeAddress = NodeAddressFactory::create($contentRepository)->createFromUriString($node);
$nodeAddress = NodeAddress::fromJsonString($nodeString);
$workspace = $contentRepository->getWorkspaceFinder()->findOneByName($nodeAddress->workspaceName);
$subgraph = $contentRepository->getContentGraph($nodeAddress->workspaceName)->getSubgraph(
$nodeAddress->dimensionSpacePoint,
$workspace && !$workspace->isPublicWorkspace()
? VisibilityConstraints::withoutRestrictions()
: VisibilityConstraints::frontend()
);
$node = $subgraph->findNodeById($nodeAddress->nodeAggregateId);
$node = $subgraph->findNodeById($nodeAddress->aggregateId);
} catch (\Throwable $exception) {
if ($baseNode === null) {
throw new NeosException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,25 +16,16 @@

use Neos\ContentRepository\Core\Projection\ContentGraph\Node;
use Neos\ContentRepository\Core\Projection\ContentGraph\VisibilityConstraints;
use Neos\ContentRepository\Core\SharedModel\ContentRepository\ContentRepositoryId;
use Neos\ContentRepository\Core\SharedModel\Node\NodeAddress;
use Neos\ContentRepositoryRegistry\ContentRepositoryRegistry;
use Neos\Flow\Annotations as Flow;
use Neos\Flow\Core\Bootstrap;
use Neos\Flow\Http\RequestHandler;
use Neos\Flow\Property\PropertyMappingConfigurationInterface;
use Neos\Flow\Property\TypeConverter\AbstractTypeConverter;
use Neos\Neos\FrontendRouting\NodeAddressFactory;
use Neos\Neos\FrontendRouting\SiteDetection\SiteDetectionResult;

/**
* To be removed legacy fragment for property mapping nodes in controllers.
* MUST not be used and MUST be removed before Neos 9 release.
* See issue: https://github.com/neos/neos-development-collection/issues/4873
*
* @Flow\Scope("singleton")
* @deprecated must be removed before Neos 9 release!!!
*/
class HackyNodeAddressToNodeConverter extends AbstractTypeConverter
class NodeAddressToNodeConverter extends AbstractTypeConverter
{
/**
* @var array<int,string>
Expand All @@ -53,8 +44,6 @@ class HackyNodeAddressToNodeConverter extends AbstractTypeConverter

#[Flow\Inject]
protected ContentRepositoryRegistry $contentRepositoryRegistry;
#[Flow\Inject]
protected Bootstrap $bootstrap;

/**
* @param string $source
Expand All @@ -68,26 +57,16 @@ public function convertFrom(
array $subProperties = [],
PropertyMappingConfigurationInterface $configuration = null
) {
$activeRequestHandler = $this->bootstrap->getActiveRequestHandler();
$contentRepositoryId = ContentRepositoryId::fromString('default');
if ($activeRequestHandler instanceof RequestHandler) {
$httpRequest = $activeRequestHandler->getHttpRequest();
$siteDetectionResult = SiteDetectionResult::fromRequest($httpRequest);
$contentRepositoryId = $siteDetectionResult->contentRepositoryId;
}

$contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId);
$nodeAddressFactory = NodeAddressFactory::create($contentRepository);
$nodeAddress = $nodeAddressFactory->createFromUriString($source);

$nodeAddress = NodeAddress::fromJsonString($source);
$contentRepository = $this->contentRepositoryRegistry->get($nodeAddress->contentRepositoryId);
$subgraph = $contentRepository->getContentGraph($nodeAddress->workspaceName)
->getSubgraph(
$nodeAddress->dimensionSpacePoint,
$nodeAddress->isInLiveWorkspace()
$nodeAddress->workspaceName->isLive()
? VisibilityConstraints::frontend()
: VisibilityConstraints::withoutRestrictions()
);

return $subgraph->findNodeById($nodeAddress->nodeAggregateId);
return $subgraph->findNodeById($nodeAddress->aggregateId);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,27 +15,19 @@
namespace Neos\Neos\TypeConverter;

use Neos\ContentRepository\Core\Projection\ContentGraph\Node;
use Neos\ContentRepository\Core\SharedModel\Node\NodeAddress;
use Neos\ContentRepositoryRegistry\ContentRepositoryRegistry;
use Neos\Flow\Annotations as Flow;
use Neos\Flow\Property\PropertyMappingConfigurationInterface;
use Neos\Flow\Property\TypeConverter\AbstractTypeConverter;
use Neos\Neos\FrontendRouting\NodeAddressFactory;

/**
* To be removed legacy fragment for property mapping nodes in controllers.
* MUST not be used and MUST be removed before Neos 9 release.
* See issue: https://github.com/neos/neos-development-collection/issues/4873
*
* @Flow\Scope("singleton")
* @deprecated must be removed before Neos 9 release!!!
*/
class NodeToNodeAddressStringConverter extends AbstractTypeConverter
{
/**
* @Flow\Inject
* @var ContentRepositoryRegistry
*/
protected $contentRepositoryRegistry;
#[Flow\Inject]
protected ContentRepositoryRegistry $contentRepositoryRegistry;

/**
* @var array<int,string>
Expand Down Expand Up @@ -64,9 +56,6 @@ public function convertFrom(
array $subProperties = [],
PropertyMappingConfigurationInterface $configuration = null
) {
$contentRepository = $this->contentRepositoryRegistry->get(
$source->contentRepositoryId
);
return NodeAddressFactory::create($contentRepository)->createFromNode($source)->serializeForUri();
return NodeAddress::fromNode($source)->toJson();
}
}
Loading
Loading