!!! TASK: Serializable Commands

[bwaidelich]

Turns the low-level commands CreateNodeAggregateWithNodeAndSerializedProperties, SetSerializedNodeProperties and SetSerializedNodeReferences into implementations of a new SerializedCommandInterface in order to prevent them from being dispatched "from the outside".

This is a breaking change in case that someone instantiated and sent one of those lowlevel (and @internal) commands to ContentRepository::handle(). In that case the corresponding high level command should be used instead.

Related: #5353

Neos ui neos/neos-ui#3881

[bwaidelich]

With the current WIP tests will fail because of the TransformationsFactory working on the ContentRepository instance (solely to call handle() and in one case to access the content graph). We could probably fix that by passing it the CommandBus instead such that it can trigger those low level commands (or maybe don't use commands but events for node migrations.. but that's a bigger rabbit hole I guess)..

But basically the PoC works with Neos

[mhsdesign]

Thanks, the idea is also that we centralise RebaseableCommand::enrichEvents which would be a real gain as this is prone to be wrong:

#5301 (comment)

Alright, at least according to the tests this works now. I also went through all Rebasable commands to check if the events get enriched. the Dimension ones were the only missing. IMHO we should centralize this behavior, I opted against doing it here though as I think we need to consider if there would have to be any more logic involved to decide what gets enriched with commands, in what way, when we override the command if there is already metadata and finally what the causation ids are. I guess we could ignore all these questions and centralize it, but it warrants a closer look and is therefore out of scope of this change.

[kitsunet]

Interesting idea, probably the right direction. i am not madly in love with the way the interfaces interact though... This is really .... something:

(self&CommandInterface)|(self&SerializedCommandInterface);

But I have no great alternative atm...

[mhsdesign]

maybe using @return $this to "fake" it for phpstan? And just using self in php? THe union is crazy :D

[bwaidelich]

i am not madly in love with the way the interfaces interact though

I agree, it's at least uncommon. But it just affects very few very internal parts of the code – does it really matter?

[bwaidelich]

There are 5 NodeMigration implementations that use SetSerializedNodeProperties – it would be interesting to find out why they shouldn't use the "official" commands.
For the RemoveProperty, RenameProperty, StripTagsOnProperty transformations it should be easily possible to use the "official" SetNodeProperties command.
But AddNewProperty and ChangePropertyValue expect serialized property values in the transformation and there is even a test with an example for adding a property that is not defined in the schema – I'm not sure what the usecase is for that..

In any case: If we need to keep it like that but want to make SetSerializedNodeProperties an internal concept, we can either expose the CommandBus to the transformations (but that would make it a new service factory dependency and.. uh..) or just pass the PropertyConverter to the transformations such that they can serialize node properties themselves..
However that would still not allow a transformation to set a node property that is not defined (or invalid) according to the schema

[bwaidelich]

@mhsdesign @kitsunet Thanks a lot for your continuous efforts!
I have to say I lost track of the discussions in this and related PRs. So allow me to summarize how I see the issues and especially the blockers for a 9.0 (and please let me know if I miss something important – it's quite possible):

Recap

During a rebase (i.e when I sync changes from a target workspace with new changes to my workspace that also has pending changes) we currently re-apply the original commands to find out wether my pending changes are in conflict with the synced changes.

But because there are some non-idempotent commands we introduced lower-level commands for those that contain "serialized" information about the state of the system at the time of the initial command handling in order to make it (more) idempotent (e.g.: the CreateNodeAggregateWithNodeAndSerializedProperties command contains the explicitly set + the default property values of the node type so that they are consistent even if the node type schema changed)

Blocker

The main issue with that is: Now users can send those low-level commands directly to the CR, skipping some constraint checks (and making the API more complex than it'd need to be).
Furthermore, the low-level commands would/should not trigger Command Hooks because they are sent to the handlers directly, creating yet another implicit divide between the two types of commands.

That is the issue, this PR tries to solve by making the low-level commands an internal concept.

Discussions

I'm happy to fine-tune the namings & types of course, but personally I don't care about those (now called) serialized commands because they are now just an internal thing and nobody should have to deal with them (apart from the core) – or am I missing something here?

Further thoughts

Some more ideas that should not distract us from the important issues, but maybe help to find better in-between-solutions:

I looked at the events and realized that we store the FQN of the commandClass in the metadata.
We should avoid that for the same reason not to store the FQN of the event class names.
Also commandClass is not necessarily the right wording. It is more like: A "snapshot" of information that is needed to re-evaluate constraints.

And, a little more controversial maybe:

The information for the "command" is mostly already contained in the corresponding event. So I wonder why we decided to store information about the command in the metadata.
E.g. for an event of NodePropertiesWereSet the corresponding commandClass will always be SetSerializedNodeProperties so we would not have to store that in the events (let alone the FQN).
Also the command payload should be extractable from the event payload in most cases (and where it is not we could add it to the event because it was apparently missing some information).

As a result we would completely get rid of the command metadata hack and instead re-apply events + checking (some) constraints.

[bwaidelich]

As discussed with @mhsdesign and @kitsunet

A result of this PR is that the "lowlevel commands" are now completely internal and should not be exposed to the userland at all (that's a good thing IMO).

As a result, we need to tweak the CommandThatFailedDuringRebase construct that was introduced to render details about a conflict in the Neos UI because that previously exposed those.
Instead we decided to expose the events that led to a conflict (for now. A cleaner solution might be to expose the affected node and some higher level "type of change" – but that would require yet another concept and should be thought through carefully)

• rename CommandsThatFailedDuringRebase to EventsThatFailedDuringRebase
• rename CommandThatFailedDuringRebase to EventThatFailedDuringRebase and change its signature from

final readonly class CommandThatFailedDuringRebase
{
    public function __construct(
        public CommandInterface $command,
        public \Throwable $exception,
        private SequenceNumber $sequenceNumber,
    ) {
    }

    public function getSequenceNumber(): SequenceNumber;
}

to

final readonly class CommandThatFailedDuringRebase
{
    public function __construct(
        private EventInterface $event,
        private \Throwable $exception,
        private SequenceNumber $sequenceNumber,
    ) {
    }

    /** @internal **/
    public function getEvent(): EventInterface;
    public function getException(): \Throwable;
    public function getSequenceNumber(): SequenceNumber;
}

...and change the UI code accordingly

[bwaidelich]

@skurfuerst you had originally introduced this test case with 0f88ed6 but in our opinion node migrations should not be allowed to set properties that are not allowed according to the node type schema. Do you agree to that assumption?

[mhsdesign]

Thank you both for polishing this so well :)

[bwaidelich]

I'm happy with this one now, but I can't +1 it. @mhsdesign @kitsunet shall we merge this?

[mhsdesign]

@bwaidelich please have a look how i handled things in the Neos ui now neos/neos-ui#3881

[mhsdesign]

we discussed that for the case we would want to replay events actually that also a succeeding event might appear here. Currently only the initial event will be available via $event, but we could still later decide to add an additional events list here.
If we were to replay events we can not allow only one to pass. All correlated must pass i assume (as otherwise tethered nodes could be missing), so the first event - if really the part of the conflict or not - will always be $event.