Use constants for directories and add smartykey. #67
Conversation
There was a problem hiding this comment.
Made two comments.
Basically it seems fine except I'm confused by the "Backwards compatible automatic appending of api path" commit. I'm not sure what its trying to accomplish and why the endpoints aren't encapsulated inside the Request.
src/Request.php
Outdated
| @@ -97,7 +97,14 @@ public function setReferer($referer) { | |||
| $this->referer = $referer; | |||
| } | |||
|
|
|||
| public function setUrlPrefix($urlPrefix) { | |||
| public function setUrlPrefix($urlPrefix, $apiPath) { | |||
There was a problem hiding this comment.
Try using a default parameter to keep this function backwards compatible, particularly since its public.
e.g. public function setUrlPrefix($urlPrefix, $apiPath = "")
src/Sender.php
Outdated
| @@ -3,5 +3,5 @@ | |||
| namespace SmartyStreets\PhpSdk; | |||
|
|
|||
| interface Sender { | |||
| function send(Request $request); | |||
| function send(Request $request, $apiPath); | |||
There was a problem hiding this comment.
I do not understand the need for an $apiPath parameter. It seems to me whatever endpoint is used should already be encapsulated inside the Request object when the request is built by the product-specific class. This feels like a leaky abstraction, thus increasing complexity for everything that uses Request.
Even if $apiPath remains, we may want to make it an optional parameter to avoid breaking consumers of the methods/interface since they are public--unless we really, really mean to make $apiPath required which it doesn't appear like since there are many defaults of '' in the PR.
Just as a tangent, you may want to consider creating a constant for the endpoint paths, like/verify and /lookup, etc (ie, what is being used as the $apiPath). Since the endpoint is specific to the product API, using a class constant may be appropriate. That said, in this PR it looked like each endpoint is only referenced once so I'd call it an optional change.
There was a problem hiding this comment.
The $apiPath was added to match the functionality of our other sdks. When providing a custom url the PHP sdk requires the endpoint whereas the other sdks only require the base url. So the backward compatible part allows the custom url to include the endpoint or not. The $urlPrefix isn't available to the buildRequest() product-specific function so I opted for the send() method and most of the calls with '' are tests anyway.
I agree updating the signature to be an optional parameter makes more sense. To your point though this isn't a necessary update so I will default to your judgement if you think it is more complicated/leaky than its worth.
Also added backward compatible automatic appending of api path.