BUGFIX: Re-introduce stricter checks for subtree en/disabling and tagging

[bwaidelich]

Previously, when trying to disable an already disabled node, we just ignored that fact and did not produce any events. That leads to a potentially opaque behavior:

User A: log into backend
User B: log into backend
User A: Hide Node X
User B: Hide Node X
User B: Publish
User A: Rebase
User B: Unhide Node X
User B: Publish
User A: Rebase <--- now Node X is re-enabled even though User A had explicitly disabled it

With this change, the sequence is:

User A: log into backend
User B: log into backend
User A: Hide Node X
User B: Hide Node X
User B: Publish
User A: Rebase <--- this leads to a conflict because both users disabled the same node. And User A can now explicitly decide to ignore that fact

Basically reverts #4284 again because we have conflict resolution

[bwaidelich]

Current, intransparent behavior:

5357_screenrecording_bug.mov

And with the fix applied:

5357_screenrecording_fix.mov

[mhsdesign]

Thanks, does that mean that EventsToPublish is never empty and we can remove all this ->isEmpty check logic?

[bwaidelich]

Thanks, does that mean that EventsToPublish is never empty and we can remove all this ->isEmpty check logic?

Right, that's why I removed it :)

[mhsdesign]

sorry i must have been blind :D Thanks a lot, reduced complexity imo and is more correct.

Currently the Ui conflict resolution could be improved though but it handles it:

[mhsdesign]

i thought already if there are other places publishing 0 events, and yes there are: SetNodeProperties with 0 properties :D

Now this introduces a regression and throws in the event publishing

Writable events must contain at least one event

Options to fix this:
A: Reintroduce the write publish events empty check ... meh ...
B: Throw an exception in SetNodeProperties ... not simple to use without any properties?
C: Catch this case in the command handler earlier and return fast
D: Nullable events to publish?

[bwaidelich]

meh..

E: Prevent PropertiesToWrite to exist empty

[mhsdesign]

E would mean that the initialProperties of a node creation must never be empty and thus the field has to be nullable then.

[bwaidelich]

the field has to be nullable then

Right, but i think that would be fine in this case. It is already nullable in CreateNodeAggregateWithNode::create() and would make this case more explicit IMO.
OTOH it would mean that NodeAggregateWithNodeWasCreated::fromArray() would have to filter an empty $values['initialPropertyValues']

[kitsunet]

Sounds fine to me, I am considering in my head if we would ever need to make a difference between no initial properties and empty initial properties, but I don't quite see it yet. (I could construct a case were you want to avoid defaultValues and eg. we make it so that empty initial fills the defaults while null initial will not, which we would block this way, buuuut that seems far fetched.) So it seems like a relatively safe choice, even if nullable is annoying of course.

[mhsdesign]

@kitsunet its not tooo far fetched: #5154 i stumbled already upon that unsetting default values does NOT work via initial properties ...

[kitsunet]

I still think this is a mistake and we should allow empty event sets 🤷 Instead of trying to fix it at the source (which might not even know without expensive checks if an event would result)

[mhsdesign]

Hmm with the followup in #5396 which leverages heavily phpstan to ensure no empty evens are ever published, im positive that we can go down this road and i do like the new strictness and it definitely makes sense that each command results in at least one event also in light that we want to prevent noops for publish and rebase on the lover level: #5337

[kitsunet]

I think it's weird to expect the command issuers to effectively know if something would result in an event or not.... But yeah lets go with this for now, we can always get more loose again.