Static analysis versus Laravel conventions

Ready to play "spot the bug"? Take a look at this simple controller method and see if you can find the issue:

$surveyData = $request->validated();

$survey = $request->user()->surveys()
    ->make($surveyData)
    ->forceFill(['some_system_field' => 'value'])
    ->save();

return redirect()->route('surveys.show', ['survey' => $survey]);

Maybe you see it immediately, or maybe you're staring at this with no clue.

Even worse, when I wrote this bug, my test passed. In fact, it only failed when I ran the full test suite. (And I am using RefreshDatabase, so it's not something from a previous test causing it to fail, at least not in the way you might think.)

The issue is that save() returns true or false, based on whether the model was successfully saved or not. It doesn't return the model instance.

But even more tricky, because of the flexibility of Laravel route binding, when I pass this boolean into the route, it gets type-juggled to the number 1, which is a valid ID for a model.

Even worse, when running the test in isolation on a fresh database, the model that gets created has an ID of 1, so the test passes. It's only when you run all the tests together, and the auto-increment value gets marched forward (even when running in a transaction), that the bug becomes apparent.

If I had used the more verbose $survey->id to bind the route, PHPStan would have yelled at me for trying to call a method on a boolean value.

There's an inherent tension between the flexibility of Laravel and the strictness of static analysis tools. In this case, I kept the cleaner model binding syntax, but I thought it was an interesting example to share and think about.

If you want to hear Aaron and I discuss our thoughts about whether we should enforce a coding standard on this, check out this podcast episode.

Here to help,

Joel

P.S. Want to share your own "war stories" of sneaky bugs like this? Join the Mastering Laravel community.