idiomatic-ar.php

<?php

# Discussion about idiomatic ActiveRecord usage
## Please help me to determine the idiomatic approach for creating a member and attaching an invoice ONLY IF the member doesn't already have 5 invoices.

class Member extends Eloquent {

}

$member = Member::create([
    'email'    => $email,
    'password' => $password,
]);

// Then later, when they are adding invoices

// my gut says that this isn't idiomatic.. suggestions?
if ($member->invoices()->count() >= 5) {
    throw new MemberCannotHaveMoreThanFiveInvoices($member->id);
}

$member->invoices()->create([
    // ...
]);

Regarding the interest creation I prefer doing $member->interests()->create(['name' => 'snowboarding']); on the first example.

Hi @sebdesign, thanks for the feedback.. Sorry that I just massively changed the example from beneath you. But, I'm using your code in an update.

You don't need to call $member->save() the create method saves automatically :)

Why should a new member have more than five instances already anyway? Or is that not necessarily happen in the same place (member creation and invoice creation)?

@franzliedke, yea the concept is mostly to show how consistency is handled. I've clarified this.

Something like this?

<?php
class Member extends Eloquent {

    public static function register(/* Email */ $email, $password) 
    {     
        // No need for ->save(), because the ->create() method does this internally
        return $this->create([
            'email'    => $email,
            'password' => $password,
        ]);
    } 

    public function attachInvoice(Invoice $invoice) 
    {
        $this->guardInvoiceMaximum();

        $this->invoices()->save($invoice);
    }

    public function numberOfInvoices() 
    {
        return $this->invoices()->count();
    }

    protected function guardInvoiceMaximum()
    {
        if ($this->numberOfInvoices() = 5) {
            throw new MemberCannotHaveMoreThanFiveInvoices($this->id);
        }
    }
}

$member = Member::register($email, $password)->attachInvoice($invoice);

Why not give the Member class a getter for that? $member->hasRechachedInvoiceLimit() : bool

Hi @desicochrane,

First of all, thanks for replying.

A few questions:

1. Do you feel like this approach is idomatic?
2. Would you actually have a guard method in your codebase or are you influence by the code sample?
3. Do you rely on Eloquent's exception for the numberOfInvoices() if a Member isn't yet stored in the database?

@NicolasSchneider I think I would only add that getter if there was a very good reason to expose that method to outside of the model. But exposing this, you will probably end up (or at least allowing) yourself to "ask" instead of "tell" your objects what to do, thus performing logic outside of your model that would probably would have been better encapsulated.

i.e. this is not so good:

if ($member->hasRechachedInvoiceLimit()) { // ask something
  $member->addInvoice($invoice); // then do something
}

I prefer the philosophy of "The model is responsible for protecting its own invariants". Then your app can catch the exceptions and decide at an application (not domain) level how to provide feedback to the user.

Not 100% sure about the idiomatic approach but from what I understand
if ($member->invoices()->has(5)) would be only using Eloquent grammar.
But the has method would have to be tweaked as the keys start by 0.

protected function has($key)
{
     return parent::has($key--);
}

@ShaunMcCool

No problem :)

1. I feel like as with every culture, there are many subcultures with their own idioms - and this is certainly a fitting metaphor for the developer communities. For the subculture I run with at least, I feel this is pretty idiomatic.

2. I would have a guard method in this case because it a place to isolate the specifics of a domain rule in a generic manner. I know there is a rule concerning maximum invoices, this rule might be appropriate in other places and the specifics might change later (6 instead of 5 perhaps) now we have one place to do this.

   It's not idiomatic to have models simply be an aggregate of getters and settings (basically a glorified array), instead it is better to attach relevant behaviour and enable the model to responsible for handling its own invariants etc.

3. This is a really good point, and depending on the team I am working with I would decide differently. Right now, if we go through the method above, then the attachInvoice method will protect against the invariant, but there is nothing to stop someone in the team from accessing the $members->invoices()->save logic directly and thus throwing our domain into an invalid state. A robust approach might be to have the guard method invoked in a pre-save eloquent model event hook (further reason to have the guard method in the model).

As @desicochrane pointed out: Someone "creative" could pull up an invoice model from anywhere and just add a new invoice. If you embrace Eloquent, then the pre-save invoice eloquent event (saving) could call a "too many invoices" guard.

I agree with @desicochrane's example, though I probably wouldn't use the email value object that's sort of hinted at.

I think accessing the persistence methods of an active record outside of the active record is a common mistake that I've talked about here (towards the bottom), and in the most recent Full Stack Radio.

I would probably setup the guard like this:

public function attachInvoice(Invoice $invoice) 
{
    if ($this->hasReachedInvoiceLimit()) {
        throw new MemberCannotHaveMoreThanFiveInvoices($this->id);
    }

    $this->invoices()->save($invoice);
}

I don't know if it's "idiomatic" to do things this way; it's probably more common to see people using AR persistence methods all over the place. But I think that's more to do with frameworks like Laravel being very accessible and using AR out of the box. If there was an equally accessible entry point that used Doctrine, I'm sure you'd find a lot of Doctrine misuse that you wouldn't want to call idiomatic ;)

I think of DB access as an implementation detail when using AR. The fact that attaching an invoice involves saving a record to the database is something the outside world doesn't need to know about.

Whatever consumer needed to create the member and invoice (say a controller action) would end up looking like this:

public function store($request)
{
    try {
        $member = Member::register($request->email, $request->password);
        $member->attachInvoice(new Invoice($request->invoice_description, $request->amount);
        return SomeHttpResponse;
    } catch (MemberCannotHaveMoreThanFiveInvoices $e) {
        return SomeOtherHttpResponse;
    }
}

@JeffreyWay went over the same ideas of encapsulating DB access inside of well name AR method in a recent Laracast as well, so I think it's a common approach for less inexperienced developers.

@adamwathan, You're approach is close to what I would suggest, but I prefer the positive case first. What are your thoughts on something like:

public function attachInvoice(Invoice $invoice) 
{
    if ($this->canAttachInvoice()) {
        $this->invoices()->save($invoice);
    }

    throw new MemberCannotHaveMoreThanFiveInvoices($this->id);
}

@zackkitzmiller I usually keep the happy path outdented and guard the less common cases up front, but not religious about it 👍