all*() methods and ::isIterable() assertion

[jmfeurprier]

Since version 2, methods like allString(iterable $value, string $message = ''): iterable are more strongly typed (param and return types), which is great.

I do understand and approve why $message is now typed, but typing the input $value introduces a new behavior: $value now has to really be of type iterable before calling the assertion method (PHPStan reports the issue, and PHP could fail at runtime if $value is not an iterable type).

This requires adding Assert::isIterable(...) before every code using all*() assertion method.

What surprises me too is the first line of this same method:

    public static function allString(iterable $value, string $message = ''): iterable
    {
        static::isIterable($value); // Here

        foreach ($value as $entry) {
            static::string($entry, $message);
        }

        return $value;
    }

In that case, the assertion is redundant and should always evaluate to true.

Shouldn't $value remain of type mixed (implicitly) like in version 1?

Also, methods allNullOr...() seem to have the wrong type defined:

public static function allNullOrInteger(?iterable $value, string $message = ''): ?iterable { ... }

Which means, for $value. that it is either "an iterable variable OR a null variable", instead of the expected "an iterable variable filled with a mix of integer or null values".

Thanks for the great tool!

[jmfeurprier]

Got some time for a PR : #338

[bnowak]

I spotted this today as well and 100% agree with you @jmfeurprier.

Added input types make all* methods inconsistent in fact as instead of validating expected type in runtime (static::isIterable($value); in their body) they'll fail on input typing (which assert methods suppose to validate).

hope your PR will be merged and released soon :) great job 💪

[jmfeurprier]

Quoting @shadowhand in PR #338 (comment) :

This looks good, but I need to think about it a bit. Technically, this would be a BC break in signature, which would require a version 3.0. But if we're going to do a new major, then I would rather be removing the inner static::isIterable($value) check instead.

There is indeed a method signature change involved here, though the changes should have no breaking impact for users:

• converting iterable $value into mixed $value widens type and allows more types to be passed to the assert methods (this is somewhat the LSP principle at work here).
• converting ?iterable $value into mixed $value also makes $value more permissive.
• converting return value ?iterable into iterable "narrows" returned types (this looks like covariance). Also, a null value can currently never be returned.

Removing the static::isIterable($value) in assert methods, would still require developers to prepend every Assert::all*(...) methods with an explicit call to Assert::isIterable(...), which means developers can have to alter their codebases to upgrade from v1 to v2. This is avoidable.

The more I look into the actual v2, the more I think the introduced types for iterable assert methods is either a mistake or an unforeseen consequence, which makes it a bug.

If you go the "this is a bug" way, then instead of releasing a 3.0 version, you could just publish patch versions (2.0.1 and 2.1.2 as of today), in full respect of semantic versioning (no Breaking Change).

[shadowhand]

Closed by #338, released in 2.1.2. There will not be a 2.0.1 release.