smartystreets / smartystreets-php-sdk

The official client libraries for accessing SmartyStreets APIs from the PHP Hypertext Processor.
https://smartystreets.com/docs/sdk/php
Apache License 2.0
28 stars 15 forks source link

[4.19.1] SDK should never be printing/echoing data #47

Closed shengslogar closed 5 months ago

shengslogar commented 9 months ago

SDKs should, in principle, be headless. https://github.com/smartystreets/smartystreets-php-sdk/commit/bdc39a1194d51dc0f50be2b90e3508e1ad899c27, tagged 4.17.1, added an echo statement to RetrySender without any rationale provided in the changelog or commit message, perhaps an artifact left over from internal debugging?

Searching across the codebase yields the following instances of this:

https://github.com/smartystreets/smartystreets-php-sdk/blob/15aef42018c7ea7b39e89fe821da24a663876a8f/src/US_Reverse_Geo/Client.php#L32

https://github.com/smartystreets/smartystreets-php-sdk/blob/15aef42018c7ea7b39e89fe821da24a663876a8f/src/RetrySender.php#L60

https://github.com/smartystreets/smartystreets-php-sdk/blob/15aef42018c7ea7b39e89fe821da24a663876a8f/src/US_Enrichment/Client.php#L40

Happy to provide an example explaining why this is causing issues in our codebase if it's not self-evident.

EDIT: if output is required, hooking into ClientBuilder's withDebug mode might be appropriate.

shengslogar commented 9 months ago

A temporary non-ideal workaround is to capture and flush output using object buffering:

/** @var \SmartyStreets\PhpSdk\US_Autocomplete_Pro\Client $autocompleteClient */

\ob_start();
$autocompleteClient->sendLookup(...);
\ob_clean();

// Use results of lookup...

Must also take care to \ob_clean in the event of a SmartyStreets\PhpSdk\Exceptions\SmartyException.

smartyeric commented 5 months ago

Hi

It looks like those echo statements were indeed leftover from internal debugging. We've removed those, along with some leftover artifacts from the US_Autocomplete classes, in the most recent version (5.0.1).

shengslogar commented 5 months ago

Excellent! Happy to see this package getting some love 🙌