Skip to content

Conversation

@nunomaduro
Copy link
Member

The contributor of this pull request is @spawnia, I've just made a few tweaks.

This pull request makes the ModelNotFoundException generic. So, in theory, once Eloquent is generic too, we will be able to do stuff like this:

try {
    User::findOrFail(1);
} catch (ModelNotFound $e) {
    $e->getModel()::myUserMethod(); // --> autocompletion, and static analysis support here.
}

@taylorotwell taylorotwell merged commit 788a18f into 9.x Feb 17, 2022
@nunomaduro nunomaduro deleted the feat/improve-model-not-found-types branch February 17, 2022 14:56
* The affected model IDs.
*
* @var int|array
* @var array<int, int|string>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have preferred array<int>|array<string> for two reasons.

  1. The type of a model ID should be monomorpic.
  2. The key of this array does not matter here, an associative array works just as well.

Comment on lines +6 to +7
/** @var ModelNotFoundException<User> $exception */
$exception = new ModelNotFoundException();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructing this class generically is always going to be awkward, since the generic type can not be derived from the constructor arguments. Perhaps it can be considered to move the arguments that are currently passed to setModel() into the constructor for the next major version of Laravel?

use Illuminate\Support\Arr;

/**
* @template TModel of \Illuminate\Database\Eloquent\Model
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ID type could be made generic too.

@spawnia
Copy link
Contributor

spawnia commented Feb 17, 2022

Thanks @nunomaduro for getting this merged. Perhaps we can iterate on it some more, although the biggest possible improvement to this would allowing inference of the generics through constructor arguments - this would require breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants