TryFind functions. Generate exception if elements not found.

[analitic1983]

Create tryFind(), tryFindOne(), tryFindByPk() wrappers. Wich will generate not found exception, if element not found.
It`s more simple to log errors.
Example from documentation: http://www.yiiframework.com/doc-2.0/guide-db-active-record.html

// "id" and "email" are the names of columns in the "customer" table
$customer = Customer::findOne(123);
$id = $customer->id;
$email = $customer->email;

in bad case will failed on line $customer->id; instead of correct error in logs.

[samdark]

Which Exception? It cannot be HttpException because these methods may be used in console. If it will be general purpose exception then you'll have to catch it and throw Http one:

try {
    $customer = Customer::findOne(123);
} catch (\Exception $e) {
    // throw 404
}
$id = $customer->id;
$email = $customer->email;

To me it's the same as doing:

$customer = Customer::findOne(123);
if (!$customer) {
    // throw 404
}
$id = $customer->id;
$email = $customer->email;

That's why I don't see how this addition is useful.

[analitic1983]

For example:
class NotFoundException extends \yii\db\Exception implements HttpExceptionInterface

If you want 404 code for rest Api, you should change \yii\web\Responce
public function setStatusCodeByException($e)
{
if ($e implements HttpExceptionInterface) { // Change to implements
$this->setStatusCode($e->statusCode);
} else {
$this->setStatusCode(500);
}

[samdark]

And if you're in console app?

[analitic1983]

Console app work correctly with \yii\db\Exception, and will correct work with: class NotFoundException extends \yii\db\Exception.

[samdark]

implements HttpException? Makes no sense in console app.

[analitic1983]

Yes, i correct it: HttpExceptionInterface

[samdark]

Still doesn't make sense to have anything about HTTP in console.

[analitic1983]

Ok. More abstraction? :)
Let`s use more common name. For example: implements responceStatusCodeInterface.

Which will return constants responceStatusCodeInterface::NOT_FOUND, responceStatusCodeInterface::SOMETHING_ELSE...

Which used by \web\Response to produce http return status codes.
Which can be used by \console\Response to produce console exit code.

[rob006]

@samdark This "HTTP stuff" is completely transparent for console. Interface may be useless at console level, but this does not mean that it does not make sense - it makes exception more flexible and nobody force you to handle HTTP-related interfaces in console.

[analitic1983]

Now console application always return "1" exit code, if catch exception . But it can be useful.
It can be used in bash scripts: https://shapeshed.com/unix-exit-codes/

[samdark]

OK. While I don't see any pros of having these methods myself, I want to hear more opinions.

[beroso]

I thought it cannot be useful, but now I'm not sure.
Laravel, for instance, provides a Model::findOrFail($pk) method, that throw a ModelNotFoundException if model cannot be found.
This approach can be useful when combined with events.

[samdark]

@berosoboy can not or can?

[rugabarbo]

I think it can be done as controller trait:

trait ActiveRecordFinderTrait
{
    public function tryFindOne($activeRecordClass, $id)
    {
        $model = $activeRecordClass::findOne($id);
        
        if ($model === null) {
            
            throw new NotFoundHttpException();
        }
        
        return $model;
    }

    // Other helpful methods
}

Then you can use it like this:

class ProfileController extends Controller
{
    use ActiveRecordFinderTrait;

    public function actionView($id)
    {
        $profile = $this->tryFindOne(Profile::class, $id);
        
        // Other stuff...
    }
}

[beroso]

@samdark Nowadays I do this way, like you suggested:

$customer = Customer::findOne(123);
if (!$customer) {
    // throw 404
}
$id = $customer->id;
$email = $customer->email;

But today I'm thinking that such methods (tryFind functions) can be useful.

[fcaldarelli]

It is just one more row. Is it really useful?

[beroso]

I'm not sure.
Gii also generates a method findModel in CRUD's Controllers (similar to @rugabarbo example).

[analitic1983]

$customer = Customer::findOne($id);
if (!$customer) {
    // throw 404
}

It`s common use case. Find element by identificator.
If i have element identification, in 90% element should be too.
Otherwise, where did we get its identifier?

Insert everywhere checks, it is lazy. You documentation show it :)

[beroso]

I was wondering something like this:

// Inside ActiveRecord
public function tryFindOne($pk) {
    if (($model = static::findOne($pk)) !== null) {
        return $model;
    } else {
        throw new ModelNotFoundException('Model of class ' . static::class . ' not found with pk = ' . $pk);
    }
}

So ModelNotFoundException could be handled separately in a custom ErrorHandler component, or in a custom ErrorAction. Regardless of the application is web or console.

[SamMousa]

I like the idea, currently what I do is implement the find method on the controller and throw exceptions.
In my opinion throwing an exception is better than returning null in case where you expect something to exist. It is different from findAll() where an empty list is not unexpected in most scenarios.

Doing return type checks makes it more complicated and is more error-prone than throwing exceptions. Even if user code doesn't handle the exception, it'll be a clear error.
The alternative approach (returning null combined with not checking return type) results in strange errors during view rendering.

Whether it is in the framework or not, it is of course easy to add to your base AR class.

[samdark]

Let's have pros and cons.

Nulls

• You have to handle both nulls and exceptions and handling null is shorter in the code.
• You can postpone failure or delegate handling to other places easily by passing null around.

Exceptions

• RecordNotFoundException would give better error than calling method X() on non-object.

[analitic1983]

In examples you use only one find method.

Real code use several find methods:

$user = User::tryFindOne(['id'=>$params['id']]);
$newCompany = Company::tryFindOne(['id'=>$params['company_id']]);
$invoice = Invoice::tryFindOne(['id'=>$params['ivoice_id']]);

Nothing need more. Execption will be cathed in \yii\base\ErrorHandler and writed to log. Working with incorrect params will be interrupted. It can use only one catcher for several find methods.

using null you should check every return value:

$user = User::findOne(['id'=>$params['id']]);
if (!$user) {
}
$newCompany = Company::findOne(['id'=>$params['company_id']]);
if (!$newCompany) {
}
$invoice = Invoice::findOne(['id'=>$params['ivoice_id']]);
if (!$invoice) {
}

You writting about error:

calling method X() on non-object.

In most of errors it's really so, but not in all
Example:

$invoice = Invoice::findOne(['id'=>$params['ivoice_id']]);
$invoiceInfo = json_encode($invoice); // Will be coorect json format
$queueInvoicesProcessor->sendNewInvoice($invoiceInfo); 

You will receive error in remote part of code, may be in next week.

Exceptions

• More compact and explicit code

[SamMousa]

I like @analitic1983 's arguments.

I'm not a fan of adding yet more static functions to the AR base class though.

I'd rather not use the shortcuts and do something like this:

// Edit: I realize this is not a good solution since the return type of `one()` will still need to include `null`. 
User::find()->where(['id' => 123])->expect(1)->one();
// This one does allow for proper typing.
User::find()->where(['id' => 123])->requireOne();

I think it's a query concern not a model one.
The advantage of this approach is that it always works not just from some shortcut cases.

Shorter code doesn't necessarily make it better imo:

• It becomes harder to understand (less self explanatory)
• It becomes harder to edit, since we must switch between formats.

Edit: my preferences in this case are about the approach to doing this, not so much about the names of the functions.

[samdark]

You're describing only one situation when you need all three find-s to find a record in order to continue. There's another case when not finding a record is fine. Mostly it's about non-ID conditions:

$primaryOperator = null;
try {
    $primaryOperator = User::tryFindOne(['type'=>'primary_operator']);
} catch (RecordNotFoundException $e) {
  // do nothing  
}

$secondaryOperator = null;
try {
    $secondaryOperator= User::tryFindOne(['type'=>'secondary_operator']);
} catch (RecordNotFoundException $e) {
  // do nothing  
}

$extraOperator = null;
try {
    $extraOperator= User::tryFindOne(['type'=>'extra_operator']);
} catch (RecordNotFoundException $e) {
  // do nothing  
}

$operator = $this->chooseOperator($primaryOperator, $secondaryOperator, $extaOperator);

vs current:

$primaryOperator = User::findOne(['type'=>'primary_operator']);
$secondaryOperator= User::findOne(['type'=>'secondary_operator']);
$extraOperator= User::findOne(['type'=>'extra_operator']);

$operator = $this->chooseOperator($primaryOperator, $secondaryOperator, $extaOperator);

[SamMousa]

@samdark we are talking about adding functionality right? Not replacing the existing functionality?

So you're free to use the current syntax in case you are fine with something failing?

[samdark]

Ah. Right right. Then I agree to add it. Looks useful.

[SamMousa]

Then the question that's open, is how.

OP suggests adding static shortcut function(s) to AR similiar to ::findOne().
I think a cleaner design is to have extra query functions in the Query class.

[asamats]

I think that the AR is not responsible for this is.

[mrbig00]

imo, the Model::findOrFail($pk) is shorter, but in just a few cases. If you have a simple crud operation it's okay, but if you need to find (findOrFail) for example 4 objects you have to add 4 try catch to each objects findOrFail method which is the same amount of code with the original findOne method.

[fcaldarelli]

I think that these methods will confuse developers, because they add (new?) functionalities that can be checked with a simple null comparison.

Less methods, more readability.

[SamMousa]

@mrbig00 I think you're misunderstanding some things.

When catching exceptions you only need to catch once. (Or even don't catch it but let your error handler handle it.)
The idea is you use this when you the record must exist.
The discussion at this point is where to add the extra functions.

The 2 options are:

User::findOrFail(); // Only supports the `findOne()` syntax.

User::find()->oneOrFail(); // Works for any query
```php

In both cases the name of the function is a separate discussion where I don't have a hard preference.
I do have a preference for the 2nd case since that is implemented on the query.

[SamMousa]

Checking nulls is not the same as throwing exceptions.
Exceptions imply that something is wrong and unexpected, return null doesn't have that implication.

Regardless, you are free to ignore the new functions, they will be documented so that should combat any confusion.

[fcaldarelli]

@SamMousa Said that, if you manage exceptions, you should check every type of exception (database not reachable, wrong field in sql, etc..), otherwise you will retrieve the same result of findOne() (simply record missing).

Finally, IMHO, when in reference I see two or more similar methods, I think to the reason because they exist, and this could be confuse.

[rob006]

@SamMousa Query is an abstraction for SQL queries. Returning empty result from SQL query is not wrong or unexpected. I don't think that this should be implemented at Query level.

Besides, this is only shortcut for throwing exception yourself or creating own method/trait which will implement it. Using it at query level will only make this shortcut harder to use.

[SamMousa]

@rob006 it is still query functionality. We have all kinds of shortcut methods for query: all(), scalar(), column(), one().

What's wrong with having a method that returns 1 record and throws an exception if it doesn't exist?

Besides, this is only shortcut for throwing exception yourself or creating own method/trait which will implement it. Using it at query level will only make this shortcut harder to use.

Writing 10 extra characters doesn't really make it that much harder. And having stricter typing (User instead of User|null makes things easier).

[rob006]

How do you want to get stricter typing at query level? At model level you can use static as a return type, at query level the best what you can get is ActiveRecord. Or actually ActiveRecord|array which may be even more confusing. We should keep it simple or don't do it at all.

[SamMousa]

The important thing for me is removing |null from the type.
For ActiveRecord::findOne() you currently have static|null and in you're right that that could be more specific: public static function findOrFail(): static.
I mostly use specific query classes that take care of the type hinting, as I prefer not to use the shortcuts.

You're right that if we want to have the correct type hint out of the box the shortcut method is a requirement.

Still in that case I'd like the shortcut method to forward the call to the query object instead of implementing it in the AR class itself.

[samdark]

Yes, having it in custom query object is an option we can show in the guide but I don't think it belongs to generic Query object.

[SamMousa]

So we don't add this functionality to the core?

[samdark]

We can in static shortcuts in AR instances but something tells me that these should be part of Repository classes.

[SamMousa]

Probably true, currently the static functions are our repositories...

[samdark]

So I think it's better to create a section in the guide or make a mini-book about architectural patterns with Yii and document it there.

[malsatin]

Idk, if this has been mentioned, but exception mechanism is expencive in any language. So, that is not great to force usage of slow mechanism in a framework that is supposed to be fast

[rob006]

@mattrh Exceptions are not slow. Not if you compare it with the whole process of fetching record from database and instantiating ActiveRecord object.

[Tigrov]

In the current implementation this could be done

1. Using additional paramener in ActiveQuery, e.g. private Throwable|bool $notFoundException = false;

$customerQuery = new ActiveQuery(Customer::class, $this->db);
$customer = $customerQuery->withNotFoundException()->findOne($condition);

2. Or with additional argument like ActiveQuery::onePopulate(Throwable|bool $notFoundException = false)

$customerQuery = new ActiveQuery(Customer::class, $this->db);
$customer = $customerQuery->findByCondition($condition)->onePopulate(true);

In both variants for $notFoundException can be a specified exception class or true to use by default RecordNotFoundException

[Tigrov]

Now can be realized as a Trait for active record model.

Related #344