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

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 👍