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

Subscription send only to others #2298

Merged
merged 27 commits into from
Feb 14, 2023
Merged

Conversation

Stevemoretz
Copy link
Contributor

@Stevemoretz Stevemoretz commented Feb 14, 2023

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

A fix for #2293

Changes

Subscriptions can now be filtered via $subscriber->socket_id and request()->header("x-socket-id"), which is similar to what https://laravel.com/docs/9.x/broadcasting#only-to-others does.

Breaking changes

Nope.

Copy link
Collaborator

@spawnia spawnia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test?

Also, please update docs/master and do not change docs/5

CHANGELOG.md Outdated Show resolved Hide resolved
docs/5/subscriptions/filtering-subscriptions.md Outdated Show resolved Hide resolved
docs/5/subscriptions/filtering-subscriptions.md Outdated Show resolved Hide resolved
docs/5/subscriptions/filtering-subscriptions.md Outdated Show resolved Hide resolved
docs/5/subscriptions/filtering-subscriptions.md Outdated Show resolved Hide resolved
docs/5/subscriptions/filtering-subscriptions.md Outdated Show resolved Hide resolved
docs/5/subscriptions/filtering-subscriptions.md Outdated Show resolved Hide resolved
docs/5/subscriptions/filtering-subscriptions.md Outdated Show resolved Hide resolved
src/Subscriptions/Subscriber.php Outdated Show resolved Hide resolved
src/Subscriptions/Subscriber.php Outdated Show resolved Hide resolved
@spawnia spawnia added the enhancement A feature or improvement label Feb 14, 2023
@Stevemoretz
Copy link
Contributor Author

Can you add a test?

Also, please update docs/master and do not change docs/5

Thanks for reviewing this and sorry about the docs, fixed.

I couldn't find any tests for filtering if there is one please tell me where it is then I probably can write a test for this based on that

@spawnia
Copy link
Collaborator

spawnia commented Feb 14, 2023

Not sure if there are any tests for it. Try adding something in https://github.com/nuwave/lighthouse/tree/master/tests/Integration/Subscriptions.

@spawnia spawnia marked this pull request as draft February 14, 2023 15:45
@Stevemoretz
Copy link
Contributor Author

Not sure if there are any tests for it. Try adding something in https://github.com/nuwave/lighthouse/tree/master/tests/Integration/Subscriptions.

Thanks that's ok, I'll write one for storage of socket_id that should be good enough.

@Stevemoretz
Copy link
Contributor Author

Not sure if there are any tests for it. Try adding something in https://github.com/nuwave/lighthouse/tree/master/tests/Integration/Subscriptions.

All done test was added too, tested locally all passed, did cs-fixer and php stan locally no errors.

The errors in the CI seem to be unrelated to the commits you might wanna check on them.

@spawnia
Copy link
Collaborator

spawnia commented Feb 14, 2023

@Stevemoretz
Copy link
Contributor Author

Stevemoretz commented Feb 14, 2023

https://github.com/nuwave/lighthouse/actions/runs/4175774729/jobs/7231159976 is definitely related.

Sure you're right, I don't get that locally different php version or dependencies maybe

/**
     * X-Socket-ID header passed on the subscription query.
     */
    public string|array|null $socket_id;

This would fix it for php 8 I think? But it will break it for php 7, what do you suggest? Would this fix it for all versions? Works for me:

    /**
     * X-Socket-ID header passed on the subscription query.
     * @var string|null|array<mixed>
     */
    public $socket_id;

I'm not really good with different php versions yet I usually use one

@Stevemoretz
Copy link
Contributor Author

@spawnia Yep that fixed it, the rest is all:

Error: Your requirements could not be resolved to an installable set of packages.

eg: https://github.com/nuwave/lighthouse/actions/runs/4176052100/jobs/7231826956

Copy link
Collaborator

@spawnia spawnia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good test

@spawnia spawnia marked this pull request as ready for review February 14, 2023 22:02
@spawnia
Copy link
Collaborator

spawnia commented Feb 14, 2023

Merging master should fix CI.

@spawnia spawnia merged commit 722ff4b into nuwave:master Feb 14, 2023
@Stevemoretz
Copy link
Contributor Author

@spawnia Thanks for working on this with me I enjoy doing PRs with you!

@spawnia
Copy link
Collaborator

spawnia commented Feb 14, 2023

Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants