Model Attribute Events

[jpkleemans]

Support listening for attribute changes on a model:

class Payment extends Model
{
    protected $dispatchesEvents = [
        'status:confirmed' => PaymentConfirmed::class,
    ];
}

Like this package: https://attribute.events/

[JustSteveKing]

I very much like this idea

[garygreen]

Probably not a good idea. In your example I would personally have a dedicated ->markConfirmed() method or something on the model. This allows you to better encapsulate your logic surrounding confirmation of payments.

For example, maybe you also want to store who confirmed the payment, or what the reference number is.

$payment->markConfirmed([
   'reference' => 'some_stripe_ref',
   'confirmed_by' => $user,
]);

In application design architecture this is called a "data clump" which is considered a code smell - though relying on individual changes to attributes seems even more prone to errors. Using dedicated objects to represent code clumps seems most recommended.

$confirmation = new PaymentConfirmation([
   'reference' => 'some_stripe_ref',
   'confirmed_by' => $user,
]);
$payment->markConfirmed($confirmation);

For example, what if you had a confirmed_by attribute and a confirmed_payment_amount on your model - what should trigger the PaymentConfirmed event class? What happens if were listening to changes to confirmed_by but fill that first without any of the other attributes that are required when confirming payments?

Overall I think this potentially encourages poor application design.

[jpkleemans]

@garygreen I agree that using dedicated methods like markConfirmed() is very explicit, but it can also lead to very cumbersome controllers.

Say we have the following API call to update a payment:

PATCH /payments/123
{
  "status": "confirmed"
}

You'll have to write your controller/handler/listener something like:

Route::patch('/payments/{payment}', function (Request $request, Payment $payment) {
  $updates = $request->post();
  $payment->fill($updates);

  if (isset($updates['status'])) {
    switch ($updates['status']) {
      case 'confirmed':
        $payment->markConfirmed();
        break;
      case 'expired':
        $payment->markExpired();
        break;

      // Etc...
    }
  }

  $payment->save();

  return $payment;
});

In comparison to something like this when using attribute events:

Route::patch('/payments/{payment}', function (Request $request, Payment $payment) {
  $updates = $request->post();
  $payment->update($updates); // Events will be fired automatically based on changes

  return $payment;
});

[jpkleemans]

@garygreen In addition, to answer your last question:

For example, what if you had a confirmed_by attribute and a confirmed_payment_amount on your model - what should trigger the PaymentConfirmed event class? What happens if were listening to changes to confirmed_by but fill that first without any of the other attributes that are required when confirming payments?

You could write an accessor to only fire the event when given criteria are met:

class Payment extends Model
{
  protected $dispatchesEvents = [
    'is_confirmed:true' => PaymentConfirmed::class,
  ];

  public function getIsConfirmedAttribute(): bool
  {
    return $this->confirmed_by !== null
        && $this->confirmed_payment_amount === $this->total_amount;
  }
}

[garygreen]

@jpkleemans interesting idea with the accessor. I assume that works based on checking all the dispatchesEvents every single time an attribute changes? Sounds like a N+1 problem waiting to happen.

As for your API - I would suggest that patching general attributes like that is possibly poor API design. I most likely would of designed dedicated endpoints changing statuses of payments:

Route::patch('/payments/{payment}/confirm', function (Request $request, Payment $payment) {
  // Validate all details required for confirmation of payment
  $validator = validator(request()->all(), ['confirmed_by' => 'exists:users,id']);

  $payment->markConfirmed($validator->validated());

  return $payment;
});

Seems like a simpler architecture and easier flow to understand - rather than some auxiliary events fired in response to updating attributes on a model. Obviously down to personal preference though. 🤷‍♀️

[garygreen]

Also these lines here:

Route::patch('/payments/{payment}', function (Request $request, Payment $payment) {
  $updates = $request->post();
  $payment->fill($updates);

I hope this is just example code and this isn't representative of what you do in production code? What attributes are fillable on your payment model? Where's the validation? Any user could post and fill any data to your payment model. In addition I assume you check the user even has access to the $payment model?

[jpkleemans]

I assume that works based on checking all the dispatchesEvents every single time an attribute changes? Sounds like a N+1 problem waiting to happen.

After updating a model, it loops over the attribute events in $dispatchesEvents and checks if the attribute has changed using isDirty(). That's very fast, so the performance impact is negligible.

As for your API - I would suggest that patching general attributes like that is possibly poor API design.

Many large and popular APIs use this design (the GitHub API for example). I think it's a straightforward design that encourages generality and visibility.

I hope this is just example code and this isn't representative of what you do in production code?

Yes, it is example code.