Add missing type declarations and fix incorrect PHPDoc types

[theodorejb]

This avoids the need for manual validation in several places, and makes it possible to use the library in codebases with static analysis, without having to suppress errors.

Also upgraded to PHPUnit 9, and added missing ext-json requirement.

[theodorejb]

@AaronAtDuo Can you review this please?

[mbish]

Thanks for adding these type annotations and fixing some typing inconsistencies. I don't think adding type information for static analysis tools removes the need for runtime type validation during the Client constructor. Since this information will likely be originating from some external data source (config file, database, etc.) enforcing that the data is in the shape we expect allows for better error reporting. PHP type checking is not enforced at runtime and therefore

Similarly exchangeAuthorizationCodeFor2FAResult the data that's being validated originates from an API request and should be validated before use.

It's harder to make an argument for createAuthUrl but in general I prefer the verbose, runtime checking in the handful of places that we have it.

If you could add the removed validation code and associated tests back in this PR looks good to me.

Thanks again for adding the typing information to this library. It greatly benefits from the consistency and clarity that automatically checked annotations can provide.

[theodorejb]

PHP type checking is not enforced at runtime

@mbish You are mistaken. PHP type declarations are enforced at runtime, and will throw a TypeError if a value incompatible with the type is passed (example). When the strict_types directive is enabled (as it is in ClientTest.php), any invalid/non-matching type will throw a TypeError at runtime.

I could add back tests which demonstrate this, but it feels somewhat superfluous to just be testing PHP's built-in validation of the declared types. Let me know if you still want such tests to be added.

[mbish]

I see, strict typing is optionally enforced. Since responsibility for enforcing type checking is opt-in and placed on the user of the library (i.e there's nothing we can do to ensure that the annotations in this library are respected) I'd prefer if the validation code was left in. We want to provide good defaults for users.

You raise a good point about the test code with the strict_types directive and the newly added annotations the tests aren't actually exercising the code they were written to exercise. Turning off that directive for clientTest.php makes sense in order to actually exercise the validation code. Moving the validation testing code into its own test file with strict_types disabled and keeping strict typing in clientTest.php would be even better but is a larger refactor.

[theodorejb]

I see, strict typing is optionally enforced. Since responsibility for enforcing type checking is opt-in and placed on the user of the library (i.e there's nothing we can do to ensure that the annotations in this library are respected)

@mbish While it's true that strict type checks are optional, type checking itself is not opt-in or a burden placed on the user. Even without strict type checks enabled, PHP enforces that functions receive the declared types, and will throw a TypeError if an incompatible value is passed. The only difference is that without strict_types enabled, PHP is more forgiving in that it will coerce compatible values to the declared type. For example:

function takesInt(int $val) {
    var_dump($val);
}

takesInt("5");   // int(5)
takesInt("foo"); // TypeError: takesInt(): Argument #1 ($val) must be of type int, string given
takesInt("");    // TypeError: takesInt(): Argument #1 ($val) must be of type int, string given
takesInt(null);  // TypeError: takesInt(): Argument #1 ($val) must be of type int, null given

So even without strict types enabled, passing null or any other value incompatible with the declared type will throw a TypeError at runtime.

I'd prefer if the validation code was left in. We want to provide good defaults for users.

When native type declarations are in place, the validation code doesn't do anything, since PHP ensures that the arguments have the declared type inside the function. The only way to have that validation code do anything is to remove native type declarations, but I would strongly disagree that this is a good default. It may prevent users from being warned by their IDE that they are passing an incorrect type to the function, leading to runtime errors which could otherwise have been avoided.

Furthermore, if a library doesn't respect a developer's choice of strict or loose type checking, they may have to add extra type casts to function calls, which ironically can reduce type safety compared to native type checks. Here's the same example as above, but with manual validation instead of a native type declaration, which caused the developer to manually cast input values:

function takesInt($val) {
    if (!is_int($val)) {
        throw new Exception("Function must be passed an integer");
    }

    var_dump($val);
}

takesInt((int) "5");   // int(5)
takesInt((int) "foo"); // int(0)
takesInt((int) "");    // int(0)
takesInt((int) null);  // int(0)

For these reasons among others, it is a best practice to use native type declarations rather than trying to replace them with non-standard manual type checks.

[theodorejb]

@AaronAtDuo I hope this can be merged and a new release tagged, so that I (and others) can enable static analysis on code using the library.

[AaronAtDuo]

Yes, sorry, there's been a few distractions recently. This is on my to-do list to get merged, hopefully in a day or two.

[AaronAtDuo]

@theodorejb For complicated reasons, we don't merge PRs directly in this repo yet. I have 'merged' the PR by direct push, though. I'll get the release bumped shortly. Thanks for the PR, the cleanup, and thanks for using Duo.

[AaronAtDuo]

And looks like's it's live on https://packagist.org/packages/duosecurity/duo_universal_php. Enjoy.

[theodorejb]

@AaronAtDuo FYI the Packagist package currently isn't auto-updated (I manually clicked the "Update Now" button there after you pushed the tag to GitHub to make the new release show up).

[AaronAtDuo]

Ah that explains it. We have a spirited internal debate on whether we want auto-updating or not. Thanks.