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

fix: MyLogger should utilize the PHP error log #33

Closed rkaiser0324 closed 2 years ago

rkaiser0324 commented 2 years ago

Instead of printing status messages (e.g., "retrying in X seconds") to stdout. Using error_log() will prevent it from interfering with the output.

SunSparc commented 2 years ago

@rkaiser0324, thank you for taking an interest in our PHP SDK. Your pull request represents a valid customization that you could choose to use when implementing your custom application. However, I do not see this as a necessary update to our SDK. We have created our SDKs as working examples of how people can interact with our APIs in a general way. Making specific customizations is outside the scope and purpose of the SDKs.

rkaiser0324 commented 2 years ago

@SunSparc - I get your point. However, the ClientBuilder class is an core part of the SDK and you would not expect people to be customizing it (at least, that's the way it looks based on the included examples). And since it references the MyLogger class directly, then MyLogger should at least adhere to standard conventions, meaning no log output to stdout, or at least provide a way to suppress it (e.g., via a log level passed in the ClientBuilder constructor etc.). As it stands, it outputs messages to stdout and the only way to prevent that is to do something hacky externally like capturing the output buffer.

So I would submit that this change to using the native error_log() is actually important to making the examples "work".

SunSparc commented 2 years ago

@rkaiser0324, excellent counter-point. After consider it a bit more I am inclined to agree with you. I appreciate your commitment to helping us more closely adhere to standard conventions.

DuncanBeutler commented 2 years ago

Your changes have been merged and published on version 4.16.16. Thanks for your contribution!