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] Add the ability to extend the generic types for DatabaseNotificationCollection #47048

Merged
merged 7 commits into from
May 12, 2023
Merged

[10.x] Add the ability to extend the generic types for DatabaseNotificationCollection #47048

merged 7 commits into from
May 12, 2023

Conversation

phh
Copy link
Contributor

@phh phh commented May 12, 2023

In my project in currently extending the DatabaseNotification and DatabaseNotificationCollection to add on top of the existing Laravel notifications.

However, when extending the DatabaseNotificationCollection with my own custom one, I'm getting errors, since I'm unable to provide a TKey and a TModel to the DatabaseNotificationCollection (like I would be able to do if I were instead extending the Illuminate\Database\Eloquent\Collection class.

Adding this markup makes it possible to add the following to my own Collection:

/**
 * @extends DatabaseNotificationCollection<int, Notification>
 */
class NotificationCollection extends DatabaseNotificationCollection

While without it gives me the following error in PHPStan:

Class App\Collections\NotificationCollection @extends tag contains incompatible type Illuminate\Notifications\DatabaseNotificationCollection&iterable<int, App\Models\Notification>.

@nunomaduro
Copy link
Member

Can you add a simple test here https://github.com/laravel/framework/tree/10.x/types?

@nunomaduro nunomaduro self-assigned this May 12, 2023
@nunomaduro nunomaduro marked this pull request as draft May 12, 2023 10:35
Patrick Hesselberg added 2 commits May 12, 2023 13:09
@phh
Copy link
Contributor Author

phh commented May 12, 2023

@nunomaduro Thanks for getting back.

This is first time for me writing these tests. Added in da93203 - something like that?

Patrick Hesselberg added 2 commits May 12, 2023 15:20
@phh phh marked this pull request as ready for review May 12, 2023 13:36
@nunomaduro nunomaduro marked this pull request as draft May 12, 2023 14:12
@nunomaduro nunomaduro marked this pull request as ready for review May 12, 2023 14:17
@taylorotwell taylorotwell merged commit 0cb3a55 into laravel:10.x May 12, 2023
milwad-dev pushed a commit to milwad-dev/framework that referenced this pull request May 12, 2023
…cationCollection (laravel#47048)

* Add the ability to extend the generic types

* Update DatabaseNotificationCollection.php

* Remove unused

* Add tests

* Fix style

* rm lb

* Updates localtion of type tests

---------

Co-authored-by: Nuno Maduro <enunomaduro@gmail.com>
@phh phh deleted the database-notification-generics branch May 12, 2023 17:02
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.

None yet

3 participants