Laravel Controller-Model Exception Handling structure with database transactions

Question

With regards to architecture, which of the two is a good practice when throwing exceptions from model to controller?

Structure A:

UserController.php

public function updateUserInfo(UserInfoRequest $request, UserModel $userModel)
{
    $isError = false;
    $message = 'Success';

    try {
        $message = $userModel->updateUserInfo($request->only(['username', 'password']));
    } catch (SomeCustomException $e) {
        $isError = true;
        $message = $e->getMessage();
    }

    return json_encode([
        'isError' => $isError,
        'message' => $message
    ]);
}

UserModel.php

public function updateUserInfo($request)
{
    $isError = false;
    $message = 'Success';

    $username = $request['username'];
    $password = $request['password'];

    try {
        $this->connect()->beginTransaction();

        $this->connect()->table('users')->where('username', $username)->update(['password' => $password]);

        $this->connect()->commit();
    } catch (\Exception $e) {
        $this->connect()->rollback();
        $isError = true;
        $message = $e->getMessage();        
    }

    return [
        'isError' => $isError,
        'message' => $message
    ];
}

Structure B:

UserController.php

public function updateUserInfo(UserInfoRequest $request, UserModel $userModel)
{
    $isError = false;
    $message = 'Success';

    try {
        $userModel->updateUserInfo($request->only(['username', 'password']));
    } catch (SomeCustomException $e) {
        $isError = true;
        $message = $e->getMessage();
    } catch (QueryException $e) {
        $isError = true;
        $message = $e->getMessage();    
    }

    return json_encode([
        'isError' => $isError,
        'message' => $message
    ]);
}

UserModel.php

public function updateUserInfo($request)
{
    $username = $request['username'];
    $password = $request['password'];

    try {
        $this->connect()->beginTransaction();

        $this->connect()->table('users')->where('username', $username)->update(['password' => $password]);

        $this->connect()->commit();
    } catch (\Exception $e) {
        $this->connect()->rollback();
        throw new QueryException();
    }
}

In Structure A, the model catches any exception, rollback the transaction and return if it has an error or none to the controller. The controller then just return whatever is returned from the model.

While in Structure B the model catches any exception, rollback the transaction then throw a QueryException if an exception occurred. The controller then catches the thrown QueryException from the model then the return if it has an error or none.

The reason why Structure B still have a catch is that the model should be the one to do the rollback. If I were to remove the try-catch on the model here and the controller to directly catch an exception, then the rollback will be handled on the controller which I think kind of clutters the functionality of the controller.

Let me know your thoughts. Thanks!

Answer 1

Why I think the approach from B is better:

1. Your Model should only include the logical part: This includes the communication with the database (transaction and rollback), not the formatting for the error message you want to print to the user.

2. Keep your model clean: It's the most important part of the MVC-structure. If you mess it up it will be very difficult to find any errors.

3. Outsourcing the error-handling: if you put it in the controller you have the choice to handle it there (maybe you want some special formatted output for this method or you need some other functions to call) or you handle it in the App\Exceptions\Handler. In this case you can render this error message here and don't have to do it in the controller.

So if you don't need any special function calls and want to use the full power of Laravel I would suggest you Structure C

UserController.php

public function updateUserInfo(UserInfoRequest $request, UserModel $userModel)
{
    $userModel->updateUserInfo($request->only(['username', 'password']));
    return response()->json(['message' => 'updated user.']); 
}

UserModel.php

public function updateUserInfo($request)
{
    $username = $request['username'];
    $password = $request['password'];
    try {
        $this->connect()->beginTransaction();

        $this->connect()->table('users')->where('username', $username)->update(['password' => $password]);

        $this->connect()->commit();
    } catch (\Exception $e) {
        $this->connect()->rollback();
        throw new QueryException();
    }
}

App\Exceptions\Handler

public function render($request, Exception $exception)
{
    //catch everything what you want
    if ($exception instanceof CustomException) {
        return response()->json([
          'message' => $exception->getMessage()
        ], 422);
    }

    return parent::render($request, $exception);
}

You have a clean separation of the Database stuff (Model), the presentation stuff (Controller) and the error handling (Handler). Structure C allows you to reuse the error-handling in other functions where you have the same situation in another controller function.

This is my opinion but I am open to discuss about any scenario where you think this approach isn't the best solution.

Answer 2

First of all, for your example, you don't even need to use Transaction. You are performing just one query. so why do you need to rollback? Which query you want to rollback? A transaction should be used when you need a set of changes to be processed completely to consider the operation complete and valid. If the first one is successful, but any of the following has any error, you can rollback everything as if nothing was ever done.

Secondly, Lets come to the point good practice or best practice. Laravel suggest Thin controller and Thick model. So all of your business logic should be in model or even better in a repository. Controller will be act as a broker. It will collect data from repository or model and pass it to view.

Alternately, laravel provides some nice and convenient way to organize your codes. You can use Event and Observers for concurrent operation in your model.

Best practice is varies based on the knowledge and experience of the users. So who knows, the best answer for your question is yet to come.

Answer 3

I'd rather keep the controllers and any other part of the system that interacts with the models, as agnostic as possible about the inner workings of the model. So for instance I'd try to avoid being aware of QueryExceptions outside of the model and instead treat it as a plain PHP object whenever possible.

Also I'd avoid the custom JSON response structure and use HTTP statuses. If it makes sense, maybe the route for updating the user info returns the updated resource, or maybe a 200 OK is enough.

// UserModel.php
public function updateUserInfo($request)
{
    $username = $request['username'];
    $password = $request['password'];

    try {
        $this->connect()->beginTransaction();

        $this->connect()->table('users')->where('username', $username)->update(['password' => $password]);

        $this->connect()->commit();

        return $this->connect()->table('users')->where('username', $username)->first();
        // or just return true;
    } catch (\Exception $e) {
        $this->connect()->rollback();

        return false;
    }
}

// UserController.php    
public function updateUserInfo(UserInfoRequest $request, UserModel $userModel)
{
    $updated = $userModel->updateUserInfo($request->only(['username', 'password']));

    if ($updated) {
        return response($updated);
        // HTTP 200 response. Returns JSON of updated user.
        // Alternatively,
        // return response('');
        // (200 OK response, no content)
    } else {
        return response('optional message', 422);
        // 422 or any other status code that makes more sense in the situation.
    }

---

(Completely off-topic, I guess this is an example, but just in case, a reminder not to store plain text passwords.)

Answer 4

i don't understand, why you not watched Jeffry lesson, but for updating user you don't need try/catch section. you controller method:

public function update(UpdateUserRequest $request, User $user) : JsonResponse
{
   return response()->json($user->update($request->all()))
}

you request rules method:

public function rules(): array
{
    return [
        'username' => 'required|string',
        'password' => 'required|min:6|confirmed',
    ];
}

And you Exception Handler render method:

public function render($request, Exception $exception)
{
    if ($request->ajax() || $request->wantsJson()) {
        $exception = $this->prepareException($exception);

        if ($exception instanceof \Illuminate\Http\Exception\HttpResponseException) {
            return $exception->getResponse();
        } elseif ($exception instanceof \Illuminate\Auth\AuthenticationException) {
            return $this->unauthenticated($request, $exception);
        } elseif ($exception instanceof \Illuminate\Validation\ValidationException) {
            return $this->convertValidationExceptionToResponse($exception, $request);
        }

        // we prepare custom response for other situation such as modelnotfound
        $response = [];
        $response['error'] = $exception->getMessage();

        if (config('app.debug')) {
            $response['trace'] = $exception->getTrace();
            $response['code'] = $exception->getCode();
        }

        // we look for assigned status code if there isn't we assign 500
        $statusCode = method_exists($exception, 'getStatusCode')
            ? $exception->getStatusCode()
            : 500;

        return response()->json($response, $statusCode);
    }
    return parent::render($request, $exception);
}

Now, if you have Exception, Laravel give you in Json with status code != 200, else give success result!