Skip to content

VAPI-3160: Add <Refer> BXML Verb Support#88

Open
s-aher wants to merge 3 commits into
mainfrom
VAPI-3160
Open

VAPI-3160: Add <Refer> BXML Verb Support#88
s-aher wants to merge 3 commits into
mainfrom
VAPI-3160

Conversation

@s-aher

@s-aher s-aher commented May 28, 2026

Copy link
Copy Markdown

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 for referCompleteUrl, referCompleteMethod, tag, and nested <SipUri> child element
  • tests/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

<Response>
  <Refer referCompleteUrl="https://example.com/handleRefer" referCompleteMethod="POST">
    <SipUri>sip:alice@atlanta.example.com</SipUri>
  </Refer>
</Response>

Important Notes

  • Critical asymmetry vs <Transfer>: On success, the call is terminated — the remote SIP endpoint redirects away from Bandwidth entirely. referCompleteUrl should only be used for failure recovery.
  • ReferCompleteCallback webhook model will be auto-generated from the OpenAPI spec once api-specs PR #2142 is merged.
  • tag attribute included (pending final confirmation via VAPI-2916).

Related

Checklist

  • Refer verb model class with builder methods
  • Nested <SipUri> child element serializes correctly
  • Unit tests for <Refer> verb XML round-trip
  • README includes BXML example and failure-recovery example
  • No "continue after success" examples
  • SDK does not ship before api-specs PR #2142 is merged

@bwappsec

bwappsec commented May 28, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@s-aher s-aher marked this pull request as ready for review June 22, 2026 07:11
@s-aher s-aher requested review from a team as code owners June 22, 2026 07:11

@stampercasey stampercasey left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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;

Comment thread tests/ApiTest.php
@@ -236,8 +236,6 @@ public function testSyncTnLookup() {
$body->phoneNumbers = [getenv("USER_NUMBER")];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 ).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment thread src/Voice/Bxml/Refer.php
*
* @param SipUri $sipUri The SipUri destination for the REFER
*/
public function sipUri(SipUri $sipUri): Refer {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment thread src/Voice/Bxml/Refer.php
$element->setAttribute("tag", $this->tag);
}

if(isset($this->sipUri)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.');
}

Comment thread tests/BxmlTest.php
$responseXml = $response->toBxml();
$this->assertEquals($expectedXml, $responseXml);
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] No tests for ReferCompleteCallback and no test for Refer without SipUri

Two gaps:

  1. ReferCompleteCallback has 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).
  2. No test documenting what (new Refer())->toBxml($doc) produces when SipUri is not set. If a guard is added (see Refer.php comment), this becomes a expectException test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants