[12.x] use type declarations for Auth events #53969
Conversation
|
Thanks for submitting a PR! Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
| * @var \Illuminate\Http\Request | ||
| */ | ||
| public $request; | ||
|
|
||
| /** | ||
| * Create a new event instance. | ||
| * | ||
| * @param \Illuminate\Http\Request $request |
There was a problem hiding this comment.
Do we want to add the docblock here that we would otherwise lose?
There was a problem hiding this comment.
good catch, thanks
| public $guard, | ||
| public $user, | ||
| public string $guard, | ||
| public Authenticatable $user, |
There was a problem hiding this comment.
There was a problem hiding this comment.
yah, i need to dig into this PR a little more. this is where it gets tricky, because do we change what the Event accepts? or do we force the calling code to actually follow what the event wants.
even with just this small PR, it's clear there are some things that just naturally slipped through since we've been looser with our types so far.
There was a problem hiding this comment.
I was chatting to Taylor about this and suggested we go deeper on static analysis insights before we make refactors like this. Hopefully that would help expose places where the dockblocks are lying.
We don't want to be unintentionally introducing breaking changes.
There was a problem hiding this comment.
yah, anything we can do to move us towards more strict typing I'm down for that.
I'd think I'd be on the side of forcing the calling code to adjust, for the most part.
glad you guys are talking about it internally. would love to help how I can.
this one is a little more curious than the last. there are some events that are completely unused. 1 that is only used in tests.
some of the calling code also only ensures the correct type based on a docblock and not an actual type declaration. honestly not surprised to see that. imagine that will just be part of the process.
we also have a couple scenarios again where the event is expecting a class, but the code can technically (but unlikely or impossible) pass
null.