Fix some missing return/throw types.

[ZebulanStanphill]

I've been trying to fix all the type errors reported by PHPStan and Psalm, but it turns out there's a LOT, so to make things easier to review, I'm splitting the changes up into several PRs, starting with this one.

This PR adds some missing return/throw types, clarifies some existing ones, and moves some from PHPDocs to native type declarations. I've mostly just done the simple ones here. I'll do the more complex ones (basically everything involving $this->hydrateResponse) in a later PR.

[DavidGarciaCat]

LGTM - Thanks!

[DavidGarciaCat]

It seems unnecessary to me

[ZebulanStanphill]

This helps Psalm and PHPStan recognize that not only does this function throw, but it specifically always throws. So if you put any code after a call to this function, the static analyzers will know you've made a mistake, because the code would never be executed. PHP 8.1 is actually adding a native never return type for this reason.

[DavidGarciaCat]

I understand, but this SDK is based on PHP 7.3 or greater, which means we should not use code that has been specifically added (or will be added) in future versions.

[ZebulanStanphill]

Well, since it's just a comment annotation (rather than a native return type) it has no effect on the code's PHP 7.3 compatibility. But Psalm and PHPStan already support this annotation, so it will provide static analysis to everyone using those tools regardless of which PHP version they're using.

[DavidGarciaCat]

We have a BC failure here - can we please review this case so we can move forward with the update?

https://github.com/mailgun/mailgun-php/pull/801/checks?check_run_id=3883732923

[ZebulanStanphill]

Yeah, the addition of native return types to non-final, non-private class methods (which currently have no defined return type) is technically a breaking change. For example:

Hypothetically a 3rd-party class that extends, say, Mailgun\Api\HttpApi, could have an implementation of handleErrors() that returns a string for some reason, but that would no longer be allowed if the return type of Mailgun\Api\HttpApi#handleErrors() was changed to void, since the overriding implementation must have the same (or narrower, covariant) return type. (Side note: using @template and @return PHPDoc annotations, you can have well-defined contravariant return types, but at first glance, I don't think that's applicable to any of the cases addressed in this PR.)

In the case of a void-returning method like HttpApi#handleErrors(), I could see an argument for allowing child class implementations to return any arbitrary type. If that's desirable, it would make sense to explicitly declare a mixed return type to clearly indicate this API promise. For now, this would have to be done via @return PHPDoc annotations rather than native return types, since the native mixed type was added in PHP 8.

But in other cases like Mailgun\Api\Attachment#show(), which already were returning a specific kind of object (Psr\Http\Message\ResponseInterface in this case), I think it makes more sense to lock down the return type to enforce covariance.

What do you think? Here are the technically-breaking cases:

• The return type of Mailgun\Api\HttpApi#handleErrors() changed from no type to void
• The return type of Mailgun\Api\Attachment#show() changed from no type to Psr\Http\Message\ResponseInterface
• The return type of Mailgun\Api\EmailValidationV4#deleteBulkPreview() changed from no type to bool
• The return type of Mailgun\Message\Exceptions\MissingRequiredParameter::create() changed from no type to self

Some of the return type additions in this PR affect methods on final classes, so they can't cause any BC breakage. If you want, I can reduce this PR to those easy cases, and punt the more complex cases to a separate PR for further discussion.

[DavidGarciaCat]

The whole point of this PR is, in essence, to set new type-hints to each method we have in the classes. However, any powerful IDE should give you these details based on the PHP annotations too, meaning there's no need to rush implementing this change.

Yes, we can discuss that the type-hint will throw an error if there's a mismatch. Still, the SDK has PHPUnit tests to minimise human errors and provide the expected results.

Maybe we should wait before implementing this update until we implement other significant changes as part of a new major version.

@Nyholm, can I please leave this call to you - as I don't have the permissions to skip this analysis, but I cannot see either a reason for keep waiting or implementing it now.

CC @Pavlico we should follow up this case.