DEV Community

Mitchell
Mitchell

Posted on

[Part 3/100] Eloquent sucks

As the README says:

GitHub logo illuminate / database

[READ ONLY] Subtree split of the Illuminate Database component (see laravel/framework)

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.

Usage Instructions

First, create a new "Capsule" manager instance. Capsule aims to make configuring the library for usage outside of the Laravel framework as easy as possible.

use Illuminate\Database\Capsule\Manager as Capsule
$capsule = new Capsule;

$capsule->addConnection([
    'driver' => 'mysql',
    'host' => 'localhost',
    'database' => 'database',
    'username' => 'root',
    'password' => 'password',
    'charset' => 'utf8',
    'collation' => 'utf8_unicode_ci',
    'prefix' => '',
]);

// Set the event dispatcher used by Eloquent models... (optional)
use Illuminate\Events\Dispatcher;
use Illuminate\Container\Container
Enter fullscreen mode Exit fullscreen mode

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 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

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);
    }
}
Enter fullscreen mode Exit fullscreen mode
$user = User::find(1);
$phone = $user->phone;
Enter fullscreen mode Exit fullscreen mode

^ 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

$user = User::find(1);

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

$freshQueryBuilder->from('table_name')->get();
Enter fullscreen mode Exit fullscreen mode

or, you could access the schema manager and drop your database... from the model

$user = User::find(1);

$user
  ->getConnection()
  ->getDoctrineSchemaManager()
  ->dropDatabase(
    $post
    ->getConnection()
    ->getDatabaseName()
  )
Enter fullscreen mode Exit fullscreen mode

or, you could build a query, and execute in Doctrine DBAL

$user = User::find(1);

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

$user
  ->getConnection()
  ->getDoctrineConnection()
  ->executeStatement($statement);
Enter fullscreen mode Exit fullscreen mode

or, you could serialize something into JSON... from your model

$json = $user
  ->getGrammar()
  ->prepareBindingForJsonContains(new class implements JsonSerializable {
  public $coolData = [
  'something' => 'cool',
  ];

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

// $json => '{"something": "cool"}'
Enter fullscreen mode Exit fullscreen mode

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.

Cyclomatic complexity

After seeing the shear size of https://github.com/illuminate/database/blob/427babd19deffaf7a288abd7cdbbcd2aaa86144d/Connection.php I decided to run a PHPMD Codesize scan 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:

 CREATE TABLE Persons (
    PersonID int,
    LastName varchar(255),
    FirstName varchar(255),
    isOverweight TINYINT(1),
    City varchar(255)
); 
Enter fullscreen mode Exit fullscreen mode

and a model of

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;
    }
}
Enter fullscreen mode Exit fullscreen mode

Laravel just miraculously happens know how to resolve the values from table columns.

$person = Person::find(1);
$person->City; // 
$person->isOverweight; // Nullable BOOL column
Enter fullscreen mode Exit fullscreen mode

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 = 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...

$person['isOverweight'];
$person->getAttribute('isOverweight');
$person->getAttributes()['isOverweight'];
$person->getAttributeValue('isOverweight');
$person->attributesToArray()['City'];
$person->getOriginal('City');
$person->getRawOriginal('City');
Enter fullscreen mode Exit fullscreen mode

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 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


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

Discussion (3)

Collapse
andreidascalu profile image
Andrei Dascalu

Yep. Also, though, Doctrine doesn’t have that complexity problem anymore (with respect to models at least) since it’s worked a lot on decoupling and models can be straight php classes, no need to extend any base model.

Collapse
byterbit profile image
Byter-Bit

It's horrid because there is NOTHING you need it for; that is: there is nothing you can't do with PHP without it, and thus ALL it does is add layers of obscure anti-pattern to your work.
Code and business logic and dev-ops are complex enough without throwing ANOTHER layer of stuff around. Just say NO.