17+ Laravel "Bad Practices" You Should Avoid

17+ Laravel "Bad Practices" You Should Avoid
Admin
Tuesday, April 30, 2024 6 mins to read
Share
17+ Laravel "Bad Practices" You Should Avoid

People keep asking me about "best practices" in Laravel. The thing is that Laravel allows us to perform things in many different ways, so there are almost no obvious-absolute-best practices. However, there are clear BAD practices. So, in this article, I will list the most common ones.


What is Considered a "Bad Practice"?

This is kind of my personal interpretation, but I define bad practice as a code that can cause real negative impact in the future:

  • Security issues
  • Performance issues
  • Increased bugs possibility

Some impact may be noticed only years later by new developers joining the team. But then, the price to fix those issues is often very high, usually about big refactorings.

First, I will list the really bad practices (in my opinion) that lead to severe impacts like performance or security issues.

In the second section of the article, I will list "not so bad" practices, some of which are debatable, so be active in the comments.

Now, let's dive into the list!


Bad Practice 1. Not Preventing N+1 Query with Eager Loading.

Let's start with the "elephant in the room".

Whenever I hear a problem with Laravel performance, I first check how many SQL queries are executed under the hood of Eloquent sentences.

Here is the classic example from the docs:

use App\Models\Book;
 
$books = Book::all();
 
foreach ($books as $book) {
echo $book->author->name;
}

That's right, for every book, it will run another SQL query to get its author.

You can fight this problem of too many (N+1) SQL queries on THREE levels.

  1. FIND & fix them: with Laravel Debugbar
  2. AVOID them: learn to use Eager Loading
  3. PREVENT them: add preventLazyLoading()

Bad Practice 2. Loading Too Much Data from DB.

This is somewhat related, also causing performance issues, but from another angle.

$posts = Post::with('comments')->get();
 
foreach ($posts as $post) {
echo $post->title . ': ' . $post->comments()->count();
}

See what is wrong here?

Yes, we're loading the full comments relationship with all the columns, although we need only the COUNT.

Instead, it should be this:

$posts = Post::withCount('comments')->get();
 
foreach ($posts as $post) {
echo $post->title . ': ' . $post->comments_count;
}

If you run too many SQL queries, they load your DB server and network which will probably just go slower and slower.

But if you load too much data from Eloquent into PHP variables, it's stored in RAM. And if that reaches the memory limit, your server will just crash and will load this for users in the browser:

Also, in the same example above, we load all the data for the Post Model, although we need only the title. If posts contain long text content with 1000+ or more words, all those kilobytes will be downloaded into memory. Multiply that by amount of users viewing the page, and you may have a big problem.

So, it should be:

$posts = Post::select('title')
->withCount('comments')
->get();
 
foreach ($posts as $post) {
echo $post->title . ': ' . $post->comments_count;
}

You may not feel the impact on smaller tables, but it may be just a general good habit to adopt.

So the general rule of thumb is "only load the data you actually need".

I also discuss these performance problems from above deeply in the premium tutorial Optimizing Laravel Eloquent and DB Speed: All You Need to Know and in the course Better Eloquent Performance.


Bad Practice 3. Chain Eloquent without checking.

Have you ever seen this code in Blade?

{{ $project->user->name }}

So, the Project belongs to a User, and it seems legit.

But guess what happens if the User model doesn't exist? For whatever reason: soft-deleted, typo in the name, etc.

Then you will get this error:

There are 4 ways to fix this relationship chain: here's a tutorial about it.

My favorite is just using PHP null-safe operators:

{{ $project->user?->name }}

But of course, it depends on the situation. However, my point is that it's a generally bad practice to chain without checking intermediate objects.

Here's another example:

$adminEmail = User::where('is_admin', 1)->first()->email;

What if there's no record of first()?

So, I see many developers trust/assume that the record will always exist because it exists for them at the moment.

Not a future-proof code.

So, always check if the expected record exists, and gracefully show errors if it doesn't.


Bad Practice 4. API returning 2xx Code with Errors.

Tired of talking about Eloquent? Let's switch to APIs. Have you seen something like this "horror" in Controller?

public function store()
{
if ($someCondition) {
return response()->json([
'error' => true,
'message' => 'Something happened'
], 200);
}
 
// ...
}

There are actually many things that need to be corrected in this snippet, but I want to emphasize that 200 number.

It's actually a default value, so this code would do the same:

return response()->json([
'error' => true,
'message' => 'Something happened'
]);

My point is that if the API has some error, you need to return the error code to the API client.

Imagine the face of a front-end or mobile developer who tries to call this API and gets no error code, but their app doesn't work. And they don't know why because API seems to return a good result!

It's so frustrating that it deserves a meme:

Unfortunately, I didn't find the original author, just this Reddit post.

In general, when creating APIs, communicate with all parties involved in consuming that API and agree on the standard requests/responses to avoid misunderstandings. Or, if you create a public API, please document all possible return results and their status codes.

There are also slight differences between returning, for example, 401 vs 403 code, and similar examples. But those are not crucial. The most important is the first number: success or not.

In general, there are dozens of HTTP status codes, but usually only about 10 of them are widely used. I like this list explained in human language.


Bad Practice 5. Blindly Trusting User Data Without Validation.

The classic example is this code in Blade.

{!! $user->about_me !!}

So, you allow your users to fill in their "About me" with HTML inside, you show that in the browser, and may have this as a result:

That happens if the user fills in something like script alert... (intentionally don't want to show the malicious example) in the "About me" page.

So, the general rule of thumb is to use this unescaped {!! !!} syntax only on the data YOU control or properly validated.

Or, of course, just use the {{ ... }} syntax to prevent such XSS attacks.

Another example of blindly trusting the data is using $request->all(). Look at this:

public function store(StoreUserRequest $request) {
User::create($request->all());
 
return redirect()->route('dashboard');
}

Looks harmless, doesn't it? Especially if you have a proper StoreFormRequest class for the validation.

But what if I told you there's a security issue here?

The thing is that $request->all() contains literally ALL of the input data, including the data that wasn't validated.

So, for example, if someone passes the is_admin=1 as a part of that request, and it's not validated in your StoreFormRequest class, it may be saved to the DB successfully without you even noticing. In other words, if your users guess your DB field name, they may register as an admin.

Instead, you should use $request->validated() after the FormRequest class or specify the exact fields individually.

But the general "bad practice" message here is the same classics: Never trust user input. EVER.


Section 2: "Not So" Bad Practices.

Ok, so we covered the top 5 "worst" practices, in my opinion, which may cause performance/security issues. In the second part of this article, I will list the less harmful practices, some of which are even debatable.

So, in no particular order...


6. Not Using Route Names

Don't do this:

<a href="/posts/{{ $post->id }}">{{ $post->title }}</a>

Do this instead:

<a href="{{ route('posts.show', $post) }}">{{ $post->title }}</a>

There are two reasons for this:

  • If you ever need to change the URLs (which happens more often than you think), you would need to change them only in the Routes file, and not everywhere in the project
  • Often route names follow the standard pattern of CRUD-like Resource Controllers, which makes it easier to read/understand by other future developers

Read more about route names in the official Laravel docs.


7. Using env() Outside Config Files

This code in Blade is considered a bad practice:

<title>{{ env('APP_NAME', 'My project') }}</title>

Instead, you should do this:

<title>{{ config('app.name') }}</title>

Then, define the config file value using the same env() and the "My project" fallback value on the config level.

config/app.php:

return [
'name' => env('APP_NAME', 'My project'),
];

I also have a video explanation on YouTube.


8. Filtering Data in Collections Instead of DB

Let's get back to the topic of Eloquent. Have you ever seen something like this?

$activeUsers = User::all()->where('active', 1);

Of course, it looks bad: you download ALL the data from the server, even if it's millions of rows, and then filter what you need on the PHP level.

Instead, of course, you should filter on the DB level because that's what the DBs were built for.

$activeUsers = User::where('active', 1)->get();

And this is an obvious example. But there are more real-life cases where developers try to use Collection methods like filter(), map(), and others instead of using conditions before calling the ->get().

I also have this related video on YouTube: Eloquent Performance: TOP 3 Mistakes Developers Make


9. Confusing/Mixing Eloquent with Query Builder

Do you see a difference in results between these two sentences?

// Eloquent Model:
$users = User::all();
 
// VS Query Builder:
$users = DB::table('users')->get();

They will likely return identical results, right?

But. What if I told you that the Model uses Soft Deletes and Global Scopes?

Now the results of those two queries may be different, right?

So, the typical mistake I've seen developers make is using Query Builder and forgetting about some Eloquent global settings.

Another case is where people use only the Query Builder without even touching Eloquent. But then, in my opinion, it defeats the purpose of using Laravel as a framework in the first place.

Anyway, the actual "bad practice" is not understanding the differences between Model and Query Builder, leaving unexpected bugs in data filtering.

I have this video on YouTube: Eloquent or Query Builder: When to Use Which?


10. Hard-Coded Paths/Values Instead of Helpers/Config

If you ever use the direct /storage/app/... path when saving files, it's a short-sighted code.

  • What if your teammate wants to save it in a different location?
  • What if future Laravel versions change that location?
  • What if your staging/production server needs to use the Amazon S3 driver?
  • etc.

So, two things here:

  • Instead of hardcoded paths, Laravel has helpers like storage_path(), app_path(), etc.
  • But generally, it should all be configurable in the config/filesystems.php, and you need to use only the driver which you use

And it's not only about file storage. You shouldn't use hard-coded values such as timezone. Instead, it should be taken from config('app.timezone').

Then the application will be much more flexible for use on other environments and by other developers.


11. Not Adding DB Foreign Keys

In Migration, the relationship can be created as just a column without the foreign key:

// No ->foreign():
$table->unsignedBigInteger('user_id');

And the Laravel code would still work.

But then, if you try to delete a record, the database will not protect you from accidentally deleting a record that should NOT be deleted because of related records.

That's what constraints are for, like RESTRICT/CASCADE on delete/update.

So, a better way is this:

$table->foreignId('user_id')->constrained();

Notice: this one is a bit debatable because some database engines work well without foreign keys and solve the same problem from a different angle.

You can also watch my explanation in a YouTube video: Laravel Foreign Keys: How to Deal with Errors


12. Editing Old Migration Files

If you want to make a "quick fix" in the database structure, like changing a column type or making a field nullable, please don't do it by making changes in the old migration files.

By "old", I mean - if the Migration has been executed at least once on your computer or another server, it's already processed.

If you make a change in an already "processed" migration file, the next time you run php artisan migrate, Laravel will not "catch" that change because, well, it's already marked as processed in the migrations DB table.

So you're risking that when you push your changes to the repository, the DB migrations will not be executed correctly on the staging/live server or for other teammates on their servers.

So the correct way is to create a new migration file.

Yup, even if you need to change something that was created recently, the safest way is a new migration.

For example, if you created a ->string() field and want to change that to ->text(), create a new migration, assuming that the field already exists.

Another alternative, if the changes haven't been pushed to the repository or other servers/teammates, is to use php artisan migrate:rollback, then make the change in the migration file, and run php artisan migrate again. But for that to work, you must be sure that your down() method is accurate and that you haven't run those migrations elsewhere.

Oh, and by the way, should I even mention that you should never make DB changes manually via SQL client? Always use Laravel migrations for any DB structure changes. Otherwise, any manual changes will be lost on other servers.

You can also watch this YouTube video: Laravel Migrations: 12 Useful Tips in 12 Minutes


13. Misconfigured .env in Prod: Debug Mode or Global Settings

One of the most significant security risks for Laravel project is to leave this setting:

.env:

APP_DEBUG=true

This means all the error details will be seen in the browser, instead of the default error pages. So, the user will see all the information about Laravel/PHP internals and can use them for malicious purposes.

There are more things that you can misconfigure on the live server, like incorrect /public folder setting, which leaves the .env file available in PUBLIC (yes, even found on Google).

The topic itself is pretty broad, but the general "bad practice" is to leave the local settings unchanged for production servers.

A related video on the topic: Laravel .env.example: APP_XXX Values Explained


14. Not Following Laravel Naming Convensions

In general, Laravel doesn't restrict you very much from naming variables/classes and other things. Still, the rule of thumb is this: the more you stick to general convensions, the more Laravel features will "just work" without additional configuration.

Examples of non-convensional names:

  • Naming classes, DB tables, Models, or smth else in non-English language
  • Naming many-to-many pivot tables like "users_roles" instead of "role_user"
  • Naming the primary key different than "id" without the need
  • Naming Eloquent Models in Plural: "class Users" instead of "class User"
  • etc.

Yes, there are specific reasons for naming things your way, if you know what you're doing.

And yes, you can specify what you named in various Laravel/Eloquent configurations and properties, but why not avoid the extra work?

My video about it: Laravel: How to Name Various Things


15. Not Preparing for Future Installations

The project may be joined by new developers who should be able to install it easily. Or, maybe you will buy a new computer and need to reinstall the project from GitHub?

Also, the project will probably be installed on multiple servers and environments: local, staging, and production. That should also smooth.

So, what project creators often forget is to "prepare the ground" for future installations, making it work only locally for themselves.

That include three things:

  • Proper installation instructions that would actually work
  • .env.example file that would include all the keys that should be needed for installation (without values, the values may be different for each server)
  • Database seeders for the initial data needed to start using the application in testing/real mode

Related video: How Do You Seed Data to Production?


16. Not Refactoring Repeatable Code, like Scopes

This one is not that much about Laravel. It's more like a common bad coding practice.

If you use the same ->where() condition in multiple Eloquent queries in at least a few places in the code, it's a candidate for refactoring into a specific function to avoid repeating yourselves.

In Laravel, such repeating conditions in Eloquent are called Local Scopes.

class User extends Model
{
public function scopeActive(Builder $query): void
{
$query->where('active', 1);
}
 
// ...
}
 
// Then in Controller:
$users = User::active()->get();

But the message here is not just about Scopes. It's about any repeating code that likely needs to be moved into one place.

Otherwise, if we want to make a change in that code, we may perform it on one occasion but forget to apply the same changes in other files.

I have a special full course just on this topic: 10+ Laravel Refactoring Examples.


17. Breaking MVC and Putting Logic in Blade

This one has been a sacred rule for me personally for many years:

  • Model is for Database
  • Controller is for parsing route parameters, performing the action (with extra classes if needed - Services/Jobs/etc), and passing the results to the View
  • Blade View is ONLY for showing the data, with minimal logic like @if or @foreach

And I've seen developers breaking these rules by putting @php inside Blade or doing calculations in the View layer. I was angry at them.

BUT...

It became debatable when they released Livewire Volt. Officially supported by the Laravel team, having ALL the logic in the Blade file became a "normal" practice for convenience.

Blade file:

<?php
 
use function Livewire\Volt\{state};
 
state(['count' => 0]);
 
$increment = fn () => $this->count++;
 
?>
 
<div>
<h1>{{ $count }}</h1>
<button wire:click="increment">+</button>
</div>

So, this "bad practice" became much more open for debates and personal preference. But, I still stand for my opinion that in most cases, Views should be only for the presentation layer, except maybe for small projects.

One of my most popular courses on this topic is called How to Structure Laravel 11 Projects.


So yeah, these are (not so) bad practices I've seen developers doing in their Laravel projects.

In addition, I will quickly add two more kinda-obvious things without deeper comments:

  • Not writing tests (do I really need to explain?)
  • Not upgrading to new versions (no need to do it right away, but eventually it's recommended)

Would you add anything else to this list? Or disagree with anything? Let's discuss this in the comments below!