Split up NodeAddress into dedicated classes
#4564
Comments
SuggestionBoth usecases (addressing and (de)serializing nodes) seem to be very common, so I would suggest to add those new DTOs to the namespace Neos\ContentRepository\Core\SharedModel\Node;
final readonly class NodeIdentity {
public function __construct(
public ContentRepositoryId $contentRepositoryId,
public ContentStreamId $contentStreamId,
public DimensionSpacePoint $dimensionSpacePoint,
public NodeAggregateId $nodeAggregateId,
) {}
}namespace Neos\ContentRepository\Core\SharedModel\Node;
final readonly class NodeSerializer {
public function __construct(
private WorkspaceName $workspaceName,
private DimensionSpacePoint $dimensionSpacePoint,
private NodeAggregateId $nodeAggregateId,
) {}
public static function fromString(string $value): self { /* ... */ }
public static function fromUri(UriInterface $uri): self { /* ... */ }
public function toString(): string { /* ... */ }
public function toQueryString(UriInterface $uri): UriInterface { /* ... */ }
}And maybe a third one to be able to (de)serialize nodes with their CR id – Alternatively the |
|
Since the NodeAdress is a concept that will be used frequently by the user we should definitely polish this concept. Main usecase would be to serialize a node into the html so a javascript application can used it as get parameter and on the server we deserialize it. |
Just a little side note: It might be tempting to add corresponding methods to the Value Object itself, but I would suggest to keep URL/HTML/JSON,.... specifics out of it and have a separate class for (de)serialization (as suggested above) |
|
yes separating this makes perfect sense! i think we need two (or three) main serialization formats that we would use ourselves: 1. uri parameter serialization
|
|
I don't see why 1, 3 and 4 are separate things, you can transport json via uri perfect fine, and why would you ever want a non fully qualified format. That said 4 (but please more sensible separator character) could work too for me, just that json also works in other "contexts" than a URI. I alson wonder about 2, the stored uris here need to be fully addressable in terms of the cr, so it needs to contain the nodeAggregate and ContentRepositoryId. How else would you cross link to a node in another cr? |
Yes, I was wondering the same; can't we just come up with a common serialization format that we use in URIs (and potentially in other places)? Some central "serializer" class that can convert node addresses to JSON vice versa. final readonly class NodeSerializer {
private function __construct() {}
public static function nodeAddressFromJson(string $json): NodeAddress { /* ... */ }
public static function nodeAddressToJson(NodeAddress $nodeAddress): string { /* ... */ }
}By the way: With #4708 we might not need the Question: What about the CR Id that is part of 3. and 4. but not of 1. ( // ...
public static function contentRepositoryIdFromJson(string $json): ContentRepositoryId { /* ... */ }
public static function NodeAddressAndContentRepositoryIdToJson(NodeAddress $nodeAddress, ContentRepositoryId $contentRepositoryId): string { /* ... */ }...but I'm not sure about that.
The idea was to keep support for: |
|
This suggests to me the NodeAddress should be a more internal thing that is localised to hand addresses around within a defined content repository. for any more "public" use it seems important to know the content repository as well? As in it should never be serializable because it is incomplete and would always need further context to work? |
|
To make a further point, we already have an absolute serialisation format that is highly specialised, the live URI. This can include the CR hidden in the domain, the dimensions, the nodeAggregate and by virtue of being specialised the workspace. So it's a fine serialization format as it includes all necessary information to deserialize into a specific node. |
|
After our discussion today, I would suggest to replace the namespace Neos\ContentRepository\Core\SharedModel\Node;
final readonly class NodeIdentity {
public function __construct(
public ContentRepositoryId $contentRepositoryId,
public WorkspaceName $workspaceName,
public DimensionSpacePoint $dimensionSpacePoint,
public NodeAggregateId $nodeAggregateId,
) {}
}because we probably don't want to expose the One of the goals would be to replace the Another goal is to make mapping of nodes in controllers easier, for example: final class SomeController extends AbstractController {
public function __construct(private readonly NodeSerializer $nodeSerializer) {}
public function someAction(NodeIdentity $nodeIdentity): void {
$node = $this->nodeSerializer->nodeFromIdentity($nodeIdentity);
}
}Note: We won't be able to allow a mapping to the |
|
Just to add another note from the previous weekly (9.2.). We discussed that a simple json presentation of the The reasons for this were that delimiting by special characters like Instead a simple json string representation like About the possible implications that the |
|
@grebaldi and me discussed this matter. For our main backend pro
con
alternative as root level query params And for service endpoints where we want to serialise the node identity as query parameter for simplicity pro
con
The dimensionspacepoint will be calculated via this expression: which results in a shorter string than if |
|
does anyone really need to read this? Also the latter variant somehow contains space characters? implicit ordering might also be dangerous-ish? aaand base64 can add = as padding wihch then is not allowed in get parameters. so I guess rather urlencode is needed.... |
|
Actually why ont combine both to some degree?
would work both as path part as well as node= GET param. The url encode is strictly not necessary when used as path segment but doesn't harm either. (and I guess the http framework takes care of that anyways so we don't have to). Yes we need a route part handler for this, but that seems somewhat trivial? |
|
i like the idea of being compatible for both. what do you say about it: yield 'no dimensions' => [
'nodeAddress' => NodeAddress::create(
ContentRepositoryId::fromString('default'),
WorkspaceName::forLive(),
DimensionSpacePoint::createWithoutDimensions(),
NodeAggregateId::fromString('marcus-heinrichus')
),
'serialized' => 'default/live//marcus-heinrichus'
];
yield 'one dimension' => [
'nodeAddress' => NodeAddress::create(
ContentRepositoryId::fromString('default'),
WorkspaceName::fromString('user-mh'),
DimensionSpacePoint::fromArray(['language' => 'de']),
NodeAggregateId::fromString('79e69d1c-b079-4535-8c8a-37e76736c445')
),
'serialized' => 'default/user-mh/language%3Dde/79e69d1c-b079-4535-8c8a-37e76736c445'
];
yield 'two dimensions' => [
'nodeAddress' => NodeAddress::create(
ContentRepositoryId::fromString('second'),
WorkspaceName::fromString('user-mh'),
DimensionSpacePoint::fromArray(['language' => 'en_US', 'audience' => 'nice people']),
NodeAggregateId::fromString('my-node-id')
),
'serialized' => 'second/user-mh/language%3Den_US%26audience%3Dnice%2Bpeople/my-node-id'
];this is only bit weird:
dsp is encoded via: PR: -> #5067 |
Currently the
NodeAddressin theNeos\Neos\FrontendRoutingnamespace has a couple of issues:contentStreamIdandworkspaceNameeven though the latter can be derived from the former (there are reasons for this, but it makes this object a little bit quirky)contentRepositoryIdto the object even though that information is not relevant for its most important usecase (serializing a node)Both not major issues, but it turns out that this object is used in many many places already for completely different purposes which it was never meant to be used for:
IMO only the last usecase fits the object and it would be worth to introduce dedicated DTOs for each:
The text was updated successfully, but these errors were encountered: