!!! BUGFIX: Always unset node property, if set to null

[mhsdesign]

Previously we made a distinction between unsetting a node property and setting it to null on the write side.

There was setting to null SerializedPropertyValues::fromArray(['tagName' => new SerializedPropertyValue(null, 'string')]) and unset SerializedPropertyValues::fromArray(['tagName' => null])

Unsetting a property would only be possible via php api by creating a custom SerializedPropertyValues.

The read site treats it the same and returns true in hasProperty only if there is a non null value in the database.

As discussed with @bwaidelich

I would recommend to not make a difference whether a property wasn't set initially or whether it was removed lateron.
While it might be useful to expose this information at some point, we should do so explicitly and not via the null-behavior of some adapter.

Previously if a property tagName was set to null

the NodePropertiesWereSet event would contain

"propertyValues":{"tagName":{"value":null,"type":"string"}}

and the properties of the node in the projection as well

"tagName":{"value":null,"type":"string"}

---

which this change the the NodePropertiesWereSet event would contain:

"propertyValues":{"tagName":null}

and the properties of the node in the projection would show no hint of this property.

---

Our logic to decide between unsetting properties and setting them to null was spread through a lot of classes and is a sheer wonder that it worked :D Now with the instruction of the UnsetPropertyValue, it is much more obvious how the behaviour works.

solves: #4467

Important

This change is breaking to your existing content repository data. To migrate your events please execute:

flow migrateevents:migratePropertiesToUnset
flow cr:projectionReplay contentGraph

otherwise you will face this error SerializedPropertyValue::__construct(): Argument #1 ($value) must be of type ArrayObject|array|string|int|float|bool, null given

[bwaidelich]

From the description and the code itself, I don't really understand this change. But I have the strong feeling that is should be backed by a test

[mhsdesign]

Yes i see, sorry for being so spare about the details.

Id like to "quickly" chat with you about this to structure my mind - after that i could probably write it down more easily, and we could also discuss if my expectations are correct.

[bwaidelich]

Id like to "quickly" chat with you about this to structure my mind

Those quotes around "quickly" make me suspicious :)
No, sure thing, let's talk about this sometimes next week!

[bwaidelich]

I added some thoughts to #4304 (comment)

[skurfuerst]

looks good :) wonder why the stuff before was done ;)

[mhsdesign]

Now i only have to come up with a test for this ;) (Thats why its WIP - and also need to rebase it)

Outdated ;)

[mhsdesign]

As we discussed previously it is an implementation detail of the adapter to save null values to the database or not, thats why its a bit hard to come up with a test? And as everything passes that seems proof enough this refactoring is correct?

[bwaidelich]

Sorry for being complicated, but I dislike that one of the highest-level APIs now has this awkward withoutUnsets() method.
I am probably missing something but I expected SerializedProperties to be just that, a set of serialized properties. But that they would never contain any null-values.
I.e. that we would filter them out at write time.

I know, that we need it to unset previously existing properties, but I thought that we could put that logic into PropertyValuesToWrite rather than having something like Node->properties->serialized->withoutUnsets() which does not make sense to me on the read side at all

[mhsdesign]

We discussed to introduce a PropertyNames dto and a new property in the SetSerializedProperties command / event $propertiesToUnset.

Additionally we briefly discussed that a property converter for the properties should probably never return null, as this would cause confusion about hasProperty seeminly not working.

[mhsdesign]

So i also just wrote a database migration that will update the events. Adding a php compatibility layer would just permanently introduce complexity and possibly bugs.

The following things have to be migrated:

• NodePropertiesWereSet payload
• SetSerializedNodeProperties in metadata
  example:

"propertyValues":{"tagName":{"value":null,"type":"string"}}
->
"propertyValues":[],"propertiesToUnset":["tagName"]

Here we just have to omit the null values because its a new node either way:

• NodeAggregateWithNodeWasCreated payload
• CreateNodeAggregateWithNodeAndSerializedProperties in metadata

"initialPropertyValues":{"title":{"value":"Blog","type":"string"},"titleOverride":{"value":null,"type":"string"},"uriPathSegment":{"value":"blog","type":"string"}}
->
"initialPropertyValues":{"title":{"value":"Blog","type":"string"},"uriPathSegment":{"value":"blog","type":"string"}}

• CopyNodesRecursively in metadata

// somewhere deep recursive in $nodeTreeToInsert
"propertyValues":{"senderName":{"value":null,"type":"string"}}}
-> 
"propertyValues":{}

Also the basically impossible "initialPropertyValues":{"tagName":null} and "propertyValues":{"tagName":null will be migrated.

[kitsunet]

Cool, fine by me now, shall I hit merge?

[kitsunet]

(I guess we agreed in the weekyl this is fine)

[mhsdesign]

Yes i even did some additional testing that the command payload is also correctly migrated and a workspace with migrated changes can be merged.

Test cases:

• unset node properties via set serialised properties
• copy nodes which have explicitly unset properties which will also be added to the command
• create node with initial unset property

Each of the scenarios worked when being published after switching the branch and applying the migrateevents:migratePropertiesToUnset migration.

[mhsdesign]

Neos.Demo followup neos/Neos.Demo#191