Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
stampercasey
left a comment
There was a problem hiding this comment.
Review from Claude Code — see inline comments for individual findings. Two blockers.
|
|
||
| /** | ||
| * @todo Write general description for this property | ||
| * @var string|null $referSipResponseCode public property |
There was a problem hiding this comment.
[BLOCKER] referSipResponseCode typed as string|null — should be int|null
SIP response codes are integers (202, 405, 503, etc.). Deserializing a real server payload into this class will leave the value as a string, breaking any comparison like === 202. Every other SDK types this correctly: C# uses int?, Python uses Optional[StrictInt].
Fix:
/** @var int|null $referSipResponseCode The SIP response code for the REFER request (e.g. 202, 405). */
public $referSipResponseCode;Same issue applies to notifySipResponseCode on line 95.
|
|
||
| /** | ||
| * @todo Write general description for this property | ||
| * @var string|null $notifySipResponseCode public property |
There was a problem hiding this comment.
[BLOCKER] notifySipResponseCode typed as string|null — should be int|null
Same issue as referSipResponseCode above. The SIP code reported via NOTIFY (e.g. 200, 404, 503) is an integer.
/** @var int|null $notifySipResponseCode The SIP response code from the NOTIFY (e.g. 200, 404). */
public $notifySipResponseCode;| @@ -236,8 +236,6 @@ public function testSyncTnLookup() { | |||
| $body->phoneNumbers = [getenv("USER_NUMBER")]; | |||
There was a problem hiding this comment.
[BLOCKER] Two assertions silently removed from testSyncTnLookup — unrelated to this PR
The deleted lines:
$this->assertIsArray($response->getResult()->links);
$this->assertInstanceOf(BandwidthLib\PhoneNumberLookup\Models\Link::class, $response->getResult()->links[0]);…have nothing to do with <Refer>. Removing assertions from an existing integration test to make it pass is a hidden regression. What broke here? If links no longer exists on the response model, that should be a separate PR with an explanation.
| /* | ||
| * BandwidthLib | ||
| * | ||
| * This file was automatically generated by APIMATIC v3.0 ( https://www.apimatic.io ). |
There was a problem hiding this comment.
[MINOR] File claims to be auto-generated but is hand-written
The APIMATIC v3.0 header is misleading — this file was written by hand. api-specs#2142 has now merged, so the actual regen job should be run to produce the real generated file (with correct types, proper docstrings, and the APIMATIC serialization scaffolding that matches the rest of the codebase). This placeholder should be replaced by regen output before merge.
| class ReferCompleteCallback implements \JsonSerializable | ||
| { | ||
| /** | ||
| * @todo Write general description for this property |
There was a problem hiding this comment.
[MINOR] All property docstrings are @todo placeholders
Every property has @todo Write general description for this property. These should describe the field — at minimum mirror the spec language used in the other SDKs (e.g. The outcome of the REFER operation. Either "success" or "failure". for referCallStatus). If this file will be replaced by regen, the stubs are fine as a temporary state, but that should be called out explicitly.
| * | ||
| * @param SipUri $sipUri The SipUri destination for the REFER | ||
| */ | ||
| public function sipUri(SipUri $sipUri): Refer { |
There was a problem hiding this comment.
[MINOR] SipUri carries Transfer-specific attributes
The SipUri class exposes transferAnswerUrl, transferAnswerFallbackUrl, uui, username, password, etc. as setters. Any of those set on a SipUri passed here will serialize into the <SipUri> element inside <Refer>, producing malformed BXML — with no warning.
Python introduced ReferSipUri (accepts only uri); C# uses a nested Refer.SipUri class. PHP should follow the same pattern to prevent misuse.
| $element->setAttribute("tag", $this->tag); | ||
| } | ||
|
|
||
| if(isset($this->sipUri)) { |
There was a problem hiding this comment.
[MINOR] No guard for missing SipUri — produces invalid BXML silently
<SipUri> is required by the spec ("exactly one"). With the current builder pattern, (new Refer())->toBxml($doc) produces <Refer/> — valid PHP, invalid BXML. Consider throwing in toBxml() when $this->sipUri is not set:
if (!isset($this->sipUri)) {
throw new \InvalidArgumentException('Refer requires a SipUri child element.');
}| $responseXml = $response->toBxml(); | ||
| $this->assertEquals($expectedXml, $responseXml); | ||
| } | ||
| } |
There was a problem hiding this comment.
[MINOR] No tests for ReferCompleteCallback and no test for Refer without SipUri
Two gaps:
ReferCompleteCallbackhas no test coverage at all — no round-trip JSON test, no scenario tests for the four outcomes (success, REFER rejected, destination unreachable, NOTIFY timeout). Every other SDK has these (Python, Ruby).- No test documenting what
(new Refer())->toBxml($doc)produces whenSipUriis not set. If a guard is added (seeRefer.phpcomment), this becomes aexpectExceptiontest.
Summary
Adds support for the new
<Refer>BXML verb to the PHP SDK, enabling SIP REFER functionality.Changes
src/Voice/Bxml/Refer.php— New BXML verb class with builder methods forreferCompleteUrl,referCompleteMethod,tag, and nested<SipUri>child elementtests/BxmlTest.php— Added unit tests for<Refer>XML serialization (with all attributes and minimal/no optional attributes)README.md— Added BXML example and failure-recovery example (no "continue after success" pattern, since the call terminates on success)Usage Example
Important Notes
<Transfer>: On success, the call is terminated — the remote SIP endpoint redirects away from Bandwidth entirely.referCompleteUrlshould only be used for failure recovery.ReferCompleteCallbackwebhook model will be auto-generated from the OpenAPI spec once api-specs PR #2142 is merged.tagattribute included (pending final confirmation via VAPI-2916).Related
Checklist
Referverb model class with builder methods<SipUri>child element serializes correctly<Refer>verb XML round-trip