[Part 3/100] Eloquent sucks
Tags: laravel, eloquent, orm, database
Published: 2021-08-29 00:00:31

As the README says:

{% github illuminate/database %}

> The Illuminate Database component is a full database toolkit for PHP, providing an expressive query builder, ActiveRecord style ORM, and schema builder. It currently supports MySQL, Postgres, SQL Server, and SQLite. It also serves as the database layer of the Laravel PHP framework.

Part of this toolkit available is called Eloquent, which is the ORM for the Laravel Framework.

tl;dr, it sucks because...
- Active Record itself sucks
- their actual implementation of Active Record is also bad and exposes a lot
- models represent a table row, but can do _so much beyond that_
- there's like 10 different ways to access model properties
- expectations aren't managed around null vs '0' vs false 
- the illuminate/database code quality is low
- mutators are gross (`get<X>Attribute` is unobviously a proxy method)
- and of course it sucks because artisan code comes at an unknown cost to the unsuspecting

## Bad Active Record implementation

For those unfamiliar with the design pattern, [Active Record is a design pattern](https://en.wikipedia.org/wiki/Active_record_pattern) commonly associated with ORMs for relational databases.

It's often seen as subjective to dislike Active Record, but it objectively mixes concerns; which is something all developers should at very least understand what they're trading off.

Even better, Laravel's implementation of Active Record publicly exposes implementation details which is unnecessarily encouraging poor development choices to the unaware.

```php
<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Model;

class User extends Model
{
    /**
     * Get the phone associated with the user.
     */
    public function phone()
    {
        return $this->hasOne(Phone::class);
    }
}
```
```php
$user = User::find(1);
$phone = $user->phone;
```
^ Standard innocent usage of Eloquent, fetching a related model.

In order for `$user` to resolve the relational 1:1 model `Phone`, it needs to perform a database query. Again, standard for Active Record - but in other words, `$user` has been tightly coupled with your actual database connection.

With that in mind, and a single Eloquent model (`$user`) here's a list of functionality you can perform **directly from your model instance**:

You could... create a fresh QueryBuilder for subsequent unrelated queries

```php
$user = User::find(1);

$freshQueryBuilder = $user
  ->getConnection()
  ->query();
  ->from(
    (new UnrelatedModel())
      ->getTable()
  );

$freshQueryBuilder->from('table_name')->get();
```

or, you could access the schema manager and drop your database... from the model
```php
$user = User::find(1);

$user
  ->getConnection()
  ->getDoctrineSchemaManager()
  ->dropDatabase(
    $post
    ->getConnection()
    ->getDatabaseName()
  )
```

or, you could build a query, and execute in [**Doctrine DBAL**](https://github.com/doctrine/dbal)
```php
$user = User::find(1);

$statement = $user
  ->newQuery()
  ->where('is_activated', 1)
  ->toSql();

$user
  ->getConnection()
  ->getDoctrineConnection()
  ->executeStatement($statement);
```

or, you could serialize something into JSON... from your model
```php
$json = $user
  ->getGrammar()
  ->prepareBindingForJsonContains(new class implements JsonSerializable {
  public $coolData = [
  'something' => 'cool',
  ];

  public function jsonSerialize() {
    return $this->coolData;
  }
}
});

// $json => '{"something": "cool"}'
```

obviously no one will ever do this (please don't). The point is this functionality **should not be exposed**.
Active Record is bad enough without some unstable APIs sprinkled on top.

I'm not going to start on Entity Mapping vs Active Record today, but if you value testability and separating concerns I would recommend Laravel-doctrine.
- http://www.laraveldoctrine.org/

## Cyclomatic complexity
After seeing the shear size of [https://github.com/illuminate/database/blob/427babd19deffaf7a288abd7cdbbcd2aaa86144d/Connection.php](Connection.php) I decided to run a [PHPMD Codesize scan](https://phpmd.org/rules/codesize.html) on the whole illuminate/database repository

Highlights:
- The class Model has 2073 lines of code.
- The class Model has 112 public methods and attributes.
- The class Builder has 1653 lines of code.
- The class Connection has 35 public methods.
- The class Connection has 43 non-getter and setter-methods.
- The class Connection has 70 public methods and attributes.
- The class Connection has 1385 lines of code.
- The method addCastAttributesToArray() has an NPath complexity of 865.
- The method withAggregate() has an NPath complexity of 1304.
- The class Factory has 37 non-getter- and setter-methods.
... and much much more.

don't forget the many magic method usages too.

I would strongly recommend looking into PHPMD, and what NPath & cyclomatic complexity means if it's new to you.

Eloquent and associated database code is essentially complex, hard to work with bloated code. On a high level what this means for framework users is that your underlying database will find it hard to introduce more functionality, and find it hard to improve existing functionality, but will also be more likely to introduce new bugs, because complexity is already high.

## Magical schema-awareness
If you have a table of people, such as:

```sql
 CREATE TABLE Persons (
    PersonID int,
    LastName varchar(255),
    FirstName varchar(255),
    isOverweight TINYINT(1),
    City varchar(255)
); 
```
and a model of 
```php
use Illuminate\Database\Eloquent\Model;

class Person extends Model {
    /**
     * The table associated with the model.
     *
     * @var string
     */
    protected $table = 'Persons';

    public function getIsOverweight($value)
    {
        return !empty($value) ? bool($value) : null;
    }
}
```

Laravel just miraculously happens know how to resolve the values from table columns.
```php
$person = Person::find(1);
$person->City; // 
$person->isOverweight; // Nullable BOOL column
```

It's very neat 'artisan' code isn't it? The cost of this magic is:
- IDE auto-completion
- Loss of static analysis
- magic methods usage [`__get`](https://github.com/illuminate/database/blob/08990b30e16f031d9edccc02fa4c947de64a4622/Eloquent/Model.php#L1942-L1945) = slower code
- Unknown types (is it a string? bool? null? what does a non-existent column return? it's unobvious without deep diving)

Don't forget you can also access model values via...
```php
$person['isOverweight'];
$person->getAttribute('isOverweight');
$person->getAttributes()['isOverweight'];
$person->getAttributeValue('isOverweight');
$person->attributesToArray()['City'];
$person->getOriginal('City');
$person->getRawOriginal('City');
```

Do you feel safe enough to trust Laravel will run the mutator (type cast '1' to true) with **all of these methods**? A sane person would say no, I don't trust like that.  

[Mutators](https://laravel.com/docs/8.x/eloquent-mutators) themselves deeper the wound of model accessors, not knowing what types to expect, and handling null vs 0, etc.

Allowing such dynamic functionality, and duplicate ways to 'get' something is another hidden cost that may be forever unobvious to those choosing to stick within the Laravel ecosystem.

## Redaction

After writing this post I came across a tweet by Laravel creator, which actually justifies some of my pain points

{% tweet 818952001748926465 %}
{% tweet 818959093150912512 %}

https://en.wikipedia.org/wiki/False_equivalence