analogueorm / analogue

Analogue ORM : Data Mapper ORM for Laravel/PHP
MIT License
633 stars 52 forks source link

[feature request] make mass-assignment possible and easy #67

Closed Evertt closed 9 years ago

Evertt commented 9 years ago

In your example in the REAME.md you show how pretty code can look when you can just do something like $gallery = new Gallery('Trip to India') and $photo = new Photo($file).

However, I notice in daily life with Laravel I rarely use that. I much more often use form-data to populate new entities and I really liked that with Eloquent I could mass-assign that like so: $user = new User($request->all()). And because I had set a $fillable property on User I knew that I wouldn't populate non-existing attributes or special attributes. And even all the right mutators would be called.

I think that use-case is much more likely in daily life. What do you think?

And I took it even one step further. I made my own base-eloquent-model in which I had my own constructor like so:

public function __construct(Request $request)
{
    parent::construct($request->input(class_basename($this)));
}

This way, in my controllers I could do something like:

public function store(CreateUserRequest $request, User $user, Person $person)
{
    $person->save();
    $user->associate($person);
    $user->save();
}

Because just by type-hinting all that stuff I knew that the data had been validated and correctly mass-assigned to the respective models. That is just so freaking convenient, can we please build something similar into Analogue?

In case you're wondering, this is basically what my form would look like:

<form>
    <input name="Person[name]" type="text">
    <input name="Person[infix]" type="text">
    <input name="Person[surname]" type="text">

    <input name="User[email]" type="email">
    <input name="User[password]" type="password">
    <input name="User[password_confirmation]" type="password">
</form>
RemiCollin commented 9 years ago

TBH, I didn't have the opportunity to use the FormRequest so i'm not sure how we could use it with analogue. Can you show me how your CreateUserRequest class looks like ?

Evertt commented 9 years ago

TBH, I haven't tested this either with Analogue yet. I'm getting this from a project where I still used Eloquent. I'm assuming the validator isn't using Eloquent, but simply DB.

<?php

namespace App\Http\Requests;

use App\Http\Requests\Request;

class CreateUserRequest extends Request
{
    /**
     * Determine if the user is authorized to make this request.
     *
     * @return bool
     */
    public function authorize()
    {
        return true;
    }

    /**
     * Get the validation rules that apply to the request.
     *
     * @return array
     */
    public function rules()
    {
        return [
            'Person.name' => 'required',
            'Person.infix' => 'required',
            'Person.surname' => 'required',
            'User.email' => 'required|email',
            'User.password' => 'required|confirmed'
        ];
    }
}
RemiCollin commented 9 years ago

Ok, then my question is, where are the User and Person object created ? Do you use a Route <-> Model binding or so ?

Evertt commented 9 years ago

Nope, that's not necessary in this case. I am only using the automatic dependency injection feature of laravel, nothing more.

So laravel sees my controller-action and looks at the arguments. It sees I'm type-hinting an App\Http\Requests\CreateUserRequest object, an App\User object and an App\Post object. So it instantiates those objects and then it looks into the constructors of those objects to see if they need further dependency injection. And I've put into all my Eloquent models' constructors that they need the Illuminate\Http\Request object, so Laravel gives it an instance of that and then the model populates its own attributes with it. Get it?

RemiCollin commented 9 years ago

Ok yes, I got it now, quite a smart use of the method injection BTW. :)

I'll think about the possibility of mass assignment on the Entity object, but it already raises some issues to my mind :

1) Entity has no construct() method, and it's encouraged for the user to use this one to ensure object's integrity (it's one of Domain Driven Design top principle). Using a generic construct() method wouldn't ensure this.

2) We could add a ::create($attributes) method with a $fillable property to the Entity class, which would work as long as the user doesn't use a custom __construct() method, and would fails otherwise.

Also, there's a big pitfall I can in your approach to me. What if you need to create objects elsewhere that within a form request (ie: using FactoryBuilder) ? By injecting the Request object into your base model's constructor you're tightly bounding it to it, which doesn't feels right.

That said, you can totally do this with analogue, by creating your own base Entity class :


class BaseEntity extends Entity {

    protected $fillable = [];

    public function __construct(Request $request)
    {
        $this->attributes = array_only($request->all(), $this->fillable));
    }

}
Evertt commented 9 years ago

Also, there's a big pitfall I can in your approach to me. What if you need to create objects elsewhere that within a form request (ie: using FactoryBuilder) ? By injecting the Request object into your base model's constructor you're tightly bounding it to it, which doesn't feels right.

Yes I just submitted a pull-request to the laravel framework and there somebody pointed me to this same problem.

We could make a static constructor, like so:

// In Analogue\ORM\Entity.php

public static function createFromRequest(Request $request)
{
    $entity = new static;
    $attributes = $request->input(class_basename($this), []);
    $entity->setEntityAttributes($attributes);

    return $entity;
}

So that in your controller you'd do:

public function store(CreateUserRequest $request)
{
    $user = User::createFormRequest($request);
    mapper($user)->store($user);
}

Not the prettiest thing in the world though.

RemiCollin commented 9 years ago

Same issue :

$entity = new static;

Would fail as soon as an Entity has a custom constructor.

Evertt commented 9 years ago

Aaah right, now I get it, I forgot about that.

Evertt commented 9 years ago

Now that I think about it, there could be great power in automatically interpreting form-data. For example, if I write a form like this:

<form>
    <input name="User[person][identity][name]" type="text">
    <input name="User[person][identity][infix]" type="text">
    <input name="User[person][identity][surname]" type="text">
    <input name="User[person][birthday]" type="date">

    <input name="User[email]" type="email">
    <input name="User[password]" type="password">
    <input name="User[password_confirmation]" type="password">

    <p>And finally, give yourself some tags? This is for demonstrating a BelongsToMany relationship<p>
    <input name="User[person][tags][]" type="checkbox" value="hot">
    <input name="User[person][tags][]" type="checkbox" value="cool">
</form>

Seriously, all the necessary information is right in the form. It will come out like this:

screen shot 2015-09-26 at 16 29 07

So why do we need to manually assign them to the right entities in the controller? All the information is right there! And is guaranteed to be valid by the CreateUserRequest. So I wish there would be some automagic to handle that all.

Evertt commented 9 years ago

Hey Rémi, maybe you can show your support here if you agree with me. :-)

Evertt commented 9 years ago

Would fail as soon as an Entity has a custom constructor.

Hey now that I think about it... how do you create entities from a query? since you can't use the constructor?

edit oh woa, I see you're using a neat trick for that. Well now I'm pretty sure we could use that same trick for the Post::create() method, don't you think?

RemiCollin commented 9 years ago

It uses a trick from : https://github.com/doctrine/instantiator

So yes it would be technically possible. But we shouldn't use it for pure object creation, let me explain why:

When the mapper instantiate and hydrate an Entity from the result of a query, it restituates the Entity in the state it was when it was previously stored in the application (well more or less if we take the proxies into account, but you get the idea). This assumes it was created before using whatever method (__construct, static method, factory class, etc..), which methods ensure the domain integrity is safe.

In DDD, the domain itself should be responsible for its own integrity. So bypassing the constructor method for object creation would mean bypassing this 'integrity check', which would be hazardous, at best, and at worse, a security issue. Also, this 'integrity check' shouldn't be substitued for input validation. Those are two distincts things. Validation should be here to ensure your object's creation won't fail, in the same fashion we use JS validation before sending a POST request.

There was a great talk about this on youtube, don't remember how it was called though, i'll try to find it back (wasn't PHP, was Ruby I think)

Evertt commented 9 years ago

Okay, I think I understand that and I don't see a problem yet. I mean, whatever method we create on the entity for this process can be built to explicitly check the entity's integrity right?

Evertt commented 9 years ago

Oh wait, now I understand better. We can't make a generic function to check the Entity's integrity, because different things will need to be checked for different entities, right?

Hmm, that sucks.

RemiCollin commented 9 years ago

Well, there's nothing preventing you to build a generic factory class that suits your workflow, by defining some kind of interface, maybe. Just don't use constructors and use Entities as simple data container. That's totally ok if your domain doesn't contains too much logic, as would an ecommerce platform for example.

I think the flexibility has to be left to the developer for this.