2.0 findings: environment/restHost silently ignored, no-side constructor, RSL1h #230

These came up while validating the Pub/Sub 2.0 website docs against integration/v2 at fef205eac56bf7599f5eab23018a8b308b0cb777. They're grouped here so they can be triaged together. If you'd rather track any of them on its own, split it into a separate issue.

Two of these (items 1 and 2) are deliberate, documented decisions. For those, I'm asking you to reconsider or to document them more clearly; they aren't bug reports.

Summary

P1 = should be fixed before the 2.0 release · P2 = should be fixed soon · P3 = nice to have.

1. environment and restHost are silently ignored, so an un-migrated client connects to production (P1)

What's wrong

On integration/v2, environment and restHost are no longer ClientOptions properties. Unknown option keys are dropped without an error or a log line. So a 1.x application that upgrades without changing its options gets a client pointed at main.realtime.ably.net.

Evidence

• BaseOptions copies only keys that are declared properties (src/Models/BaseOptions.php#L14-L18), so it skips environment and restHost.
• endpoint then defaults to main (src/Models/ClientOptions.php#L181-L183), which resolves to main.realtime.ably.net (src/Defaults.php#L38-L46).
• Nothing is thrown or logged, even at Log::VERBOSE:
  • The array form (the one in the README and UPDATING.md) and new ClientOptions([...]) produce nothing at all.
  • PHP's own "Creation of dynamic property" deprecation appears only if the code assigns $options->environment = ... on an existing object, and production error reporting usually hides it.

// each of these sends GET https://main.realtime.ably.net:443/time for $client->time()
Server::createHttpClient(['key' => $sandboxKey, 'environment' => 'sandbox']);  // requests fail later with an error from production that doesn't mention the option
Server::createHttpClient(['key' => $key, 'environment' => 'acme']);            // dedicated cluster: traffic goes to the shared `main` cluster instead
Server::createHttpClient(['key' => $key, 'restHost' => 'rest.example.com']);   // custom host bypassed

(I didn't make a live call, so I haven't checked exactly what production returns for a sandbox key.)

{"environment":"sandbox"} -> endpoint=main primaryDomain=main.realtime.ably.net request=GET https://main.realtime.ably.net:443/time
{"environment":"acme"} -> endpoint=main primaryDomain=main.realtime.ably.net request=GET https://main.realtime.ably.net:443/time
{"restHost":"rest.example.com"} -> endpoint=main primaryDomain=main.realtime.ably.net request=GET https://main.realtime.ably.net:443/time
{"realtimeHost":"rt.example.com"} -> endpoint=main primaryDomain=main.realtime.ably.net request=GET https://main.realtime.ably.net:443/time
log lines emitted: 4
  LOG: ["Auth: anonymous, using basic auth"]   (x4, one per client)
--- new ClientOptions([... 'environment' => 'sandbox'])
isset($o->environment)=false primaryDomain=main.realtime.ably.net        <- no deprecation
--- $o = new ClientOptions(); $o->environment = 'sandbox';
Deprecated: Creation of dynamic property Ably\PubSub\Models\ClientOptions::$environment is deprecated in .../g1.php on line 32
primaryDomain=main.realtime.ably.net

History

• This is deliberate and documented. 7b72fbc (Replace environment and restHost with endpoint #229) removed the options and wrote the "ignored" wording in the same change. CHANGELOG.md#L20, UPDATING.md#L47 and #L107 all say the options are ignored and that such a client connects to production.
• Removing them is within the spec. TO3k1/TO3k2 let a library drop them.
• No reason is recorded for ignoring rather than rejecting. I couldn't find one in the commit, the Replace environment and restHost with endpoint #229 description or its review threads.
• Where the dropping comes from. BaseOptions has dropped unknown keys since 2015. In 1.x that was harmless, because these keys existed.

So this is a request to reconsider the failure mode before 2.0.0, not a bug report.

Impact

• The failure is silent misrouting, not an error. For dedicated-cluster and custom-host users, the requests can succeed against the shared cluster, so nothing signals that the configuration was lost. Release notes reach whoever reads them; a running application only gets what the code does.
• The other 2.0 SDKs don't let this through.
  • Java, .NET and Go removed the options, so un-migrated code fails to compile.
  • Python still maps them to endpoint and logs a warning.
  • PHP is the only one where the same upgrade mistake is silent.
• The decision window is now. Once 2.0.0 ships with "ignored", changing it to "rejected" is itself a breaking change.

Recommendation

• Reject the keys. In ClientOptions::__construct, throw AblyException (code 40000, status 400) when environment or restHost is present.
  • Optionally include realtimeHost too: PHP never had it, but other SDKs did.
  • Make the message name the replacement, for example "environment was removed in 2.0; use 'endpoint' => 'nonprod:sandbox' (see UPDATING.md)".
• Or at least warn. If throwing is too strict, Log::w() a warning.
• Tests and docs. Add a test, and change "ignored" to "rejected" (or "ignored with a warning") in CHANGELOG.md and UPDATING.md.

2. A client built with new PubSubHttpClient() declares no side, with no warning, and UPDATING.md:25 leads users to it (P2)

What's wrong

• Only Server::createHttpClient() adds ably-pubsub-server to the Ably-Agent header (src/Server.php#L54-L56, #L77-L97).
• The client constructor is still public, and only marked @internal (src/PubSubHttpClient.php#L76-L89).
• So a directly constructed client sends no side flag.

Evidence

From a local listener:

{"method":"GET","uri":"/g2-constructor","host":"127.0.0.1:18765","agent":"ably-pubsub-php/2.0.0 php/8.3.32"}
{"method":"GET","uri":"/g2-door","host":"127.0.0.1:18765","agent":"ably-pubsub-php/2.0.0 php/8.3.32 ably-pubsub-server"}

Nothing was printed or logged for the constructor path.

Three things make it easy to end up there without meaning to:

1. The upgrade guide's prose points at it.
   • The mapping table maps new AblyRest($opts) to Server::createHttpClient($opts) (UPDATING.md#L16).
   • The paragraph under it (UPDATING.md#L25) says the rename is mechanical: replace Ably\ with Ably\PubSub\ throughout, "and then AblyRest with PubSubHttpClient".
   • Applied to new AblyRest($opts), that gives new \Ably\PubSub\PubSubHttpClient($opts), which is the one form that declares no side. Service providers and DI bindings that construct the class directly end up in the same place.
2. Nothing signals it at runtime. @internal is visible only to static analysers and IDEs, and the client logs nothing.
3. The SDK's own docs disagree about the consequence.
   • The constructor docblock says such a client "does not qualify for the server exemption from monthly-active-user counting" (src/PubSubHttpClient.php#L79-L84).
   • README.md#L39, UPDATING.md#L83 and CHANGELOG.md#L16 say it "will be rejected" (or "is rejected") on accounts with monthly-active-user pricing.
   • Which one the platform actually does is outside the SDK; I didn't verify it.

History

• The no-side constructor is deliberate. Split Pub/Sub SDK: ably/pubsub-server package, Ably\PubSub namespace, server door #223 (5890766) says the constructor stays public and is marked @internal, and that a client built with it declares no side. tests/HttpTest.php#L161-L167 asserts it. I'm not asking to undo that.
• The misleading UPDATING.md:25 wording came later, by accident. The second half of that sentence ("and then AblyRest with PubSubHttpClient") came in with the rename in Rest* to Http* renaming  #228 (e79c2a9). Before that, the sentence covered only the namespace prefix.

Impact

• PHP is server-only, so every PHP client runs on a server, but a client without the flag isn't recognised as one.
• Per the SDK's own docs, such a client either loses the server exemption from monthly-active-user counting or is rejected outright.
• The user finds out from billing or from a rejection, not from the SDK.

Recommendation

• Before 2.0.0, fix the docs:
  • reword UPDATING.md:25 so the mechanical rename covers only type hints, catch blocks and string class names, and says that construction goes through Server::createHttpClient() (as the table at :16 already does);
  • make the docblock, README, UPDATING.md and CHANGELOG describe the consequence the same way.
• Then add a runtime signal, one of:
  • log a warning (Log::w) from the constructor when the resulting agents has no ably-pubsub-server entry, and add @deprecated to the constructor so IDEs strike it through; or
  • have the constructor declare ably-pubsub-server itself (PHP has only one side), keeping the door as the documented entry point. This reverses the decision in Split Pub/Sub SDK: ably/pubsub-server package, Ably\PubSub namespace, server door #223 and the test above, so it's the owner's call.

3. request(): HttpPaginatedResponse::$items is always empty for non-2xx responses (P2)

What's wrong

For a 4xx response, request() returns an HttpPaginatedResponse with these set:

• success and statusCode;
• errorCode/errorMessage, from the X-Ably-Errorcode/X-Ably-Errormessage headers.

But items is always [], so the response body can't be read.

Evidence

• src/Models/HttpPaginatedResponse.php#L49-L57 catches the AblyRequestException and keeps its response in the protected $response. That response's body is already decoded (src/Http.php#L175-L196).
• PaginatedResult fills items only on the success path (src/Models/PaginatedResult.php#L47-L79), so on an error it keeps its default [] (#L25).
• The spec doesn't limit items to success. HP3 defines items from the response body (a JSON array, or a single object wrapped in an array), with no 2xx restriction.
• ably-js returns the body in items for non-2xx request() responses (returnErrOnly/handlePage in paginatedresource.ts).

I tested it against a local server returning:

• a 400 with {"error":{"code":40020,...},"batchResponse":[...]};
• a 400 with a JSON array;
• a 404.

count($response->items) is 0 every time. A 200 returns the body as expected.

GET /ok                statusCode=200 success=true errorCode=NULL errorMessage=NULL count(items)=1
POST /err400obj-hdr    statusCode=400 success=false errorCode=40020 errorMessage='Batched response includes errors' count(items)=0
POST /err400obj        statusCode=400 success=false errorCode=NULL errorMessage=NULL count(items)=0
POST /err400arr        statusCode=400 success=false errorCode=NULL errorMessage=NULL count(items)=0
GET /err404            statusCode=404 success=false errorCode=NULL errorMessage=NULL count(items)=0

Example: the REST API docs describe a partially failed batch publish or batch presence request as 400 / 40020, with a batchResponse array of per-channel results. In PHP, $response->items[0]->batchResponse is unreachable because items[0] is unset, so the per-channel successes and errors are lost.

(I didn't verify which batch format the server returns for PHP's X-Ably-Version: 2. The UTS spec says protocol 3 and later return partial failures as HTTP 200. The core problem doesn't depend on this: no 4xx body is reachable.)

History

• This has been the behaviour since request() was added in 1.x. The catch branch has been the same since 3a66ba8 (2016).
• It wasn't designed to drop the body. The docblock only says the method rethrows for status 500 and above.
• It isn't a 2.0 regression. But the fix changes what items holds on an error, so 2.0 seems the right release to make it.

Impact

PHP users of request() can't read error or batch bodies, and they lose per-channel results on partial batch failures. The 2.0 docs currently carry a PHP-only caveat for this.

Recommendation

• In the catch branch, fill items from $this->response['body'] the same way the success path does: wrap a single object in an array, and map through the Untyped model when one is given.
• Keep the existing success/statusCode/errorCode/errorMessage handling.
• Add a unit test with a mocked 400 response that carries a body, checking both items and errorCode.

4. Decide RSL1h before release: publish($name, $data, $clientId, $extras) is still accepted (P2)

What's wrong

Channel::publish() still accepts extra arguments to the (name, data) form:

• publish($name, $data, $clientId)
• publish($name, $data, $extras)
• publish($name, $data, $clientId, $extras)

They sit under // TODO RSL1h: Remove clientId/extras extras support for 2.0 (src/Channel.php#L125-L139).

Evidence

RSL1h: "The publish(name, data) form should not take any additional arguments. If a client library has supported additional arguments to the (name, data) form … in any 1.x version, it should continue to do so until version 2.0."

Nothing on integration/v2 settles it yet:

• The RSL1h forms aren't mentioned in UPDATING.md or CHANGELOG.md, either way.
• By contrast, Auth::authorise() is documented as kept (UPDATING.md#L111).

History

• This was deliberately deferred to 2.0, then never decided. The TODO dates from 2019, when these forms were marked deprecated in a code note (see the discussion on Need to support Push payloads in publish() #82, where jdavid noted adding it).
• Split Pub/Sub SDK: ably/pubsub-server package, Ably\PubSub namespace, server door #223 planned "the deprecated-surface deletions" alongside the 091d rename.
• Rest* to Http* renaming  #228 then did the rename and documented authorise() as kept, but didn't decide RSL1h.

This item asks for that decision; either answer is fine.

Impact

• Removing the forms is a breaking change, so 2.0.0 is the release to do it in.
• If 2.0 ships with neither the removal nor a note, the next opportunity is 3.0, and the TODO keeps promising something that didn't happen.

Recommendation

Either:

• remove the 3- and 4-argument handling, so publish() takes ($name, $data) or (Message|Message[] $messages, array $params = []), and add a row to UPDATING.md: publish($name, $data, $clientId, $extras) → publish(new Message([...name, data, clientId, extras])); or
• keep them, delete the TODO, and add a line next to the authorise() note in UPDATING.md saying they're kept in 2.0.

5. phpdoc types that static analysers reject, null/object auth arguments that throw TypeError, and two error-path bugs (P3)

What's wrong

Small, non-breaking fixes to the public surface. All of them reproduce on integration/v2.

1. phpdoc that static analysers reject or that invites a TypeError (checked with PHPStan 2.3.1, level 5):

• publish() tags: Channel declares @method publish(Message $message) and @method publish(string $name, string $data) (src/Channel.php#L16-L17).
  • PHPStan keeps only the second, so it reports $channel->publish($message) as "invoked with 1 parameter, 2 required".
  • It also flags publish([$m1, $m2]) and publish($message, $params), although both are supported (#L62-L74, #L142-L147).
• stats(): PubSubHttpClient::stats() is @return array, but it returns a PaginatedResult (src/PubSubHttpClient.php#L121-L128).
• createTokenRequest(): it documents @param TokenParams $tokenParams / @param AuthOptions $authOptions (src/Auth.php#L284-L285), but only arrays work. createTokenRequest(new TokenParams([...])) throws TypeError from array_key_exists at #L292 (with an AuthOptions object, at #L291).
• PresenceMessage: $action is documented as "The timestamp for this message", and $memberKey as @var int (src/Models/PresenceMessage.php#L6-L15).

2. null auth arguments throw, although the docblocks allow them. authorize() and requestToken() document array|null for both arguments (src/Auth.php#L131-L132, #L179-L180), but:

• authorize(null) / authorize([], null) throw TypeError from array_merge (#L116-L117);
• requestToken(null) / requestToken([], null) throw from array_key_exists (#L191, #L193);
• createTokenRequest(null) behaves the same way (#L291-L292).

RSA8e, RSA9h and RSA10j make both arguments optional.

3. Two error-path bugs:

• BaseOptions::fromJSON() throws AblyException without importing it (src/Models/BaseOptions.php#L1-L4, #L37). So (new TokenDetails())->fromJSON('not json') (or any other model) fails with Error: Class "Ably\PubSub\Models\AblyException" not found instead of an AblyException.
• request() passes code and status the wrong way round: new AblyException('GET requests cannot have a JSON body', 400, 40000) (src/PubSubHttpClient.php#L279). The constructor is ($message, $code, $statusCode), so getCode() returns 400 and getStatusCode() returns 40000. (The $method == 'GET' check on the line above is case-sensitive, which relates to Passing lowercase get to the sdks request method results in a curl GET request with data #217.)

Evidence

createTokenRequest([ttl=>1000])                      OK Ably\PubSub\Models\TokenRequest
createTokenRequest(new TokenParams([ttl=>1000]))     TypeError: array_key_exists(): Argument #2 ($array) must be of type array, Ably\PubSub\Models\TokenParams given
createTokenRequest([], new AuthOptions([]))          TypeError: array_key_exists(): Argument #2 ($array) must be of type array, Ably\PubSub\Models\AuthOptions given
createTokenRequest(null)                             TypeError: array_key_exists(): Argument #2 ($array) must be of type array, null given
requestToken(null)                                   TypeError: array_key_exists(): Argument #2 ($array) must be of type array, null given
requestToken([], null)                               TypeError: array_key_exists(): Argument #2 ($array) must be of type array, null given
authorize(null)                                      TypeError: array_merge(): Argument #2 must be of type array, null given
authorize([], null)                                  TypeError: array_merge(): Argument #2 must be of type array, null given
(new ClientOptions())->fromJSON("not json")          Error: Class "Ably\PubSub\Models\AblyException" not found
request('GET','/x',[],['a'=>1])                      Ably\PubSub\Exceptions\AblyException: GET requests cannot have a JSON body [getCode()=400 getStatusCode()=40000]

History

All of these predate 2.0 and look accidental:

• the @method tags, the stats() and PresenceMessage docblocks, and the array|null docblocks date from 2015;
• the array_merge/array_key_exists calls date from 2015-2016;
• the missing import dates from 2015;
• the swapped arguments date from 2016 (restored by a 2022 revert).

Impact

• PHPStan and Psalm users get false errors on documented calls (publish($message), publish([...]), stats()).
• Code that follows the SDK's own docblocks (TokenParams objects, null arguments) gets a TypeError at runtime.

Recommendation

• phpdoc:
  • replace the two @method tags with one that covers every form, e.g. @method bool publish(string|Message|Message[] $nameOrMessages, mixed $dataOrParams = null) (or a real signature);
  • fix the stats(), createTokenRequest() and PresenceMessage docblocks (@var int action constants; $memberKey is a string).
• Auth arguments:
  • at the top of authorize(), requestToken() and createTokenRequest(), normalise with $tokenParams = $tokenParams ?? []; $authOptions = $authOptions ?? [];;
  • either accept TokenParams/AuthOptions objects (->toArray()), or change the createTokenRequest() docblock to array.
• Error paths:
  • add use Ably\PubSub\Exceptions\AblyException; to BaseOptions.php;
  • swap the arguments to new AblyException('GET requests cannot have a JSON body', 40000, 400).

Checklist

• environment/restHost rejected (or warned on) instead of silently ignored, with CHANGELOG/UPDATING updated
• UPDATING.md:25 fixed, the docs agree on the no-side consequence, and the constructor warns (or declares the side)
• HttpPaginatedResponse::$items filled from the error body for non-2xx
• RSL1h decided: forms removed with an UPDATING row, or kept and documented
• phpdoc, null/object auth arguments, fromJSON import, swapped request() exception arguments

Related

• 2.0 release PR: Integration branch for ably/pubsub-server v2 package #227
• Replace environment and restHost with endpoint #229: the environment/restHost → endpoint change
• Split Pub/Sub SDK: ably/pubsub-server package, Ably\PubSub namespace, server door #223 (the server door and the no-side constructor) and Rest* to Http* renaming  #228 (the AblyRest → PubSubHttpClient rename)
• Need to support Push payloads in publish() #82: the 2019 RSL1h discussion
• Passing lowercase get to the sdks request method results in a curl GET request with data #217: case-sensitive GET check in request(), in the same lines

┆Issue is synchronized with this Jira Task by Unito