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.