Checkbox viewhelper should correctly compare non-entity objects in collections

[albe]

Currently the checkbox viewhelper supports comparing against a collection property in order to do a "contains" check for the value. This however always assumes entities and tries to compare against the persistenceIdentifier, which fails for non-entity objects like e.g. Roles or other value objects.

<f:section name="Content">
    <f:form method="post" action="updateRoles" objectName="account" object="{account}" arguments="{account: account}">
        <label>Roles</label>
        <f:for each="{roles}" as="role" iteration="iterator">
            <label>
                <f:form.checkbox property="roles" value="{role}" multiple="true" /> {role.name}
            </label>
        </f:for>
        <f:form.submit class="btn btn-primary" value="Update" />
    </f:form>
</f:section>

This template currently behaves wrong and will not check the roles assigned to the account.

See c4b15e8#diff-c710aa37625a231b5c692f161d74fd4f

The // assume an entity part would probably need to be changed to something similar to:

$valueIdentifier = $this->persistenceManager->getIdentifierByObject($value);
if ($valueIdentifier !== null) {
   $checked = $valueAttribute === $valueIdentifier;
} else {
   $checked = $valueAttribute == $value;
}

[j6s]

I tried to fix this issue - however, the main Problem here seems to be serializing the objects for the value attribute, not setting the checked state.

In this example the Role object has a identifier property, identifying it explicitly - however this is not the norm. From this point I see 3 ways of making this possible:

1. Add an argument to CheckboxViewHelper in order to define how an object should be serialized into the value Tag attribute.
2. Assume that non-entity objects bound to checkboxes have a unique return value of __toString and just use that
3. Serialize the object for value. This may work for simple DTOs but may break for more involved objects and may have serious security implications.

Looking at other, similar ViewHelpers, SelectViewHelper seems to use option 2: casting the value to string for the value tag attribute: https://github.com/neos/flow-development-collection/blob/master/Neos.FluidAdaptor/Classes/ViewHelpers/Form/SelectViewHelper.php#L332

[albe]

Thanks for looking into this and giving a clear assessment of the issue!

My initial thought was to use PHPs object byValue comparison semantics to make this work (at least in cases of POPOs/ValueObjects/DTOs): https://3v4l.org/5lpIC

But you are right, that even if that check is done as above, the $valueAttribute needs to be stringified into the value atttribute, so unless the object implements __toString() this would not work anyway.
So we should probably check if the object implements __toString() first and throw a meaningful error that it needs to implement that in a way that distinguishes the object uniquely.

I wouldn't go to lengths to try to make this work for any edge case of putting a deep complex object with injections into the checkbox as a value. So I'd also go for option 2 on this.

This still would glitch if the MyVo from above would implement a __toString() method returning only $this->foo of course.
If we would want to have some better guarantees, we could only depend on an contract interface, that the object class needs to implement and throw if it doesn't (and isn't an entity in the first place). It trades off safety (from subtle bugs or information disclosure) against convenience in the common use cases (array<MyPopo>).