I broke your Laravel app

But I didn’t mean to

Mockup of a GitHub feature to ban users from repositories, like me.

While I was building an application for a company, I encountered a bug that couldn’t fix because no error was thrown. That headache was a relationship query for a particular Model.

To paraphrase what I was doing, imagine a Repair Shop. Each car to repair is assigned a “Responsable Mechanic”, which is in charge to manage the repairs and communicate with the owner.

To avoid idle mechanics, the app automatically assigns free mechanics to other ongoing repairs to help. In other words, multiple Mechanics can be assigned to multiple Cars.

$car->mechanics()->attach($user, ['is_responsable' => true]);

On the other hand, we had the problem of requesting parts. You see, it’s fine when you have a part in the inventory, it can be charged right away and the repair can be done quickly. It’s a problem when the required part is not on inventory and has to be bought elsewhere.

We came to the conclusion that only the “Responsable Mechanic” for the car could request a part. These requests were, and still are, not cheap. Sometimes rare parts must be imported from overseas, and cannot be returned when requested by mistake, by excess or by a typo.

The Idea

To code that, I created a “request” policy on the Part model.

I create a simple check to only let a mechanic to request a part.

namespace App\Policies;

use Illumindate\Gate\Response;
use App\Models\User;
use App\Models\Repair;

class PartPolicy
{
    /*
     * Determine the user can request a part for a car repair.
     */
    public function request(User $user, Repair $repair)
    {
        if ($user->isNotMechanic()) {
            return Response::deny('Only mechanics can request parts');
        } 

        $isNotResponsable = $repair->mechanics()
            ->whereKey($request->user())
            ->wherePivot('is_responsable', true)
            ->doesntExist();
  
        if ($isNotResponsable) {
            return Response::deny("Only responsable mechanics can request external parts");
        }
    
        return true;
    }
}

Everything it’s fine on that policy. Now let’s add a new variable: if the part is not on inventory, only the assigned mechanic can request for it.

use Illumindate\Gate\Response;
use App\Models\User;
use App\Models\Repair;
use App\Models\Part;

class PartPolicy
{
    /*
     * Determine the user can request a part for a car repair.
     */
    public function request(User $user, Repair $repair, Part $part)
    {
        if ($user->isNotMechanic()) {
            return Response::deny('Only mechanics can request parts');
        } 

        $isNotResponsable = $repair->mechanics()
            ->whereKey($request->user())
            ->when($part->isDepleted())->wherePivot('is_responsable', true)
            ->doesntExist();
  
        if ($isNotResponsable) {
            return Response::deny("Only responsable mechanics can request external parts");
        }
    
        return true;
    }
}

Everything looks fine from here. There is an additional check to find if the Part requested has enough inventory: we use when() to only call wherePivot() if the prior condition is truthy, with is part of the many magic tools of Laravel.

It may look good from here, but the check appeared to randomly deny the permission. That was a bug, and I didn’t know where to look at.

The culprit

That kind of bug turn me around an entire week. First, I took notice that the permission failed when the piece was not on inventory, regardless of the mechanic status. Since it was one of the many parts of the application where the policy was enforced, it was hard to catch where ot with who the problem lied.

Low and behold, I finally decided to check the database query generated by Laravel as a last resort in Friday afternoon. Something was not right.

SELECT * FROM "mechanics"
    INNER JOIN "repair_mechanic" 
        ON "mechanic"."id" = "repair_mechanic"."mechanic_id"
    WHERE "repair_mechanic"."repair_id" = 57 AND
    WHERE "repair_mechanic"."pivot" = 'is_responsable'

The last line was totally wrong. SQLite was happily receiving the invalid column “pivot”, which doesn’t exist in the table, and returning zero results for that query. Remember that SQLite is very lenient and it didn’t throw any error when hitting a table that didn’t exist.

Since that was the culprit, then I had to find what part of the code generated that last line, and it was wherePivot().

Laravel’s wasn’t picking up the wherePivot() method as it should. I expected to call the BelongsToMany class method, but instead it was passing that to the Query Builder. There, Laravel magic would transform the method into a simplier WHERE clause.

$query->when($part->isDepleted())->wherePivot('is_responsable', true);

// Same as...
if ($pary->isDepleted()) {
    $query->where('pivot', 'is_responsable');
}

The code was what created the WHERE table."pivot" clause .

I did remove the when() method, and it worked! Then, the problem was not the query builder, the tables, or the logic itself. It was the method implemented by the Conditionable trait.

To fix that, I needed to break the query and add the condition manually.

$isNotResponsable = $repair->mechanics()->whereKey($request->user());

if ($part->isDepleted()) {
    $isNotResponsable->wherePivot('is_responsable', true);
}

$isNotResponsable = $isNotResponsable->doesntExist();

The explanation

Well, each Relationship instance, from BelongsTo to MorphToMany, extends a base class called Relation.

This class will proxy method calls to the underlying Query Builder instance and return itself. That’s why you can immediately call query methods when invoking a relationship method from a model:

$part->requesters()->where('name', 'John Doe')->first();

// Same as...
$part->requesters()->newQuery()->where('name', 'John Doe')->first();

This is also true for tap(), when() and unless(). The call is proxied to these methods, and inside these the Query Builder instance is passed down, not the Relation class whatever it may be.

In the end, executing wherePivot() after these methods, or inside the function they received, would be executed on the Query Builder.

$user->cars()
    ->when(true, fn($query) => ...)
    ->get();

// Same as...
$cars = $user->cars();

if (true) {
    $cars = $cars->getQuery()->doSomething();
}

$cars = $cars->get();

The solution

I had to allow these methods to proxy the call Relationship instance first, instead of the underlying Query Builder. What’s better than just shoving the Conditionable and Tappable (that works the same) into the Relation class.

I remembered that I have that idea from weeks ago, so I pushed a PR as I thought it would benefit everyone. All framework tests passed with flying colors except the styling (which is fixed automatically on merge), but the issue was no more…

…until Tuesday, of course.

The Aftermath

You see, there is a point on thoroughly testing things: you can avoid edge cases. Even a single innocent line can save a lot of headaches and time lost. Sometimes it’s not possible due to scope, then it becomes something you build upon as requested. That where Laravel and I failed.

That PR that was approved by none other than Taylor Otwell, so I had confidence it was fine. At the end of the day, the PR passed internal testing , so what could be wrong?

Well, a lot.

What happened, then?

1. High Order proxy method calls, like $query->orWhere->something() , where not working anymore.
2. Type Hinting the Builder instance in the function passed down to the traits methods, like fn(Builder $query) => ..., also broke.

Not only some applications, also some other appliances that rely on relationships like Laravel Nova were also affected.

The above problems have their immediate fix:

1. Add the __get() magic method to the Relation class to allow passing High Order method calls to the query.
2. Telling all developers to use the Builder contract (interfaces) rather than the classes directly.

While I was going to do the first small patch, I took notice that Taylor rolled back the PR, so I had no time to make the hotfix.

The point 2 was a contention. Requiring changing your code on a non-major version was something that would break many Laravel installations, and it was already happening. For me, it was weird because since Laravel 9 there is the Builder interface, which helps to avoid the “What class is it?” problem. Another case of RTFM?

The Workaround

The PR was rolled back, so the problem still persisted.

The only workaround for this problem is to not use these methods if you need to call a Relation method, like wherePivot(). Instead, just break the query and add a raw condition, or be wary to expect the Query Builder instance instead.

$query = $user->cars();

if ($something) {
    $query->wherePivot('foo', 'bar');
}

return $query->get();

It’s kind of a bummer, because it makes your code less compact, but it is what it is.

Who knows if I resubmit this PR for Laravel 12 and tell everyone to slightly change their code with the promise of enabling more Laravel magic.