Skip to content
This repository has been archived by the owner on Feb 7, 2024. It is now read-only.

[1.x] Add support for php-pusher 5.x #698

Merged
merged 2 commits into from
Feb 24, 2021
Merged

[1.x] Add support for php-pusher 5.x #698

merged 2 commits into from
Feb 24, 2021

Conversation

Koozza
Copy link
Contributor

@Koozza Koozza commented Feb 24, 2021

This package is almost compatible with php-pusher 5.x out of the box.

5.x expects a empty repsonse, or a response containing channel information (Expirimental) (See pusher docs: https://pusher.com/docs/channels/library_auth_reference/rest-api#successful-response)

This change looks to be backwards compatible with 4.x since the old php-pusher package didn't do anything with the responded data.

@vesper8
Copy link

vesper8 commented Feb 24, 2021

@mpociot @rennokki I'm hoping this one can be merged soon, can't upgrade to Pusher 5.0 without it. And of course.. can you please also include it in the awesome new 2.0 beta : ) tyvm!

@rennokki
Copy link
Collaborator

As far as I see, this is supported only for Laravel 8.29.0+ and the upcoming Laravel 9.x. I guess a constraint for illuminate/broadcasting to ^8.29 would work better to make sure things don't break?

Also, I'm not sure if 6.x or 7.x users would get it working because setting the pusher to ^4.0|^5.0 will install ^5.0 unless it's specified by the user as ^4.0 in the root composer.json file.

@Koozza
Copy link
Contributor Author

Koozza commented Feb 24, 2021

I could try to install a 6.x / 7.x project to test what happens if you'd like?

As far as I know laravel <8.29 will recomend ^4.0 and not ^5.0.

@Koozza
Copy link
Contributor Author

Koozza commented Feb 24, 2021

@rennokki I've given this a second thought. I don't think the 5.x upgrade in this project (the composer change) is really nessecary. 5.x clients would be able to connect with just the response change. 4.x clients shouldn't mind the change either.

Would that be better?

@rennokki
Copy link
Collaborator

We can use this specific check from Laravel core here for the empty response in case this changes a lot. And we can support both versions.

@Koozza
Copy link
Contributor Author

Koozza commented Feb 24, 2021

@rennokki but that check checks the server side version right? It's the client side version that matters.

So with a 4.x client you van connect the server, but with a 5.x client you cannot.

This is due to that the 5.x client expects a empty response (just like pusher itself does).

The 4.x clients just check for a 200 status code and returns the reponse to laravel, which ignores it.

So I don't see how this change (the empty reponse) will break anything. It's only more true to how pusher works.

@rennokki
Copy link
Collaborator

Right, also the tests run for both stable and lowest, so it's a good to go.

@rennokki rennokki merged commit 5cda453 into beyondcode:1.x Feb 24, 2021
@Koozza
Copy link
Contributor Author

Koozza commented Feb 24, 2021

Alright! Thanks!

@rennokki
Copy link
Collaborator

@vesper8 Tagged 2.0.0-beta.33

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants