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

[5.3] Implement object casting #13706

Closed
wants to merge 3 commits into from
Closed

[5.3] Implement object casting #13706

wants to merge 3 commits into from

Conversation

taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented May 25, 2016

This PR implements the ability to cast Eloquent values to arbitrary value objects. You may also cast an "attribute" that doesn't actually exist in the database which will allow you to create aggregate value objects that consist of multiple actual fields. Modifying the value object will affect the base model's attributes as you access them, so the model will always "feel" updated.

Value objects MUST implement two methods:

class User extends Eloquent
{
    protected $casts = ['address' => Address::class];
}


class Address
{
    public $lineOne, $lineTwo;

    public function __construct($lineOne, $lineTwo)
    {
        $this->lineOne = $lineOne;
        $this->lineTwo = $lineTwo;
    }

    public static function fromModelAttributes($model, $attributes)
    {
        return new static($attributes['line_one'], $attributes['line_two']);
    }

    public function toModelAttributes()
    {
        return [
            'line_one' => $this->lineOne,
            'line_two' => $this->lineTwo,
        ];
    }
}

The fromModelAttributes will be called when Eloquent needs to create an instance of the value object. The model instance and raw attribute array will be passed to the method and you may construct the value object however you wish. You have total freedom here.

The toModelAttributes method will be called by Eloquent when it needs to persist the value object's state back into the model. In this method you should return an array that maps back onto the Eloquent object's actual attributes that are stored in the database.

Using the example value object above, the following is perfectly valid:

$user->address->lineOne = 'New Line 1';
$user->save();

Eloquent will sync the value object's state back into its attributes using the toModelAttributes automatically before saving.

If a value object is set to null, all of the attributes returned by toModelAttributes will be set to null:

$user->address = null;
$user->save();

$user->line_one; // null
$user->line_two; // null

When a value object cast object is resolved, the same instance will be returned each time you access it:

$user->address
$user->address // same instance

@Francismori7
Copy link
Contributor

I like it. Very much.

@daftspunk
Copy link
Contributor

I like it, because as always Taylor, I didn't need to read a single word you wrote. The code sells itself.

@pstephan1187
Copy link
Contributor

Assuming this behaves as expected when casting the object to JSON, I think it's great!

@taylorotwell
Copy link
Member Author

taylorotwell commented May 25, 2016

@pstephan1187 you can implement Arrayable on your value objects and they will be cast to JSON correctly.

@DanielDarrenJones
Copy link

For a moment I was like hmm, this seems ok... Then it clicked. THIS IS AMAZING.

@alexbowers
Copy link
Contributor

alexbowers commented May 25, 2016

Does everything get casted to a string? or would it be possible to cast to say, integers, booleans, etc?

Edit: I guess it'd be part of the fromModelAttributes method?

@robertscholts
Copy link

robertscholts commented May 25, 2016

Wouw awesome! (y)

@taylorotwell
Copy link
Member Author

Alex, things are cast to whatever you want. But I'm not sure I understand the question.

On Wed, May 25, 2016 at 3:13 PM -0700, "Alex Bowers" notifications@github.com wrote:

Does everything get casted to a string? or would it be possible to cast to say, integers, booleans, etc?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@alexbowers
Copy link
Contributor

It is my understanding, that since for example booleans aren't a thing in mysql, but instead it is an integer, there is currently the value of either 0 or 1 coming back. Whereas, it'd be cleaner to see true or false as their true values.

One thing you can currently do in Eloquent is use

$casts = [
    'billing' => 'boolean',
];

If you want, for example to have an address and one of the fields is billing (boolean), how would you ensure it is a boolean, whilst also casting into an 'address' with $address->billing returning as a boolean, not an integer.

@alexbowers
Copy link
Contributor

You state that

Value objects MUST implement two methods

But your code example doesn't show an implements contract. Will one be enforced for this?

@taylorotwell
Copy link
Member Author

I don't see a large need for interface because your code will error anyways if you don't define the methods. So adding an interface is just changing the error message, not really doing anything useful and it's just another file to load off of disk. 

On Wed, May 25, 2016 at 3:21 PM -0700, "Alex Bowers" notifications@github.com wrote:

You state that

Value objects MUST implement two methods

But your code example doesn't show an implements contract. Will one be enforced for this?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@alexbowers
Copy link
Contributor

Will one be available to use, for those of us that like to use them to ensure that methods are setup properly, and to make the code headers read well?

@tpavlek
Copy link
Contributor

tpavlek commented May 25, 2016

I really like this and would be happy as is, but I agree with @alexbowers here that I would really like an interface.

I know it might not be very zonda, but I feel like since we have the tools to make the contract explicit, might as well.

Plus it opens up interoperability via illuminate/contracts so I don't necessarily need to depend on all of Laravel to provide decent value objects, but I can show that they're compatible.

@ravanscafi
Copy link

ravanscafi commented May 25, 2016

The approach taken by Mongolid, an ODM for MongoDB, designed by @Zizaco works "out of the box" (without the need for a new interface, using fiil and toArray) as this:

class User extends Eloquent
{
    public function address()
    {
        return $this->embedsOne(Address::class, 'address'); // class name, field name
    }
}

The attribute 'address' holds the object serialized to an array, eg lineOne and lineTwo, so you can invoke the field directly and retrieve the array.
The magic is that calling $user->address() will do something like it:

$address = new Address;
$address->fill($user->address);
return $address;

Also, calling $user->embed('address', $address); will cast that $address to array and save to the attribute.

There's a lot more logic involved but the main concept is this.

It might be a way to go about it.

For reference: http://leroy-merlin-br.github.io/mongolid/relationships/

@taylorotwell
Copy link
Member Author

An interface adds no value though. It just changes the error message. Does anyone have a use case for the interface other than changing the error message. I don't really want to get distracted by something so minor so maybe lets focus on actual implementation.

On Wed, May 25, 2016 at 3:41 PM -0700, "Troy Pavlek" notifications@github.com wrote:

I really like this and would be happy as is, but I agree with @alexbowers here that I would really like an interface.

I know it might not be very zonda, but I feel like since we have the tools to make the contract explicit, might as well.

Plus it opens up interoperability via illuminate/contracts so I don't necessarily need to depend on all of Laravel to provide decent value objects, but I can show that they're compatible.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@mitchellvanw
Copy link
Contributor

mitchellvanw commented May 25, 2016

Although I love the idea of having a embeddables feature, like Doctrine 2 has, I don't like this implementation. It's too intrusive, but I guess this is due to Eloquent's fundamental of being kinda magic-ish. I don't like having this behaviour, as actual code, in my value objects (VOs). There's a difference between the configuration and not intrusive way that Doctrine 2 uses and creating actual public methods on a value object that can be called from anywhere. Since it's just an array that's passed in, you'd have to additional validation just to check if the array has all the correct values. Unlike when you use a normal or named constructor (factory method) in which you get implicit validation and you create a stronger boundary.

With the current implementation there's no way to use one of the constructors on the VO that might have the needed validation to keep it consistent and valid.

In my opinion this type of feature doesn't fit on Eloquent. The idea behind this concept of a VO, which is literally just a good ol' object, is to package certain behaviour up in neat little chunk of code that will always have the same, it'll always be deterministic and thus immutable, but because of Eloquent's inherent openness it doesn't seem like a good fit.

A better (seeming) solution could simply be to make a method on the model that instantiates the VO.
Literally something like:

public function address() {
    return new Address($this->lineOne, $this->lineTwo);
}

Edit 1:
A value object is only for reading.

@alexbowers
Copy link
Contributor

I have to agree, having something similar to the current way that relationships works would be pretty neat.

To me, casting to an object seems odd (i think). it feels more like what the relationships setup, in terms of the properties created and such.

@mitchellvanw
Copy link
Contributor

mitchellvanw commented May 25, 2016

@alexbowers except for that this doesn't need any framework support. A value object is simply just, an object. It shouldn't need outside support. If it does, if it's not boxed off (or so to speak), then there's something wrong.

@mikebronner
Copy link
Contributor

mikebronner commented May 25, 2016

Taking the address example, this could be solved by creating getAddressAttribute() and setAddressAttribute() mutators on the user model.

It appears then this is attempting to refactor that multi-value getter and setter out to its own VO to simplify the model? (Is that the intended problem this PR is trying to solve?)

I like it, as I have done the mutator methods many times in the past and it always felt convoluded and dirty.

Update: thinking on this some more, really the only place this makes sense is in one of two situations:

  • you have objects serialized in JSON fields in your database.
  • your database tables aren't normalized.

I'm thinking that this concept is best used only after thinking long and hard if it is the right solution.

@mitchellvanw
Copy link
Contributor

mitchellvanw commented May 25, 2016

@mikebronner could you give me a concrete example of what these methods could look like? To me, it all just seems like this can be handled by a simple method on the model itself, just code, not anything the framework has to do.

@mitchellvanw
Copy link
Contributor

Eloquent doesn't need to able to change the properties on the VO, because they're immutable. If they're mutable, there's no need to use a value object.

@mikebronner
Copy link
Contributor

@mitchellvanw using the address example above:


namespace App;

use Illuminate\Foundation\Auth\User as Authenticatable;

class User extends Authenticatable
{
    protected $appends = [
        'address',
    ];
    /**
     * The attributes that are mass assignable.
     *
     * @var array
     */
    protected $fillable = [
        'name', 'email', 'password',
    ];

    /**
     * The attributes that should be hidden for arrays.
     *
     * @var array
     */
    protected $hidden = [
        'password', 'remember_token',
    ];

    public function getAddressAttribute()
    {
        return new Address([
            'street' => $this->streetAddress,
            'city' => $this->city,
            'state' => $this->state,
        ]);
    }

    public function setAddressAttribute(Address $address)
    {
        $this->street = $address->streetAddress;
        $this->city = $address->city;
        $this->state = $address->state;
    }
}

And the Address class would be a simple VO, or perhaps a non-eloquent model using jenssegers/model.

@torkiljohnsen
Copy link

I would love to see this in Laravel. I must say I like the Doctrine embeddables a bit better. Here you have to define both the constructor and two other methods to make it work. Also: When you add up city/zip for your address there are suddenly four fields that need to be handled and it will get messy with more method parameters and more boilerplate code to be written. The address example is what Symfony uses as well, so it's tempting to compare…

Also: PHPMD will squawk at the unused $model parameter in fromModelAttributes() ;)

All in all though, I would love to see this feature in Laravel.

@taylorotwell
Copy link
Member Author

There an other ways to inject the constructor properties. For example we could allow defining which attributes should be injected in the cast definition.
Address:line_one,line_two
etc...

On Wed, May 25, 2016 at 5:02 PM -0700, "Torkil Johnsen" notifications@github.com wrote:

I would love to see this in Laravel. I must say I like the Doctrine embeddables a bit better. Here you have to define both the constructor and two other methods to make it work. Also: When you add up city/zip for your address there are suddenly four fields that need to be handled and it will get messy with more method parameters and more boilerplate code to be written. The address example is what Symfony uses as well, so it's tempting to compare…

Also: PHPMD will squawk at the unused $model parameter in fromModelAttributes() ;)

All in all though, I would love to see this feature in Laravel.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@taylorotwell
Copy link
Member Author

Eh nevermind that won't be that great.

On Wed, May 25, 2016 at 5:14 PM -0700, "Taylor Otwell" taylorotwell@gmail.com wrote:

There an other ways to inject the constructor properties. For example we could allow defining which attributes should be injected in the cast definition.
Address:line_one,line_two
etc...

On Wed, May 25, 2016 at 5:02 PM -0700, "Torkil Johnsen" notifications@github.com wrote:

I would love to see this in Laravel. I must say I like the Doctrine embeddables a bit better. Here you have to define both the constructor and two other methods to make it work. Also: When you add up city/zip for your address there are suddenly four fields that need to be handled and it will get messy with more method parameters and more boilerplate code to be written. The address example is what Symfony uses as well, so it's tempting to compare…

Also: PHPMD will squawk at the unused $model parameter in fromModelAttributes() ;)

All in all though, I would love to see this feature in Laravel.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@taylorotwell
Copy link
Member Author

taylorotwell commented May 26, 2016

@mitchellvanw I'm not sure if your argument that you won't have your constructor validation is valid.

You said this:

With the current implementation there's no way to use one of the constructors on the VO that might have the needed validation to keep it consistent and valid.

In PHP 7 you could just do this:

return new static(
     $attributes['line_one'] ?? null,
     $attributes['line_two'] ?? null,
);

You don't really need to do any additional validation if the attributes exist in the mapping method because you can just send null into the constructor if they don't and let your constructor barf an error if it doesn't like null.

So, I'm not sure it's correct to say you need to check if the attributes exist. All you're doing in this method is creating a new instance of the object. No validation is needed. You can still let your constructor do all that.

@taylorotwell taylorotwell changed the title implement value object casting implement object casting May 26, 2016
@rphcrosby
Copy link

@taylorotwell I think regardless, we are talking about value objects here and there are advantages to them being immutable.

@halaei
Copy link
Contributor

halaei commented May 27, 2016

@taylorotwell Sometimes it does not worth the trouble of having a database-design in the first normal form. When the decision made, right or wrong, I am going to have composite values in some columns, e.g. using json. Each json value in a column has its own schema, and to detect what is the schema, one solution is to have another column, namely type. It would be very nice to be able to convert those json objects to the appropriate php objects based on the type column:

Example

Suppose you need to make a survey-monkey-like application. There are different types of questions in a single questionnaire, but in your app, the idea of having one or multiple tables per each question type is an overkill. So here is what the questions table looks like:

id questionnaire_id question_type schema
1 1 short-answer {title: "first name"}
2 1 long-answer {title: "what is your address?"}
3 1 multiple-choice {title: "Do you like ice-cream?", answers: ['yes', 'no']}
4 1 column-matching {title: "Match the English words in the left with their Farsi meanings in the right", column1: ["cat", "dog", "chicken", "horse"], column2: ["اسب", "گربه", "مرغ", "سگ"]}

And in the PHP side, I would like to not have any switch-case at all when I need to, for example validate an answer against the question schema:

abstract class QuestionSchema
{
    public abstract function validationRules();
    public function validate($answer)
    {
        //apply the validation rules against the answer
    }
}

class ShortAnswerQuestionSchema
{
    public function validationRules()
    {
        return ["answer" => "string|max:64"];
    }
}
class ColumnMatchingQuestionSchema
{
     protected colum1;
     protected column2;
    public function validationRules()
    {
        return [
                       "answer" => "array|size:"count($this->column1),
                       "answer.*" => "in:".toCSV($this->column2);
       ];
    }
}

So when I have an $answer to be validated against question 10, I will do this:

Question::where('id', 10)->schema->validate($answer);

No switch-case at all 😄
Lets do that please.

@melloc01
Copy link
Contributor

Definitely any class that implements these two methods should be implementing an EloquentCastingInterface @taylorotwell

@taylorotwell
Copy link
Member Author

All discussion of an interface can basically stop because it’s not going to happen, heh. There is no point to making an interface other to calm people’s superstitions that it somehow makes the code better. It doesn’t. Period. So, you can discuss other things here but not that :)

@joshbrw
Copy link

joshbrw commented May 27, 2016

@taylorotwell You wouldn't know this as you don't use an IDE, but using an Interface helps massively when creating new code, as the stub methods are added automatically.

@mikebronner
Copy link
Contributor

@joshbrw I can understand why that seems like a reasonable requirement, but coding to satisfy the functionality of an IDE is fundamentally wrong. What do you do with all your specialized code if you switch IDEs or your IDE changes behavior. That's a no-win scenario for me.

@taylorotwell
Copy link
Member Author

Going to table this for now as it's apparently fairly divisive and I'm not sure I want to take on the complexity in Eloquent for something that isn't more unanimous.

@torkiljohnsen
Copy link

@taylorotwell I think it's sad that this was closed… For a proper example though, try using Google's Address Data Service format when making a value object. It has roughly these fields:

  • Country
  • Administrative area
  • Locality (City)
  • Dependent Locality
  • Postal code
  • Sorting code
  • Address line 1
  • Address line 2
  • Organization
  • Recipient

I might have said earlier that it get's messy with a lot of getters, setters and a huge constructor, but I'm going back on that as I see it as more important to get this feature into the codebase.

Check out what the value object looks like in commerceguys/addressing, in their Address class, which can basically be used as an embeddable in Doctrine.

Ross Tuck also has a nice article on the embeddables feature in Doctrine, which he calls "the promised land".

I think that says it all. Please reconsider! :)

tillkruss added a commit to tillkruss/framework that referenced this pull request Feb 24, 2017
tillkruss added a commit to tillkruss/framework that referenced this pull request Feb 24, 2017
tillkruss added a commit to tillkruss/framework that referenced this pull request Feb 24, 2017
tillkruss added a commit to tillkruss/framework that referenced this pull request Mar 2, 2017
tillkruss added a commit to tillkruss/framework that referenced this pull request Mar 4, 2017
tillkruss added a commit to tillkruss/framework that referenced this pull request Mar 16, 2017
tillkruss added a commit to tillkruss/framework that referenced this pull request Mar 25, 2017
tillkruss added a commit to tillkruss/framework that referenced this pull request May 6, 2017
tillkruss added a commit to tillkruss/framework that referenced this pull request Jun 25, 2017
tillkruss added a commit to tillkruss/framework that referenced this pull request Jul 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet