ably / laravel-broadcaster

Official Laravel Ably Broadcaster
https://laravel.com/docs/broadcasting
Apache License 2.0
47 stars 7 forks source link

Remove "socket" key from message data #18

Closed kwhohasamullet closed 1 year ago

kwhohasamullet commented 1 year ago

Noticed that all messages being sent form Laravel broadcast events included an extra socket key in the message data. Depending on the structure of data I was sending from Laravel (and in turn receiving in the client) this extra attribute could cause the data structure to be different from what was expected and cause errors.

Screenshot 2022-11-22 at 8 22 39 pm

It appears that this socket value comes from the InteractsWithSockets trait which is included by default in all new events created by artisan so this should be handed as the default. Using the Pusher implementation as inspiration it appears they are handling this by pulling the socket key from the payload array before building up the message.

https://github.com/illuminate/broadcasting/blob/68939fc64fb3d553e066dbfcee7c0d835d7d24bc/Broadcasters/PusherBroadcaster.php#L151

This pull request implements the same logic that is used by the Pusher implementation. The Arr::pull function returns a default value of null which is the same as data_get function currently used.

sacOO7 commented 1 year ago

Hi @kwhohasamullet, thanks for raising the PR. we will take a look 👍

kwhohasamullet commented 1 year ago

Hi @sacOO7.

I do not have a huge amount of experience writing unit tests. Had a quick go at it below at am able to get the serialised string when it hits the post message but the de-serialisation does not seem straight forward (and I imagine is not really required in the php SDK). Strictly speaking it would be easier to test the buildAblyMessage method directly but it is a protected method and a quick google suggests the general view is not to test protected methods (and if you do requires using reflection to make it public for testing)

If there is an easier way to do this that I am not seeing and you can nudge me in the right direction I am happy to keep looking at this - else I imagine it would be a pretty simple test for someone with experience to write.

image

sacOO7 commented 1 year ago

I do not have a huge amount of experience writing unit tests. Had a quick go at it below at am able to get the serialised string when it hits the post message but the de-serialisation does not seem straight forward (and I imagine is not really required in the php SDK). Strictly speaking it would be easier to test the buildAblyMessage method directly but it is a protected method and a quick google suggests the general view is not to test protected methods (and if you do requires using reflection to make it public for testing)

@qsdstefan would you be able to take a look at this? This does look simple ...

qsdstefan commented 1 year ago

@sacOO7 @kwhohasamullet I've decided to expose the protected method by creating a new class that inherits AblyBroadcaster and provides a single public method which we use in the test. This seems a bit cleaner than using reflection.