Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[10.x] Fix typehint for EventServiceProvider::$observers property #47316

Merged
merged 1 commit into from
Jun 2, 2023
Merged

[10.x] Fix typehint for EventServiceProvider::$observers property #47316

merged 1 commit into from
Jun 2, 2023

Conversation

bradietilley
Copy link
Contributor

Issue:

New typehint was added d800f9e which is too strict (only supports array of strings).

image

My syntax is valid (unless string-to-string mapping is now deprecated in which case whoops I missed the memo). AFAICT it's still valid in HasEvents:

image


Also, while you can't instantiate objects directly in the property declaration, it's still important to define a typehint that supports objects, otherwise the following valid logic would appear invalid in static analysis:

// EventServiceProvider
$model = \App\Models\User::class;
$object = new \App\Observers\UserObserver();

$this->observers[$model] = $object;

I mean, I never use objects in this context, but it's technically valid so it should be typehinted as valid too.

@bradietilley bradietilley changed the title [10.x] Fix typehint for EventServiceProvider observer property [10.x] Fix typehint for EventServiceProvider::$observers property Jun 2, 2023
@driesvints driesvints requested a review from nunomaduro June 2, 2023 08:29
@cosmastech
Copy link
Contributor

cosmastech commented Jun 2, 2023

#46942 (comment)

I had noted that this should be a class-string and not just a string for the docblock type hint too. Maybe @nunomaduro can chime in on that, I'm not sure how much Laravel team uses class-string but I personally love the precision 😄

@bradietilley
Copy link
Contributor Author

@cosmastech Yeah I was itching to do that too, instead I just emulated what was done with the $listen property. Keen for class-string if that's everyone else's preference 👍

@taylorotwell taylorotwell merged commit 4d891db into laravel:10.x Jun 2, 2023
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.

3 participants