13 - "The Payoff": Eliminate Trait and One More Controller

This will probably be the "payoff" lesson: we will be able to delete a lot of code that we refactored not to be used in the earlier lessons.

For that, let's look at other Controllers and what we can simplify/eliminate there.

Wait, What's that OwnerOrdersController?

I found a "suspicious" OwnerOrdersController that seems to almost duplicate the purpose of OrderController, just filtering by the order's user_id.

routes/api_v1.php:

Route::apiResource('orders', OrderController::class);
 
// ...
 
Route::apiResource('owners.orders', OwnerOrdersController::class);

My question was: do we really need that separate Controller?

That Route::apiResource('xxx.yyy') syntax is called Nested Resources and works well to build the endpoints like these:

GET owners/1/orders - get the list of orders by user_id 1
GET owners/2/orders/3 - get the order with ID 3 by user_id 2

But... user_id should come from the Authentication with auth()->id(). What's the purpose of creating a separate Controller?

This is my (a bit longer) sequence of thoughts. Hear me out.

I thought it might look logical if some administrator user wanted to get the orders by a specific user manually, without logging in as that user.

But looking at the contents of this Controller... it felt wrong to me.

app/Http/Controllers/Api/V1/OwnerOrdersController.php

class OwnerOrdersController extends ApiController
{
    use AuthorizesRequests;
 
    protected $policyClass = OwnerPolicy::class;
 
    public function __construct(
        protected OrdersService $orderService
    ) {}
 
    /**
     * Display a listing of the resource.
     */
    public function index($ownerId, OrderFilter $filter)
    {
        try {
            $this->authIsOwner($ownerId);
 
            return response()->json(new OrderCollection(
                Order::where('user_id', $ownerId)->filter($filter)->paginate()
            ), Response::HTTP_OK);
        } catch (ModelNotFoundException $eModelNotFound) {
            return $this->responseNotFound('User not found');
        } catch (AuthorizationException $eAuthorizationException) {
            return $this->responseNotAuthorized();
        }
    }
 
    // ...

So much duplicated code with the original OrderController, just to additionally call $this->authIsOwner($ownerId) and where('user_id', $ownerId)?

At first, I started refactoring, but then I looked at what was inside that authIsOwner().

app/Http/Controllers/Api/V1/ApiController.php

class ApiController extends Controller
{
    use AuthorizesRequests;
 
    public function authIsOwner($ownerId)
    {
        $owner = User::findOrFail($ownerId);
        $this->authorize('authIsOwner', $owner); // policy
    }
}

Notice: here, $this->authorize() can be called instead of Gate::authorize() because of the use AuthorizesRequests trait added on top. It was added by default in Laravel 10, but since Laravel 11, it has to be added manually if you want to use this "older" syntax.

Next, what's inside that authIsOwner in the Policy?

app/Policies/V1/OwnerPolicy.php:

/**
 * Determine whether the user is the Owner of the resources.
 */
public function authIsOwner(User $authUser, User $owner): bool
{
    return $authUser->id === $owner->id;
}

So, wait a minute. We are querying the user just to check whether the logged-in user is the same as the owner?

This means we could just use auth()->id() for the user in the query.

So yeah, then I concluded that this Controller probably doesn't even work.

And, again, there were no automated tests to test it.

So, I completely deleted this Controller and removed it from the routes.

routes/api_v1.php

Route::apiResource('owners.orders', OwnerOrdersController::class);

Side benefit: now we can eliminate the useless methods in the ApiController checking the "ownership" because they were used ONLY in that OwnerOrdersController. Let's get to that cleanup!

Cleanup of ApiController

Look at what methods we have in here:

app/Http/Controllers/Api/V1/ApiController.php

class ApiController extends Controller
{
    public function isAble($ability, $model)
    {
        return Gate::authorize($ability, [$model, $this->policyClass]);
    }
 
    public function authIsOwner($ownerId)
    {
        $owner = User::findOrFail($ownerId);
        $this->authorize('authIsOwner', $owner); // policy
    }
 
    public function isAbleIsOwner($ability, $model, $ownerId)
    {
        $this->authIsOwner($ownerId);
 
        return $this->isAble($ability, $model);
    }
}

When I searched for where they are used...

isAble() is now used only in the same Controller, in isAbleIsOwner()
authIsOwner() is used only in the same Controller and was used in the OwnerOrdersController that I just deleted
isAbleIsOwner() was used only in the OwnerOrdersController that I just deleted

So... all those three methods can be safely deleted now!

app/Http/Controllers/Api/V1/ApiController.php

namespace App\Http\Controllers\Api\V1;
 
use App\Http\Controllers\Controller;
use App\Models\User; 
use App\Traits\V1\ApiResponses;
use Illuminate\Foundation\Auth\Access\AuthorizesRequests; 
use Illuminate\Support\Facades\Gate; 
 
class ApiController extends Controller
{
    use ApiResponses;
    use AuthorizesRequests; 
 
    protected $policyClass; 
 
    public function include(string $relationships): bool
    {
        $param = request()->query('include');
 
        if (is_null($param)) {
            return false;
        }
 
        $includes = explode(',', strtolower($param));
 
        return in_array(strtolower($relationships), $includes);
    }
 
    public function isAble($ability, $model) 
    {
        return Gate::authorize($ability, [$model, $this->policyClass]);
    }
 
    public function authIsOwner($ownerId)
    {
        $owner = User::findOrFail($ownerId);
        $this->authorize('authIsOwner', $owner); // policy
    }
 
    public function isAbleIsOwner($ability, $model, $ownerId)
    {
        $this->authIsOwner($ownerId);
 
        return $this->isAble($ability, $model);
    } 
}

So now we have an ApiController with just one method include() and then that ApiResponses Trait?

But wait, do we even need that Trait after we have moved all the try-catches into the bootstrap/app.php file?

Removing ApiResponses Trait

Here's where we get to the primary goal of this whole course.

This is the list of methods inside that Trait and where they are used:

responseNotAuthorized(): not used anymore (deleting it)
responseNotFound(): not used anymore (deleting it)
responseSuccess(): used once in the OrderController and 2x in the AuthController that we didn't touch yet
responseError(): used only internally in the same Trait and the AuthController that we haven't touched yet

So, here's my plan:

Move responseSuccess() back to the ApiController (that method is still needed, used in multiple Controllers)
Change responseError() usage in AuthController (that method is not needed, used only once)
Delete the ApiResponses Trait

This is the new version of ApiController:

app/Http/Controllers/V1/ApiController.php:

namespace App\Http\Controllers\Api\V1;
 
use App\Http\Controllers\Controller;
use Illuminate\Http\JsonResponse;
 
class ApiController extends Controller
{
    public function include(string $relationships): bool
    {
        $param = request()->query('include');
 
        if (is_null($param)) {
            return false;
        }
 
        $includes = explode(',', strtolower($param));
 
        return in_array(strtolower($relationships), $includes);
    }
 
    protected function responseSuccess(string $message, array $data = [], int $statusCode = 200): JsonResponse
    {
        return response()->json([
            'data' => $data,
            'message' => $message,
            'status' => $statusCode,
        ], $statusCode);
    }
}

Then, in the AuthController, we make these replacements:

use App\Http\Controllers\Controller; 
use App\Http\Controllers\Api\V1\ApiController; 
use App\Traits\V1\ApiResponses; 
use Illuminate\Validation\ValidationException; 
 
// ...
 
class AuthController extends Controller 
class AuthController extends ApiController 
{
    use ApiResponses; 
 
    public function login(LoginUserRequest $request)
    {
        $request->validated($request->all());
        $credentials = $request->only('email', 'password');
 
        if (! Auth::attempt($credentials)) {
            return $this->responseError([ 
                'email' => ['The provided credentials are incorrect.'],
                'password' => ['The provided credentials are incorrect.'],
            ], 401); 
            throw ValidationException::withMessages([ 
                'email' => ['The provided credentials are incorrect.'],
                'password' => ['The provided credentials are incorrect.'],
            ]); 
        }
 
        // ...
    }
 
    // ...
}

Here, I removed the responseError() in favor of just the Validation Exception. And yes, I've changed the HTTP status code from 401 to 422 because I personally think it should be a validation error.

In the following lessons, we'll get back to cleaning up the entire AuthController.

And now, we can (finally!!!) delete the trait ApiResponses. It's not used anywhere anymore. #achievementUnlocked

Here's the GitHub commit for all this significant change.

Laravel API Code Review and Refactor

"The Payoff": Eliminate Trait and One More Controller

Wait, What's that OwnerOrdersController?

Cleanup of ApiController

Removing ApiResponses Trait

Intro and Styling with Laravel Pint

Feature/Unit Tests: Make them Work

Tests actingAs(): No Need to Send Tokens

No Need for response()->json()

Single Responsibility: Move Controller Code to Trait

Duplicate Code: Make Controller Resourceful

Route Model Binding

Static String Letters to PHP Enum

Handling AuthorizationException Globally

Using Gates Properly: Remove isAble() Method

Quick Fix for Test Factories

OrderController with no try-catch: Final Version

"The Payoff": Eliminate Trait and One More Controller

Order Owner Refactor to User Controller

AuthController and Review Conclusion