pusher / pusher-http-php

PHP library for interacting with the Pusher Channels HTTP API
https://pusher.com/docs/server_api_guide
1.4k stars 306 forks source link

Batch message sending does not add socket_id from event #365

Closed Justinas-Jurciukonis closed 1 year ago

Justinas-Jurciukonis commented 1 year ago

Pusher client discards socket_id when sending batch of messages

We are sending messages in batches:

foreach ($messagesChunks as $messages) {
  $pusher->triggerBatch($messages, true);
}

Sample body:

[
   {
      "channel": "channel-id-123",
      "name": "action-name",
      "data": "**REDACTED**",
      "socket_id": "442535.6684692"
    }
]

Problem with this is that Pusher::make_trigger_batch_body validates $event['socket_id'], but later just discards it.


Adding socket_id into parameters solves this issue Was this intensional or is it bug in Pusher client?

Screenshot 2022-12-06 at 15 16 54 Screenshot 2022-12-06 at 15 35 08
Justinas-Jurciukonis commented 1 year ago

Also documentation is wrong. It states, that you must provide not indexed array as ['socket_id' => $socket_id], but code actually checks for $event['socket_id'] and not $event[0]['socket_id']

benjamin-tang-pusher commented 1 year ago

Hi, I will take a look at this. It's likely this is an oversight, since the readme shows passing ['socket_id' => $socket_id] with triggerBatch($batch).

benjamin-tang-pusher commented 1 year ago

The issue with adding adding socket_id back in at Pusher.php:1171 is that you don't always need to specify a socket_id, and if you don't, the library (correctly) throws a warning that PHP Warning: Undefined array key "socket_id".

I need to ask around whats the best way of doing this. The (isset($event['socket_id'])) might be used for this.

Justinas-Jurciukonis commented 1 year ago

Where exactly it's thrown? And yes, most likely isset($event['socket_id']) should solve the issue.

Justinas-Jurciukonis commented 1 year ago

I see you have created PR for this issue.

To fix failing tests, replace assertObjectHasProperty with assertTrue(property_exists) (or update PHPUnit to 10.1 version)

benjamin-tang-pusher commented 1 year ago

https://github.com/pusher/pusher-http-php/pull/370 was merged, and the new version includes the socket id in batch trigger requests.

I will close this ticket. Thanks for bringing this to our attention.