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

[5.6] [WIP] Add notification database channel custom types #23245

Closed
wants to merge 1 commit into from
Closed

[5.6] [WIP] Add notification database channel custom types #23245

wants to merge 1 commit into from

Conversation

antonkomarev
Copy link
Contributor

@antonkomarev antonkomarev commented Feb 21, 2018

Related to: #23236

This PR introduces the same behavior to the database notification channel to keep notification channels data consistency.

Use case: I'm adding notification to database and broadcasting them to the user. But if user has missed notifications - he can call GET /notifications later and get the same data which was broadcasted earlier. Otherwise notification type will be mismatch and there will be issues with data merge.

The only thing I don't like in this implementation is method naming. Type resolver checks if broadcastType() method exists in Notification. Earlier I suggested name broadcastAs() inspired from Broadcasting Event. Taylor's name broadcastType() is way better, but still coupled with broadcasting. Maybe it's better to rename it to more generic getType()?

@antonkomarev
Copy link
Contributor Author

Or there could be issues with instantiating correct notification type from the database record?

@taylorotwell
Copy link
Member

broadcastType doesn't make any sense. This has nothing to do with broadcasting.

@antonkomarev
Copy link
Contributor Author

@taylorotwell I understand it. But how this could be implemented then? I want to have same data in database and in broadcasting messages. Create custom driver in application which will include this PR?

@antonkomarev antonkomarev deleted the feature/notification-database-channel-custom-type branch February 28, 2018 23:37
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.

2 participants