-
-
Notifications
You must be signed in to change notification settings - Fork 438
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
Log in subscribers when broadcasting a subscription update #1306
Conversation
…ving subscription queries
@stayallive that approach looks solid. I especially like that it is pluggable, so there is little risk in experimenting a little. I am totally fine with shipping it once we add tests, docs and some polish. |
Do you want to add a configuration option to enable this behaviour so we can ship it without enabling it at all and if there are no issues we can enable it by default on v5 and remove the config options for it? |
I don't see a need for that, since this should work out of the box. If it needs a bugfix, we can add that. In case someone needs to revert to the old behaviour, they can rebind the interface. |
# Conflicts: # CHANGELOG.md # composer.json
tests/Unit/Subscriptions/Iterators/GuardContextSyncIteratorTest.php
Outdated
Show resolved
Hide resolved
tests/Unit/Subscriptions/Iterators/GuardContextSyncIteratorTest.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks really solid. I like the attention to detail, good naming and descriptive comments.
Fixes #1302.
Changes
Add a new interator used to iterator over subscription subscribers to restore the guard context when executing the GraphQL queries to broadcast to the subscribers.
Breaking changes
Might be considered a bugfix instead of a breaking change because
auth()->user()
is now set correctly to the subscriber user context when resolving subscription broadcasts.