PHPORM-47 Improve Builder::whereBetween to support CarbonPeriod and reject invalid array #10
Conversation
src/Query/Builder.php
Outdated
| 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]'); | ||
| } |
There was a problem hiding this comment.
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:
| 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]'); | |
| } |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I've not seen it used in Laravel codebase.
e0dd130 to
63ef1fa
Compare
…eject invalid array
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
src/Query/Builder.php
Outdated
| @@ -554,11 +555,20 @@ public function whereAll($column, array $values, $boolean = 'and', $not = false) | |||
|
|
|||
| /** | |||
| * @inheritdoc | |||
| * @param array{mixed, mixed}|CarbonPeriod $values | |||
There was a problem hiding this comment.
Could a list shape be used here, or is this package not yet requiring Psalm 5?
tests/Query/BuilderTest.php
Outdated
| ]], | ||
| [], // options | ||
| ]], | ||
| fn (Builder $builder) => $builder->whereNotBetween('id', [1, 2]), |
There was a problem hiding this comment.
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.
tests/Query/BuilderTest.php
Outdated
| fn (Builder $builder) => $builder->whereBetween('id', [[1], [2, 3]]), | ||
| ]; | ||
|
|
||
| yield 'whereNotBetween array of numbers' => [ |
There was a problem hiding this comment.
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.
…eject invalid array (#10) 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.
Fix PHPORM-47
The
Query\Builder::whereBetween()method can be used like this:Laravel allows other formats: the
$valuesarray 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.