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([
    // ...
]);

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 👍