FEATURE: Create ValueObjects from simple types by calling from$TYPE for objects and JsonArray properties

[mficzel]

When converting from simple types the denormalizer now checks wether the target class has a method from$TYPE and uses this to create the object. Supported method names are fromArray, fromString, fromBool, fromInt and fromFloat.

For example the following controller action would use the methods BirthDate::fromString or BirthDate::fromArray (depending on submitted data) to instantiate the $birthDate argument.

public function indexAction(BirthDate $birthDate) 

In addition the JsonArray Type is extended to support this aswell. The serialization as flow entities takes precedence over the serialization as value object flow objects that implement \JsonSerializable are still serialized as Flow Objects in JsonArray properties. While beeing a Flow feature this allows to store and restore ValueObjects in Neos Node properties aswell.

Note: This is a rebased version of #2703 from @nezaniel

Resolves: #2763
Replaces: #2703

Checklist

• Code follows the PSR-2 coding style
• Tests have been created, run and adjusted as needed
• The PR is created against the lowest maintained branch

[mficzel]

@albe @bwaidelich @mhsdesign as you commented on the original pr and already liked it just not as a bugfix.
It would be great to get this approved for 8.0

[kdambekalns]

Looks good, left a "naming question".

[kdambekalns]

😘

Approval by reading…

One last question: Do you think this would deserve some documentation? Or should we rely on generated type converter reference?

[mficzel]

@kdambekalns docs would be great but i am not entirely sure where to put what. Can be done later imho.
I wonder where those psalm errors come from. The mentioned lines are not changed in the pr.

[mficzel]

Found and fixed the issues psalm detected. All green now.

[nezaniel]

Still looks good to me :)

[mficzel]

@kdambekalns @nezaniel pushed a final adjustment to handle the value object serialization in JsonArray after the check for flow-objects. That should avoid problems wit flow entities that implement JsonSerializable.

[mficzel]

After chat with @nezaniel we agreed that the last change makes sense and improves confidence. Will merge with the existing reviews as this is a minor cosmetic change.

[bwaidelich]

Oh no, I was one hour too late..
I really like the change but I'm super sad that we now have fromInt vs fromInteger etc as this will trip folks who were used to using Value Objects with Neos.EventSourcing where we use the full names (see #2703 (comment))

[mficzel]

@bwaidelich can you decide on this with @kdambekalns i am really not that opinionated on this and until we push release we can still adjust this.

To me the argument to use the same syntax as in typeAnnotations made more sense.

[mhsdesign]

i guess there is no right or wrong way - but if we decided to took road fromInteger in the event sourcing already, i think we keep that course - (or we end up like php namings ^^)

[bwaidelich]

OK, I'll try to rephrase my arguments for the full version.
To recap, it is about fromInt() and fromBool() (this PR) vs fromInteger() and fromBoolean() (Neos.EventSourcing):

IMO...

• ...method names are less "technical" than type hints. While it makes sense to use short versions for the latter, we usually try to avoid abbreviations in class and method names (and I think for a good reason)
• ...we use the full versions in our TypeHandling
• ...the Neos.EventSourcing package comes with a ValueObjectNormalizer that uses the full names

In the end it probably boils down to preference. If you guys agree to my suggestion, I'm happy to create a follow-up PR with adjustments (and provide some documentation on the way).
Otherwise I would at least vote for a version that supports both fromInt() and fromInteger() etc.

[mficzel]

@bwaidelich you had me at "and provide some documentation on the way"

Not entirely sure about supporting long and short versions but that might be the friendliest way in the end.
Also for me as i do not have to remember which was which.

[mficzel]

Thinking further i would prefer to add the additional methods as a last minute feature. If we do not get this intuitively noone else will.

[kdambekalns]

While it makes sense to use short versions for the latter

No, it does not… not anymore than for anything else. And I find it unfortunate that PHP now uses both ways, but not in all places. 🤷‍♂️

we usually try to avoid abbreviations in class and method names (and I think for a good reason)

I repeat my argument about the short form being more "universal" in the PHP context. We do support int in docblocks, too, after all.

But I didn't know about the same methods used in the EventSourcing package already, and I do see value in both ways.

So… whatever. 🙃

[albe]

I agree to what Martin said - having both ways supported is probably the most friendly way to handle this in the core. I do not see any drawbacks from that, since there are no subtle differences in using one or the other. It's just there to remove mental load from developers, which is good. Let's do this @bwaidelich

[bwaidelich]

There you go: #2766