laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.39k stars 10.98k forks source link

firstOrFail and findOrFail with custom message #23742

Closed axdemelas closed 6 years ago

axdemelas commented 6 years ago

Add an optional parameter "message" on Illuminate\Database\Eloquent\Builder:

API:

Method
findOrFail(mixed $id, array $columns = ['*'], $message = null)
firstOrFail(array $columns = ['*'], $message = null)

Usage:

$author = Author::findOrFail(2, ['*'], 'Second author not found');
$book = Book::firstOrFail(['*'], 'First book not found');
HSPDev commented 6 years ago

I stand corrected https://github.com/laravel/framework/issues/23742#issuecomment-378334816

I like the concept and have often missed this myself, but what about automatic injection of models?

public function show(Book $book) 
{
   //Code
}

Maybe we should have a protected parameter or function on the model class itself?

class Book extends Model 
{
    protected $notFoundMessage = 'The book could not be found';
}

This assumes a MVC design where the Model is "heavy", but maybe this could also be extracted elsewhere; Language files, a call in a Provider, or something "new"?

BrandonShar commented 6 years ago

This is a better fit for the laravel/ideas project, since it's a proposal, not an issue with the framework.

I'm not sure what you'd want this to do though. *orFail methods throw an exception if not found. Why do you want that exception to contain a user message?

HSPDev commented 6 years ago

@BrandonShar Because many projects have customized error views, containing code like this.

({{ $exception->getMessage() }})

It's to better the user experience, not just having a "404 page not found", but specific messages like "Sorry, we could not find your book.".

dwightwatson commented 6 years ago

Is it normal to display the exception message to the user? I'd worry that it might contain sensitive information or be more confusing to a user than just "whoops, something went wrong".

Miguel-Serejo commented 6 years ago

I would argue that the developer should know what constitutes sensitive information and what doesn't. In the given example, you "leak" information about whether or not a book is contained in your library. That's hardly sensitive information.

If you're using findOrFail to fetch a user's personal data, just don't use the additional custom error message.

devcircus commented 6 years ago

You should already be able to get any information you need from the exception then customize your "not found" page as needed.

HSPDev commented 6 years ago

I actually stand corrected on my previous comment. I'm using explicit model binding now, and using the clousure method of resolving my models. This let's me handle failures any way I want in a centralized location; I'm happy!

From my RouteServiceProvider

Route::bind('administrator', function ($id) {
        return Administrator::find($id) ?? abort(404, 'Administrator does not exist.');
});

I can also see how this affords much more advanced logic and I think it's a fair tradeoff instead of cluttering everybody's Eloquent.

olivernybroe commented 6 years ago

@HSPDev For reference, you can also do it in the model. So in your serviceProvider add

Route::model('administrator', Administrator::class);

and in your model do

public function resolveRouteBinding($id) {
    return Administrator::find($id) ?? abort(404, 'Administrator does not exist.');
}
twickstrom commented 3 years ago

I am creating a RestAPI and was looking for a solution that would dynamically generate a more user friendly response for dozens of models. This works well based on a few assumptions. Your Models ar CamelCased and they have meaningful names. Using @HSPDev's thought from above I also added the ability to override the dynamically created message on a model by model basis.

By adding the following to an model: public $modelNotFoundMessage = "The user was not found";

I am on Laravel 7. In: App\Exception\Handler.php

add use Illuminate\Database\Eloquent\ModelNotFoundException;

find the render method and add:

if ($exception instanceof ModelNotFoundException) {
    $model = explode('\\', $exception->getModel());
    $modelPhrase = ucwords(implode(' ',preg_split('/(?=[A-Z])/', end($model))));

    return response()->json([
        'error' => \App::make($exception->getModel())->modelNotFoundMessage ?? $modelPhrase . ' not found'
    ], 404);
}

Note, this will be called whenever you use: findOrFail, firstOrFail, or injection of a model into a controller method such as show, update, destroy etc.

stevebaros commented 3 years ago

public $modelNotFoundMessage = "The user was not found";

Still getting the default message "No query results for model [App\\Models\\Pos\\TaxItem] 496aab18-c039-4a1b-9b3e-1038326be12dlkl" after following this , FYI am using Lumen 7.X

RomuloLim commented 2 years ago

Hey @twickstrom, i implement using ValidationException (frontend can display the error more easily) and returning the parent::render case false. What do you think?

    public function render($request, Throwable $exception)
    {
        if ($exception instanceof ModelNotFoundException) {
            $model = explode('\\', $exception->getModel());
            $modelPhrase = ucwords(implode('',preg_split('/(?=[A-Z])/', end($model))));

            throw ValidationException::withMessages([
                $modelPhrase => \App::make($exception->getModel())->modelNotFoundMessage ?? $modelPhrase . ' not found'
            ]);
        }

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

I'm using Laravel 8.X

omarelewa21 commented 2 years ago

@RomuloLim Nice to add a status 404 to the ValidationException. throw ValidationException::withMessages([ $modelPhrase => \App::make($exception->getModel())->modelNotFoundMessage ?? $modelPhrase . ' not found' ])->status(404)

brijmansuriya commented 6 months ago

please add findOrFail() custom message

plumthedev commented 6 months ago

Dear @brijmansuriya You can customize your exception message using Handler::renderable on exception handler.

You can also always wrap *OrFail() in try/catch block and use abort helper - available from ages

use Illuminate\Database\Eloquent\ModelNotFoundException

try {
    return Airport::query()->where(...)->firstOrFail()
} catch (ModelNotFoundException) {
    abort(404, 'Airport cannot be found')
}