PHPORM-47 Improve Builder::whereBetween to support CarbonPeriod and reject invalid array

[GromNaN]

Fix PHPORM-47

The Query\Builder::whereBetween() method can be used like this:

whereBetween('date_field', [min, max])
whereBetween('date_field', collect([min, max]))
whereBetween('date_field', CarbonPeriod)

Laravel allows other formats: the $values array is flatten and the builder assumes there are at least 2 elements and ignore the others. It's a design that can lead to misunderstandings. I prefer to raise an exception when we have incorrect values, rather than trying to guess what the developer would like to do.

Support for CarbonPeriod was fixed in Laravel 10: laravel/framework#46720 because the query builder was taking the 1st 2 values of the iterator instead of the start & end dates.

[alcaeus]

Changes LGTM. Just a couple of suggestions around exceptions and formatting for complex expectations.

[alcaeus]

The first message does a good job at explaining the expected format, but the second one is less clear about it. I suggest combining these two cases into a single conditional:

Suggested change

	if (! array_is_list($values)) {
	throw new \InvalidArgumentException('Between $values must be a list with exactly two elements: [min, max]');
	}
	if (count($values) !== 2) {
	throw new \InvalidArgumentException('Between $values must have exactly two elements: [min, max]');
	}
	if (! array_is_list($values) || count($values) !== 2) {
	throw new \InvalidArgumentException('Between $values must be a list with exactly two elements: [min, max]');
	}

[GromNaN]

I think it's a good place to use assert, because this check should be skipped for production.

        assert(
            !is_array($values) || array_is_list($values) && count($values) === 2,
           new \InvalidArgumentException('Between $values must be a list with exactly two elements: [min, max]')
        );

The exception will be throw exactly in the same way when assert is enabled (by default).

[alcaeus]

While assert definitely is a good call for such code, I'm not sure Laravel users have this configured appropriately for development. Is this pattern used in other places in Laravel?

[GromNaN]

I've not seen it used in Laravel codebase.

[jmikola]

Question about adding additional unit tests, but I expect those may not be necessary.

LGTM.

[jmikola]

Could a list shape be used here, or is this package not yet requiring Psalm 5?

[jmikola]

Looking at this again, I don't understand why this flips the query into separate $lte and $gte ranges instead of wrapping whereBetween() with $not, but that's beyond the scope of this PR.

[jmikola]

It might make sense to move this below all of the whereBetween yield statements to keep things grouped by the method being tested.

Also, this appears to be the only whereNotBetween test. For orWhereNotBetween you also test "nested array of numbers" and "collection" (no "CarbonPeriod"). Should there be additional tests, or would those be redundant? It looks like the implementation of the "NotBetween" methods is outside of this package.