-
-
Notifications
You must be signed in to change notification settings - Fork 221
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
FEATURE: References on creation and Copy #5148
base: 9.0
Are you sure you want to change the base?
Conversation
Do not merge yet, probably want to refactor some of the code in the DTOs. I think we should make the "collection of references for one reference property" an explicit DTO, as the code like this feels a bit too messy for my taste... |
On the upside the somewhat weird and unfinished reference snapshots can be removed with this. |
And forgot to adapt legacy migration tests (which rely on specific event payloads, see below, before I adjust those we should decide which way to go with b/c)... |
oh and I should mention that at the moment this is not forgiving to the existing events. We could do this in the respective fromArray constructors of the DTOs and just "upcast" from the old structure on the fly without much problem, or add a migration for the events. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this, some early thoughts before ill play with this ;)
private function mapNodeReferencesToSerializedNodeReferences(NodeReferencesToWrite $references, NodeTypeName $nodeTypeName): SerializedNodeReferences | ||
{ | ||
$serializedReferences = []; | ||
foreach ($references->getIterator() as $reference) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
foreach ($references->getIterator() as $reference) { | |
foreach ($references as $reference) { |
isnt it part of the IteratorAggregate
thing that one doesnt have to do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, right, I messed with this a bit, there are probably more artifacts like this in there.
* | ||
* @api used as part of commands | ||
*/ | ||
final readonly class NodeReferenceNameToEmpty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hu, i thought we wouldnt need a *Unset
class like this as only the references will be set that are part of the SetNodeReferences
command and passing an empty array will unset all references?
Also i had to read twice before i understood ToEmpty
literally ... im not super happy with it :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep naming TBD, I am not super sold on this either, AND if we introduced the "in between" DTO as said above we wouldn't need this but would just say "NodeReferencesForProperty" and have that be empty for the specific reference name.
*/ | ||
public function jsonSerialize(): array | ||
public function targetAndPropertiesToArray(): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would rather like to inline this code to the caller instead of here?
or a generic toArray
? Idk or make it internal :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah internal I guess... But htis might again become easier with the middle DTO.
@@ -47,32 +66,34 @@ public static function fromReferences(array $references): self | |||
} | |||
|
|||
/** | |||
* @param array<int,array<string,mixed>> $values | |||
* @param array<string, array<array{"referenceName": string, "target": string, "properties"?: array<string, mixed>}>> $namesAndReferences |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im not sure if we really like to start annotating the fromArray shape as its a lost battle in terms of time to invest :D But thanks either way ^^
@@ -26,32 +27,37 @@ | |||
* (this is validated in the command handler). | |||
* @api used as part of commands | |||
*/ | |||
final readonly class NodeReferenceToWrite implements \JsonSerializable | |||
final readonly class NodeReferenceToWrite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for improving the api for setting references ill look into it how this new way is to be used instead and what is to be migrated for Neos 9 users...
While fine tuning the references api id like to also note that it often came to my mind that it didnt seem to have gotten the same love as setting properties. I think both setting references and properties should feel similar and not be totally a different thing.
This reworks references so that multiple reference properties can be set via a single command and also references can be attached to `CreateNodeAggregateWithNode` which is also used for copying nodes.
1ee4c27
to
1612659
Compare
# Conflicts: # Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php
This reworks references so that multiple reference properties can be set via a single command and also references can be attached to
CreateNodeAggregateWithNode
which is also used for copying nodes.The serialization format is adapted to allow multiple reference properties, which also affects all behat tests with references.
A behat test to show reference copies work was added.