Find out Whether Model Observers in Laravel are a Bad Practice

Written by dkhorev | Published 2022/11/10
Tech Story Tags: laravel | php | phpunit | design-patterns | web-development | programming | backend | antipattern

TLDRLaravel provides an interesting way to automate common model events inside your app with [dispatched events, closure events, and observers]. While it sounds cool to have this kind of plug-and-play solution — there are certain cases when this will backfire on your project if you tend to overload this feature with business logic.via the TL;DR App

Laravel provides an interesting way to automate common model events inside your app with dispatched events, closure events, and observers.

While it sounds cool to have this kind of plug-and-play solution — there are certain cases when this will backfire on your project if you tend to overload this feature with business logic.

TL;DR

  • I think observers and model events are fine for MVP and/or smaller projects.
  • When you have 2+ devs working and/or 100+ test cases — they can become an issue (not absolutely will).
  • For very large projects that will be an issue for sure. You would need to spend a lot of time refactoring, QAing, and regress-testing your app. So think ahead and refactor early.
  • Reason: model events create hidden side effects, sometimes unexpected and not required by the executed action.

The most common side effects can be observed when writing and running Unit tests and Feature tests for your Laravel app. This article will demonstrate this scenario.

Our example

Processing temperature measures from IoT devices, storing them in a database and doing some additional calculations after each sample is consumed.

Our business requirements:

  • store a sample consumed via exposed API
  • for each stored sample update and average temperature for the last 10 measures

This is our Sample model and migration:

<?php

declare(strict_types=1);

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

return new class extends Migration {
    public function up(): void
    {
        Schema::create('samples', static function (Blueprint $table) {
            $table->id();
            $table->string('device_id');
            $table->float('temp');
            $table->timestamp('created_at')->useCurrent();
        });
    }

    public function down(): void
    {
        Schema::dropIfExists('samples');
    }
};


<?php

declare(strict_types=1);

namespace App\Models;

use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Database\Eloquent\Model;

class Sample extends Model
{
    use HasFactory;

    public $timestamps = false;

    protected $fillable = [
        'device_id',
        'temp',
        'created_at',
    ];
}

Now every time we store a sample we want to store the average temperature for the last 10 samples in another model, Avg Temperature:

<?php

declare(strict_types=1);

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

return new class extends Migration {
    public function up(): void
    {
        Schema::create('avg_temperatures', static function (Blueprint $table) {
            $table->id();
            $table->string('device_id');
            $table->float('temp');
            $table->timestamps();
        });
    }

    public function down(): void
    {
        Schema::dropIfExists('avg_temperatures');
    }
};


<?php

declare(strict_types=1);

namespace App\Models;

use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Database\Eloquent\Model;

class AvgTemperature extends Model
{
    use HasFactory;

    protected $fillable = [
        'device_id',
        'temp',
    ];
}

We can achieve this simply by attaching an event to the created state of the Sample model:

<?php

declare(strict_types=1);

namespace App\Models;

use App\Events\SampleCreatedEvent;
use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Database\Eloquent\Model;

class Sample extends Model
{
    use HasFactory;

    public $timestamps = false;

    protected $fillable = [
        'device_id',
        'temp',
        'created_at',
    ];

    /**
     * @var array<string, string>
     */
    protected $dispatchesEvents = [
        'created' => SampleCreatedEvent::class,
    ];
}

Now we add a listener with average recalculation logic:

class EventServiceProvider extends ServiceProvider
{
    /**
     * @var array<string, array<string>>
     */
    protected $listen = [
        SampleCreatedEvent::class => [
            RecalcAvgTemperatureListener::class,
        ],
    ];
}


<?php

declare(strict_types=1);

namespace App\Listeners;

use App\Events\SampleCreatedEvent;
use App\Models\AvgTemperature;
use App\Models\Sample;

class RecalcAvgTemperatureListener
{
    public function handle(SampleCreatedEvent $event): void
    {
        $average = Sample::orderByDesc('created_at')
            ->limit(10)
            ->avg('temp');

        AvgTemperature::updateOrCreate([
            'device_id' => $event->sample->device_id,
        ], [
            'temp' => $average ?? 0,
        ]);
    }
}

Now, our naive controller implementation, **skipping validation and any good development patterns**, would look like this:

<?php

declare(strict_types=1);

namespace App\Http\Controllers;

use App\Models\Sample;
use Illuminate\Http\Request;

class SampleController extends Controller
{
    public function store(Request $request): void
    {
        Sample::create(
            array_merge($request->all(), ['created_at' => now()])
        );
    }
}

We can also write a feature test that confirms that our API route works as expected — sample is stored and avg sample is stored:

<?php

declare(strict_types=1);

namespace Tests\Original;

use App\Models\AvgTemperature;
use App\Models\Sample;
use Tests\TestCase;

class SampleControllerTest extends TestCase
{
    /** @test */
    public function when_sample_is_sent_then_model_is_stored(): void
    {
        // act
        $this->post('/sample', [
            'device_id' => 'xyz',
            'temp'      => 10.5,
        ]);

        // assert
        $sample = Sample::first();
        $this->assertSame('xyz', $sample->device_id);
        $this->assertSame(10.5, $sample->temp);
    }

    /** @test */
    public function when_sample_is_sent_then_avg_model_is_stored(): void
    {
        Sample::factory()->create(['device_id' => 'xyz', 'temp' => 20]);

        // act
        $this->post('/sample', [
            'device_id' => 'xyz',
            'temp'      => 10,
        ]);

        // assert
        $sample = AvgTemperature::first();
        $this->assertSame('xyz', $sample->device_id);
        $this->assertSame(15.0, $sample->temp);
    }
}

That looks perfectly fine, right?

Now when things go wrong

Imagine a second developer on your team is going to write a Unit test where he wants to check average temperature calculations.

He extracts a service from the listener class to do this job:

<?php

declare(strict_types=1);

namespace App\Listeners;

use App\Events\SampleCreatedEvent;
use App\Services\AvgTemperatureRecalcService;

class RefactoredRecalcAvgTemperatureListener
{
    public function __construct(protected AvgTemperatureRecalcService $recalcAvg)
    {
    }

    public function handle(SampleCreatedEvent $event): void
    {
        $this->recalcAvg->withLatestTenSamples($event->sample);
    }
}


<?php

declare(strict_types=1);

namespace App\Services;

use App\Models\AvgTemperature;
use App\Models\RefactoredSample;
use App\Models\Sample;

class AvgTemperatureRecalcService
{
    public function withLatestTenSamples(Sample|RefactoredSample $sample): void
    {
        $average = Sample::where('device_id', $sample->device_id)
            ->orderByDesc('created_at')
            ->limit(10)
            ->pluck('temp')
            ->avg();

        AvgTemperature::updateOrCreate([
            'device_id' => $sample->device_id,
        ], [
            'temp' => $average ?? 0,
        ]);
    }
}

He has this unit test written where he wants to seed 100 samples at once at 1-minute intervals:

<?php

declare(strict_types=1);

namespace Tests\Original;

use App\Models\AvgTemperature;
use App\Models\Sample;
use App\Services\AvgTemperatureRecalcService;
use Tests\TestCase;

class AvgTemperatureRecalcServiceTest extends TestCase
{
    /** @test */
    public function when_has_existing_100_samples_then_10_last_average_is_correct(): void
    {
        for ($i = 0; $i < 100; $i++) {
            Sample::factory()->create([
                'device_id'  => 'xyz',
                'temp'       => 1,
                'created_at' => now()->subMinutes($i),
            ]);
        }
        $sample = Sample::factory()->create(['device_id' => 'xyz', 'temp' => 11, 'created_at' => now()]);

        // pre assert
        // this will FAIL because average was already recounted 100x times when factory was creating 100x samples
        $this->assertCount(0, AvgTemperature::all());

        // act
        $service = new AvgTemperatureRecalcService();
        $service->withLatestTenSamples($sample);

        // assert
        $avgTemp = AvgTemperature::where('device_id', 'xyz')->first();
        $this->assertSame((float)((9 + 11) / 10), $avgTemp->temp);
    }
}

This is a pretty simple example and can be fixed by disabling the model event or faking the whole Event facade on an ad-hoc basis.

Event::fake();
// or
Sample::unsetEventDispatcher();

For any more or less large project such options are painful — you always need to remember your model creates side effects.

Imagine such an event creates side effects in another database or an external service via API call. Every time you create a sample with a factory you’d have to deal with mocking external calls.

We have a combination of a bad development pattern of model events and not enough code decoupling.

Refactoring and decoupling our example

For better visibility, we will create a second set of models in our project and a new route.

First, we remove the model event from our Sample model, now it looks like this:

<?php

declare(strict_types=1);

namespace App\Models;

use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Database\Eloquent\Model;

class RefactoredSample extends Model
{
    use HasFactory;

    protected $table = 'samples';

    public $timestamps = false;

    protected $fillable = [
        'device_id',
        'temp',
        'created_at',
    ];
}

Then we create a service that will be responsible for consuming new samples:

<?php

declare(strict_types=1);

namespace App\Services;

use App\Events\SampleCreatedEvent;
use App\Models\DataTransferObjects\SampleDto;
use App\Models\RefactoredSample;

class SampleConsumeService
{
    public function newSample(SampleDto $sample): RefactoredSample
    {
        $sample = RefactoredSample::create([
            'device_id'  => $sample->device_id,
            'temp'       => $sample->temp,
            'created_at' => now(),
        ]);

        event(new SampleCreatedEvent($sample));

        return $sample;
    }
}

Notice our service is now responsible for firing an event in case of success.

Our new route handler will look like this:

<?php

declare(strict_types=1);

namespace App\Http\Controllers;

use App\Http\Requests\StoreSampleRequest;
use App\Models\DataTransferObjects\SampleDto;
use App\Services\SampleConsumeService;

class SampleController extends Controller
{
    public function storeRefactored(StoreSampleRequest $request, SampleConsumeService $service): void
    {
        $service->newSample(SampleDto::fromRequest($request));
    }
}

Request class:

<?php

declare(strict_types=1);

namespace App\Http\Requests;

use Illuminate\Foundation\Http\FormRequest;

/**
 * @property-read string $device_id
 * @property-read string|float|int $temp
 */
class StoreSampleRequest extends FormRequest
{
    public function authorize(): bool
    {
        return true;
    }

    /**
     * @return array<string, array<string>>
     */
    public function rules(): array
    {
        return [
            'device_id' => ['required', 'string'],
            'temp'      => ['required', 'numeric'],
        ];
    }
}

Now we replicate our second developer’s tests with the new route and can confirm it passes:

<?php

declare(strict_types=1);

namespace Tests\Refactored;

use App\Models\AvgTemperature;
use App\Models\RefactoredSample;
use App\Services\RefactoredAvgTemperatureRecalcService;
use Tests\TestCase;

class AvgTemperatureRecalcServiceTest extends TestCase
{
    /** @test */
    public function when_has_existing_100_samples_then_10_last_average_is_correct(): void
    {
        for ($i = 0; $i < 100; $i++) {
            RefactoredSample::factory()->create([
                'device_id'  => 'xyz',
                'temp'       => 1,
                'created_at' => now()->subMinutes($i),
            ]);
        }
        $sample = RefactoredSample::factory()->create(['device_id' => 'xyz', 'temp' => 11, 'created_at' => now()]);

        // pre assert
        $this->assertCount(0, AvgTemperature::all());

        // act
        $service = new RefactoredAvgTemperatureRecalcService();
        $service->withLatestTenSamples($sample);

        // assert
        $avgTemp = AvgTemperature::where('device_id', 'xyz')->first();
        $this->assertSame((float)((9 + 11) / 10), $avgTemp->temp);
    }
}

Conclusion

What was improved:

  • We decoupled our controller from the database model.
  • We decoupled sample processing (business logic) from the framework.
  • The firing of SampleCreatedEvent is more controllable and will not trigger when not expected.

How this helps:

  • Developers are happier when working with your code.
  • You can now mock sample processing when testing the sample controller.
  • CI/CD runs faster and costs less as we don’t do unnecessary work (valid for large projects).

The repository with code can be found here: https://github.com/dkhorev/model-observers-bad-practice.

Also Published Here

Written by dkhorev | Sharing my ideas on software development (NestJS, Laravel, System Design, DevOps for backend engineers)
Published by HackerNoon on 2022/11/10
ABOUTHow hackers start their afternoons. We publish tech stories and build publishing software.

READEntirely free library read by hundreds of millions to learn anything about any technology.

WRITEHackerNoon elevates quality technology writing far and wide across the interwebs.

PARTNERHackerNoon works directly with tech companies to place ads by content relevancy