search

Flowpack / Flowpack.NodeTemplates

Neos CMS package that auto creates nodes based on a declarative template
GNU General Public License v3.0
31 stars 10 forks source link

DISCUSSION: Should `property: null` be ignored? #41

Open mhsdesign opened 1 year ago

mhsdesign commented 1 year ago

When building a more complex template and a loop, you might encounter the situation, when you only want to set this property on iteration 1 or something and otherwise return null, thinking it will be ignored.

Currently the behaviour with null might not be 100% obvious.

template:
  properties:
    someSimplePropertyWithDefaultValue: null
    someReferencePropery: null
    someReferencesProperty: [null]

in the first case someSimplePropertyWithDefaultValue will be set to null (see $node->getProperties()) (also see somewhat related discussion about properties without default value or not previous value beeing skipped: https://github.com/neos/neos-development-collection/issues/4292)

in the latter cases with the reference properties, the null value will be (rightfully) ignored, but because the setProperty will guard this:

https://github.com/neos/neos-development-collection/blob/1b744c8a6959a4aae735ffc6dd360f33847c98aa/Neos.ContentRepository/Classes/Domain/Model/AbstractNodeData.php#L167-L188C1

the question i asked myself, if it would be generally expected, that when null is returned, that the setProperty operation will be skipped for the property.

This would be helpful for cases, where you dont want to hardcode the default value. But this change might be breaking in unexpected ways...

mhsdesign commented 1 year ago

~will be solved via: https://github.com/Flowpack/Flowpack.NodeTemplates/pull/53~

Properties with default values cannot be set to null anymore. This would cause the property to be unset and thus might cause unintended behaviour when null is not handled in the Fusion Rendering. We know this is a bolder move, but in our test it actually prevented us from writing malformed templates. If you disagree feel free to start and discussion in the issues and explain us your use case.

mhsdesign commented 1 year ago

Currently the discussion is to "not try to be smarter than the system" (the system being the neos cr)

We are thinking about introducing a required: true field next to type https://github.com/neos/neos-development-collection/issues/4304, which will work like a not empty validator, except its validated on server side. This feature will make my odd approach in https://github.com/Flowpack/Flowpack.NodeTemplates/pull/54 useless and resolve this discussion, as one should just declare properties to be required instead and it will be enforced by the CR.

mhsdesign commented 4 months ago

Neos 9 has a bug which initially omits the null values but an explicit set properties will still work: https://github.com/neos/neos-development-collection/issues/5154

As talked with @kitsunet we are unsure what the correct behaviour should be :D