API suggestions

[rtek]

This is a random list of suggestions I see with the API when reviewing it for use:

Inject the guzzle client via setter I am unable to provide a guzzle client instance. I see the wiki about TLS verification but the problem would be better solved by allowing a custom guzzle instance. I would like to provide my own CA bundle, for example:

$client = new Client([
    'verify' => '/path/to/my/cafile.pem',
    'base_uri' => PostmarkClientBase::HTTPS_ENDPOINT,
    'timeout' => 60,
    //etc
]);
$postmark->setClient($client);

If no client is provided write a getter to return a default instance.

SendEmail has too many arguments

Create a PostmarkEmail class as the sendEmail argument:

class PostmarkEmail
{
    public function __construct($from, $to, $subject) { ... }
    public function setHtmlBody($html) { ... }
    //...
}

Docblock typo: DyanamicResposeModel and Annotate DynamicResponseModel with @method to show all available keys The return param on PostmarkClient public methods should be Dynamic instead of Dyanamic. IDEs fail to resolve the correct class when providing autocomplete. Then add annotations for methods for further autocomplete.

/**
 *  @method string to()
 *  @method int errorCode()
 *  ...
 */

General style

• Many instances of equality testing with null ($var != null) recommend using identity operators and explicit casting
• Many instances of missing method visibility
• Remove the trailing PHP tag for all classes as its not needed. Some classes already exclude it.

[atheken]

Hi @rtek,

Thanks for taking the time to review the code. I appreciate and agree with your suggestions.

Related to the Guzzle verification: It is actually possible for you to pass the certificate directly using the $VERIFY_SSL property, but when I wrote the wiki document, I didn't describe this option, which is probably better than Option 1 for most users Use of $VERIFY_SSL

I also agree that the email methods are getting unwieldy. My initial thoughts when writing these methods was that consistency of simply passing scalars instead of a model was better than having some methods accept a list of parameters, and others accepting models. The email methods also don't require most of those params, so I thought that having some defaults on the tail made sense. However, it's certainly the case that if you need to specify the last param, but not some of the intervening ones, this is a pain! I think the email methods have reached a tipping point where adding an overload for a "model" makes sense.

With respect to the style aspect. PHP is not my "first language", so when I authored this, I tried to follow reasonable coding styles where possible, your recommendations all seem reasonable, and are probably more idiomatic than what I wrote, all of the recommendations seem safe and appropriate.

To summarize, I agree with your position on all of these items. It may take me some time to get to each of them. I'd also be happy to accept pull requests on them if you have time and interest in contributing.

Thanks again for the review!

[rtek]

Great. I submitted one for the Guzzle client but the CI failed because of the key settings. Could you recommend what I should do there? Locally I fudged the a few of the keys to be POSTMARK_API_TEST to get some tests working, but most required live keys.

As for the styling, may I recommend PSR-2? It's widely followed (Guzzle uses it, for example) and is pretty close to what's already in place.

[krukru]

Hi guys, any progress on this? Having 10+ params in the sendEmail method is really impractical. Perhaps it's time to refactor. Would you accept a PR for this issue? Since it changes the API in a way that brakes backward-compatibility, this should be a new major release.