RFC: Value Objects vs. Type Conversion and Validation

Nezaniel · September 11, 2017, 10:13am

Fellow Neosians,

I think it’s time for a new RFC regarding the integration of value objects into Flow and Neos in particular. As a first step, I’d like to talk about type conversion because it’s at the heart of everything.

Motivation

I found Mathias’ workshop in Hamburg earlier this year rather inspiring. In the past months I have increased using value objects a lot and am extremely happy having them in my code base.

##Status quo
Type conversion currently relies on implementations of Neos\Flow\Property\TypeConverterInterface which in convertFrom returns either the target object, null or an error message if anything went wrong. These error messages then are used to populate validation results etc.

##Considerations
###Conversion
The most simple conversion is that of value objects with only one constructor parameter. This already works just fine. It gets more interesting though for value objects with multiple properties, which will require a new property mapping mode from e.g. array to value object.
###Validation
Value objects usually validate themselves upon construction. The way to go there is to throw something if the validation fails. The thing to throw would be a ValidationError / a ValidationException which then could be caught by the validation framework to populate the validation result

##Example
Consider the following alternatives for defining a form field in a form factory:

        $emailElement = $registrationPage->createElement('email', 'Neos.Form:SingleLineText');
        $emailElement->addValidator(new EmailAddressValidator());

        $emailElement = $registrationPage->createElement('email', 'Neos.Form:SingleLineText');
        $emailElement->setDataType(EmailAddress::class);

The latter implementation, which imho is much more desirable, currently works only partially until you add the following ScalarTypeToValueObjectConverter:

public function convertFrom($source, $targetType, array $convertedChildProperties = [], PropertyMappingConfigurationInterface $configuration = null)
    {
        try {
            return new $targetType($source);
        } catch (\Exception $exception) {
            return new Error($exception->getMessage(), $exception->getCode());
        }
    }
```
which I'd like to get rid of.

##Proposal
As a first step, let's

* Use validation exceptions everywhere instead of returning Errors on type conversion
* Introduce value objects where they can replace validators (EmailAddress etc)
* Deprecated all those validators and use value objects in there for now

I'm not sure wheter this is breaking. The validator deprecation/fallback should make the transition quite smooth, I don't know how much of our validation framework is public API though.
Target versions would be 4.3 or 5.0 respectively.

bwaidelich · September 14, 2017, 1:57pm

Thanks for that RFC, great initiative!

I fully agree that we should promote VOs more and improve support for them.

Yes, this should work out of the box IMO.
For example:

final class FullName
{

    /**
     * @var string
     */
    private $givenName;

    /**
     * @var string
     */
    private $familyName;

    public function __construct(string $givenName, string $familyName)
    {
        $this->givenName = $givenName;
        $this->familyName = $familyName;
    }

    public function getGivenName(): string
    {
        return $this->givenName;
    }

    public function getFamilyName(): string
    {
        return $this->familyName;
    }

    public function __toString(): string
    {
        return "{$this->givenName} {$this->familyName}";
    }

}

I’d expect this to work:

$fullName = new FullName('Some', 'Name');
$array = $this->propertyMapper->convert($fullName, 'array');
$fullName2 = $this->propertyMapper->convert($array, FullName::class);

The first part works, but it creates an array

{
  "familyName": "Some",
  "givenName": "Name",
  "__type": "Namespace\FullName"
}

and that __type thingy should not be part of this by default IMO.
That would also fix the conversion back that currently dies with an exception “Override of target type not allowed”.

And of course the even simpler case should work, too:

final class EmailAddress
{

    /**
     * @var string
     */
    private $value;

    public function __construct(string $value)
    {
        // TODO validate $value
        $this->value = $value;
    }

    public function getValue(): string
    {
        return $this->value;
    }

    public function __toString(): string
    {
        return $this->getValue();
    }

}

Converting to the VO already works as expected:

$this->propertyMapper->convert('some@email.com', EmailAddress::class)

But

$this->propertyMapper->convert(new EmailAddress('some@email.com'), 'string');

would require a custom TypeConverter as you mentioned… And, yes, we should add that to the Flow package IMO!

We could also add support for the \JsonSerializable interface to the PropertyMapper and use that directly if the source object implements it (problem though: it has a return type of mixed - barf - and it only goes one way).

Returning a Result object has the advantage that it can carry more information. Also, it doesn’t stop processing which is required if you want to display all validation errors in a form for example. How would you solve this with exceptions?

Can you provide an example of where we use a validator that could be replaced today?

Right. I think we have to categorize the validators. Those that act on things “outside of the object scope” (like the UniqueEntityValidator ) still make sense in general (even though there’s probably better ways to enforce these kinds of constraints)…
What about the NotEmpty validator?

Nezaniel · September 14, 2017, 9:45pm

I think implementing the backwards direction shouldn’t be that hard. Yes, it is mixed but in the end usually int (easy), string (easy), array (rather easy) or \stdClass (rather easy) comes out. The tricky part is rather to what class to convert to (I guess that’s why there’s no jsonUnserialize in PHP) , not how since the object can do that itself imho. And the information about the class name is already there in the type converter.

I won’t challenge the convenience here but I’m in favor of a little more type safety. To be fair, a generic ValueObjectConverter won’t be all that type safe as there is no \ValueObjectInterface, but at least it would always return a value object and not sometimes a value object and sometimes a validation result.

That’s a good point I already ran into. Take this - absolutely fictional - example:

final class CredentialsSource implements \Neos\Flow\Security\Value\CredentialsSourceInterface
{
    /**
     * @var string
     */
    protected $credentialsSource;


    public function __construct(string $credentialSource)
    {
        if (mb_strlen($credentialSource) < 8) {
            throw new Validation\Exception('This field must contain at least 8 characters.', 1238108068);
        }
        $this->credentialsSource = $credentialSource;
    }

    public static function fromMatchingArray(array $values)
    {
        if ($values[0] !== $values[1]) {
            throw new Validation\Exception('The two values do not match.', 1505414085);
        }

        return new CredentialsSource($values[0]);
    }


    public function __toString()
    {
        return $this->credentialsSource;
    }
}

If a form field validation fails for multiple reasons (e.g. the given passwords neither match nor are long enough), you’d probably like to display both messages at once. You could pass both exceptions nested as follows:


    public static function fromMatchingArray(array $values)
    {
        try {
            $result = new CredentialsSource($values[0]);
            if ($values[0] !== $values[1]) {
                throw new Validation\Exception('The two values do not match.', 1505414085);
            }
            return $result;
        } catch (Validation\Exception $parentException) {
            if ($values[0] !== $values[1]) {
                throw new Validation\Exception('The two values do not match.', 1505414085, $parentException);
            } else {
                throw $parentException;
            }
        }
    }

which is surely not as convenient (and can be still optimized) but still easily readable for the validation framework.

Almost all non-contextual validators outside Flow itself fall under this category, take Neos’ PackageKeyValidator for example (which actually looks like it belonged to Flow anyway). In Flow itself, the EmailAddressValidator would be the obvious choice, although we actually need an EmailAddress and a NullableEmailAddress to keep the current functionality where you can use email address validation without NotEmpty checks.

Agreed, value objects make no sense as a replacement for contextual validators.

That’s actually an unexpectedly tough one. I see two options here: keep it the way it is or deprecate/remove it without substitute. The latter would be very inconvenient but would forc… encourage our users to think more about what they’re validating in the first place and provide the respective value objects (similar to the nullable email address example above).

bwaidelich · September 15, 2017, 1:52pm

I think I started some misunderstandings, I’ll try to be clearer

It’s not hard, but there is no defined way. But for a start we could map an array to constructor arguments with the same name and scalars to the only constructor argument (which the ScalarTypeToObjectConverter already does).

What I meant is: $this->propertyMapper->convert($someObject, '<someTargetType>') has to know the target type up front or we call jsonSerialize() and find out but I’m not sure if that’s feasible.

Re Exceptions > Result I’m still in doubt about the advantage and esp. about a migration path. For example a common case of a sign up form: I’m all for using Value Objects here (for username, email address, password what not) and they can all validate themselves.
But when I call $this->propertyMapper->convert($formInput, SignUpUserCommand::class) (i.e. if it’s called during argument mapping) I’d expect to get a list of errors.

I didn’t mean the validators itself but places where we use them in the framework instead of VOs. I think we can go over all @Flow\Validate occurrences and replace their respective properties where it makes sense. A few candidates from Flow:

PersistentResource::filename
PersistentResource::mediaType
Account::accountIdentifier
Account::authenticationProviderName

And, yes, the PackageKeyValidator in Neos should definitely be replaced/deprecated.

Can you provide an example for a substitute? I can’t find the nullable email address example you were referring to.

Nezaniel · September 15, 2017, 10:19pm

This is definitely getting more and more fun. I just finished my next iteration and started removing the first workaround implementations that suddenly got obsolete

But let’s start at the top:

Yep, exactly my thought. That’s what I tried hinting at with the following:

Good point, I haven’t given the forward direction too much thought I have to admit. Maybe I underestimated that one but I’m sure we’ll find a solution (maybe check the return type of JsonSerialize? I’d like to avoid reflection, though)

…and you shall have that list! It’s not like we’ll expose validation exceptions to the end user, they are to be caught by the property mapper and converted to a validation result, e.g. like this:

    public function convert($source, $targetType, PropertyMappingConfigurationInterface $configuration = null)
    {
        /* nothing new here */
        } catch (SecurityException $exception) {
            throw $exception;
        } catch (ValidationException $exception) {
            $this->messages->addError($exception->toValidationError());
            while ($exception->getPrevious() instanceof ValidationException) {
                /** @var ValidationException $exception */
                $exception = $exception->getPrevious();
                $this->messages->addError($exception->toValidationError());
            }
            return null;
        } catch (\Exception $exception) {
        /* nothing new here */

I’ve just tested this with the following enhancement of the CredentialsSource:

final class CredentialsSource implements Neos\Flow\Security\Value\CredentialsSourceInterface
{
    /**
     * @var string
     */
    protected $credentialsSource;


    public function __construct(string $credentialSource)
    {
        if (mb_strlen($credentialSource) < 8) {
            throw new Validation\ValidationException('This field must contain at least 8 characters.', 1505510246);
        }
        $this->credentialsSource = $credentialSource;
    }

    public static function fromMatchingValues(array $values): CredentialsSource
    {
        try {
            $result = new CredentialsSource(reset($values));
            self::validateMatchingValues($values);

            return $result;
        } catch (Validation\ValidationException $parentException) {
            self::validateMatchingValues($values, $parentException);
            throw $parentException;
        }
    }

    protected static function validateMatchingValues(array $values, Validation\ValidationException $parentException = null)
    {
        $commonValue = null;
        array_walk($values, function ($value) use (&$commonValue, $parentException) {
            if (!is_null($commonValue) && $value !== $commonValue) {
                throw new Validation\ValidationException('The two values do not match.', 1505414085, $parentException);
            }
            $commonValue = $value;
        });
    }

    public function __toString(): string
    {
        return $this->credentialsSource;
    }
}
```
I've added a small custom type converter for CredentialsSourceInterface (hashing omitted):

public function convertFrom($source, $targetType, array $convertedChildProperties = [], PropertyMappingConfigurationInterface $configuration = null)
{
    $className = $this->objectManager->getClassNameByObjectName(Domain\Value\CredentialsSourceInterface::class);
    if (is_array($source)) {
        return $className::fromMatchingValues($source);
    } else {
        return new $className($source);
    }
}

et voilà:

$passwordElement = $registrationPage->createElement('password', 'Neos.Form:PasswordWithConfirmation');
$passwordElement->setDataType(Domain\Value\CredentialsSource::class);

``works like a charm - provided one removes the PasswordWithConfirmation form element implementation which messes with the precious array we want to validate

Ah, okay, I got you wrong there. I didn’t even think about those yet but yes, sounds totally sensible. What I was hinting at was just the idea to deprecate the validators themselves.

Nope, no substitute in that case. Each field you want to validate but only if anything was inserted at all would require a nullable value object - I haven’t implemented one yet but it should be easy. Just make the constructor arguments nullable, check against null before validation and be good (in theory).

Nezaniel · September 16, 2017, 8:20am

Two more ideas that came up:

We don’t necessarily have to introduce ValidationException, we could alternatively just make ValidationError trowable and let the PropertyMapper catch them directly and add them to the result
I think what has been a little confusing about type conversion is the fact that we currently have TypeConverterInterface implementations for that while value objects are at least partially capable of converting themselves via named constructors, which are methods and not classes. Maybe we’ll have to adjust the type conversion framework accordingly to be able to detect and use those type converter methods.

bwaidelich · September 26, 2017, 10:04am

Hi there, let’s continue with this one

I wonder if we should maybe split this up into two discussions.
Regarding the validation I’m still not convinced that using exceptions (and thus try/catch blocks when collecting them) has an advantage big enough that it’s worth the effort, but I might still miss something.
IMO what we need is a way for (Value)Objects to throw an exception during creation that will be caught by the PropertyMapper and converted to ValidationErrors.

That’s an interesting idea… Unfortunately the Throwable interface is a bit “weird” and we probably wouldn’t be able to fully implement it.

Personally I’m more into the PropertyMapper topic and better support for ValueObjects.

To recap, this is what I would expect from the PropertyMapper to work out-of-the-box (I think):

1. object to array

Converting some object to an array should (recursively) go through the object’s getter in the order they are defined.

Currently they are first sorted alphabetically (which doesn’t do any harm, but loses some semantic/readability IMO).

Also the ArrayFromObjectConverter adds a __type field and a __identity field for entities - this is quite bad in my opinion.

Lastly, sub-objects with only one property (see below) should be “inlined”.

Concretely, if I had:

$fullName = new FullName('Some', 'Name');
$email = new EmailAddress('some@email.com');
$nameAndEmail = new FullNameAndEmailAddress($fullName, $email);
$result = $this->propertyMapper->convert($nameAndEmail, 'array');

I’d expect the result to be

{
    "fullName": {
        "givenName": "Some",
        "familyName": "Name",
    },
    "emailAddress": "some@email.com"
}

but currently it is:

{
    "emailAddress": {
        "value": "some@email.com",
        "__type": "Some\NS\EmailAddress"
    },
    "fullName": {
        "givenName": "Some",
        "familyName": "Name",
        "__type": "Some\NS\FullName"
    },
    "__type": "Some\NS\FullNameAndEmailAddress"
}

2. array to object

The way back from array to a specific object already works as expected (thanks to the ObjectConverter): The array fields are mapped to constructor arguments of the same name, recursively.
And due to the new ScalarTypeToObjectConverter this even works with the “inlined” ValueObjects:

$nameAndEmailArray = [
    'emailAddress' => 'some@email.com',
    'fullName' => [
        'givenName' => 'Some',
        'familyName' => 'Name'
    ]
];
$this->propertyMapper->convert($nameAndEmailArray, FullNameAndEmailAdress::class);
// this works!

3. object to scalar

Converting an object with multiple getters to a string for example should throw an exception (if there is no custom converter of course). And that’s the case already.
But if it’s an object that only “wraps” one value (i.e. a ValueObject with a single “$value” property) I’d expect that to work:

$email = new EmailAddress('some@email.com');
$string = $this->propertyMapper->convert($email, 'string');
// $string should now be 'some@email.com'

4. scalar to object

This already works thanks to the ScalarTypeToObjectConverter

¹ converting objects to a nested object requires some special PropertyMappingConfiguration for security reasons. I’d like to find a better way around this for the cases where that’s not an issue (e.g. ValueObjects) - see related PR