Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Value types/value objects - use clearly defined types for anything that would otherwise be a scalar or composition of scalars #3

Open
wants to merge 2 commits into
base: feature/oop-login-logout
Choose a base branch
from

Conversation

Ocramius
Copy link
Member

This patch introduces value objects into the mix.

What we did is isolating these concepts into these specific well-defined data structures, which would otherwise be strings, and therefore prone to a lot of errors:

  • ClearTextPassword - a password type, designed specifically to never be cast back to string in order to avoid obvious security issues
  • PasswordHash
  • EmailAddress (which is also the concept of "identityfor ourUser` object)

@Ocramius Ocramius added the enhancement New feature or request label Oct 25, 2018
@themark147
Copy link

For these value objects instead creating of own method toString it's not much better to use magic method __toString ?

@Ocramius
Copy link
Member Author

Ocramius commented Feb 4, 2019

@themark147 good question: I prefer to use ->toString() to enforce its explicit usage in the code. I used to write ->__toString() a lot, but then realised that it's supposed to be used in rare technical scenarios (much like Object.toString() is supposed to be used in Java as well).

Here's an example of code that is perfectly valid:

function foo() : string {
    $someVar = SomeValueType::fromSomeInput(...);

    return (string) $someVar;
}

So far, so good, but then over time the code is changed:

function foo() : string {
    $someVar = SomeValueType::fromSomeInput(...);

    if (condition()) {
        $someVar = null;
    }

    return (string) $someVar;
}

Is the second example still valid? Maybe, but the implicit cast obfuscated it, and prevented static analysis from picking up the null pointer exception.

To recap: I avoid language casts as a way to downcast value types, and instead suggest having the cast as an explicit non-magic method on the value type.

EDIT: for the sake of clarity, I'm adding another equivalent example, showing that this has to do with long-term stability, rather than bugs in a single block of code.

Consider following someValue() function.

function someValue() : SomeValueType
{
    return SomeValueType::fromSomeInput(...);
}

We can have two consumers foo() and bar():

function foo() : string
{
    return (string) someValue();
}


function bar() : string
{
    return someValue()->toString();
}

Should someValue() change to following logic due to change in requirements:

function someValue() : ?SomeValueType
{
    if (condition()) {
        return null;
    }

    return SomeValueType::fromSomeInput(...);
}

Now we have bar() crashing (expected), and foo() returning an empty string (incorrect/bug).

By using ->toString() explicitly, in combination with a static analysis checker, this sort of issues can be prevented, and also make the author of someValue() aware of what is going to break in consumers downstream.

@Serhii-DV
Copy link

Serhii-DV commented Feb 5, 2019

@Ocramius, and why do you prefer to use static method for creating object instead of using __construct in value objects?

@Ocramius
Copy link
Member Author

Ocramius commented Feb 5, 2019

I go with named constructors for anything that represents data, because you can only have one __construct (that's what the PHP language imposes), but you will very likely have more and more ways to instantiate the structure.

For example:

  • Username::fromUserInput(...) - applies strict validation
  • Username::fromPersisted(...) - more lax requirements, allowing for past usernames to still be accepted
  • Username::forAccount(...) - infers a username from an existing account

I also make __construct private to prevent misuse that would later on lead to BC breaks in all consumers if the logic or parameters in it change.

@kaystrobach
Copy link

Mhmm i’d go with factories for that.

@Ocramius
Copy link
Member Author

A static method that produces an instance is a factory function. Moving it to a separate class has no technical advantage, and reduces discoverability.

@igorpan
Copy link

igorpan commented May 20, 2019

@kaystrobach @Ocramius and forces __construct to be public

ramsey added a commit to ramsey/uuid that referenced this pull request Nov 30, 2019
…n 4.0.0"

This reverts commit 4c467ce.

For more information, please see the discussion at:
4c467ce#commitcomment-31174263

Also refer to the discussion at:
ShittySoft/symfony-live-berlin-2018-doctrine-tutorial#3 (comment)

TL;DR: I am retaining `toString()` for improved static analysis and
long-term stability.
Baptouuuu added a commit to Innmind/Immutable that referenced this pull request Dec 6, 2019
@asgrim asgrim mentioned this pull request Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants