search

ably / ably-php

PHP client library SDK for Ably realtime messaging service
https://ably.com/download
Apache License 2.0
54 stars 11 forks source link

Fix batch publish #198

Closed sacOO7 closed 7 months ago

sacOO7 commented 7 months ago

Related to #197

sacOO7 commented 7 months ago

@AndyTWF I agree with your point. Though according to https://faqs.ably.com/do-you-binary-encode-your-messages-for-greater-efficiency, ably uses msgpack as a default encoding. Also, spec regarding AblyRest->Request is not clear when we recommend msgpack as a default encoding. It says to accept JsonObject | JsonArray body? as a param ( not a plain string though ). Depending on the useBinaryProtocol clientOptions, encoding will be applied internally.

sacOO7 commented 7 months ago

I feel use of type string for payload should be avoided at all cost ( atleast for batch publish ). We should be documenting the same across SDKs.

sacOO7 commented 7 months ago

@AndyTWF I have added note for users wanting to use json => https://github.com/ably/ably-php/pull/198/commits/b37daab4f0621f912dc1fa8f9e603f3e40b14e66

AndyTWF commented 7 months ago

I feel use of type string for payload should be avoided at all cost ( atleast for batch publish ). We should be documenting the same across SDKs.

In other SDKs (e.g. Java) we avoid the issue because the body param has to match an interface that can only be one of msgpack or json.

Next time there's a breaking version of this SDK, we should consider enforcing parameters types on the request method (e.g. an array for the params).