PHPORM-49 Implement Query\Builder::whereNot by encapsulating into $not
#13
Conversation
GromNaN
changed the title
Query\Builder::whereNot by encapsulating into $not
GromNaN
force-pushed
the
PHPORM-49
branch
2 times, most recently
from
9ee1e65 to
bf569c5
Compare
| && str_starts_with($where['boolean'], 'and') | ||
| && str_starts_with($wheres[$i + 1]['boolean'], 'or') | ||
| ) { | ||
| $where['boolean'] = 'or'.(str_ends_with($where['boolean'], 'not') ? ' not' : ''); |
There was a problem hiding this comment.
$where['boolean'] can be and, or, and not or or not. It is used as-this into SQL queries. I tried several implementations to split into vars that are easier to use later, but using str_ functions is the simplest.
jmikola
left a comment
jmikola
left a comment
There was a problem hiding this comment.
Some questions and suggestions, but the tests look correct.
Even thought it's a bug fix, this change looks like it warrants special documentation to keep users informed. Should we start a Markdown file in the project (perhaps an UPGRADING-X.Y.md) to demonstrate how query builder methods are changing? That could just show what a method previously built and what it would now.
tests/Query/BuilderTest.php
Outdated
| [], // options | ||
| ]], | ||
| fn (Builder $builder) => $builder | ||
| ->whereNot([['foo', 1], ['bar', '<', 2]]), |
There was a problem hiding this comment.
Should both elements be a three-element array with an operator, or was it intentional to only do so for the second?
There was a problem hiding this comment.
That's intentional, arrays are arguments to call Query\Builder::where(...$args).
https://github.com/GromNaN/laravel-mongodb-private/blob/cf103ba0ddc8a4ca601aab338e8298e0fdbff018/src/Query/Builder.php#L943
Laravel Eloquent has this feature.
tests/Query/BuilderTest.php
Outdated
| fn (Builder $builder) => $builder->where(['foo' => 1, 'bar' => 2]), | ||
| ]; | ||
|
|
||
| yield 'nested orWhere and where' => [ |
There was a problem hiding this comment.
"nested orWhere and where" seems a bit simplistic given the complexity of this query.
Is it relevant that you're testing orWhere with a callable? Is the combination of where and whereNot relevant? I'm not sure how this compares to the where orWhereNot (per my suggested name below) test later in the file.
There was a problem hiding this comment.
I removed this test and added whereNot orWhere to ensure the not it kept when the 1st condition get the $or logical operator from the 2nd.
jmikola
left a comment
jmikola
left a comment
There was a problem hiding this comment.
Some comments to follow-up on with a separate ticket, but changes here LGTM.
| ->whereNot('name', 'foo') | ||
| ->whereNot('name', '<>', 'bar'), |
There was a problem hiding this comment.
This is likely beyond the scope of this PR, but there's room for improvement here. Rather than unconditionally wrap the query with $not, we could just use a more appropriate operator. For instance:
where()assumes an equality match, sowhereNot()could use$neinstead of a direct value match (as we have here) or$eq.whereNot()with a<>operator could utilize$eqor a direct value match instead.
If you agree, perhaps we can track this in a new PHPORM ticket. I wouldn't limit the query optimization to $not. It should probably be a much larger task to scope out for the entire query builder.
| ]], | ||
| [], // options | ||
| ]], | ||
| fn (Builder $builder) => $builder->where(['foo' => 1, 'bar' => 2]), |
There was a problem hiding this comment.
Beyond the scope of this PR I'm sure, but I don't understand why this results in an $and with separate conditions instead of ['find' => ['foo' => 1, 'bar' => 2]]. Something else to address in query builder optimization follow-up work I suppose.
| [], // options | ||
| ]], | ||
| fn (Builder $builder) => $builder | ||
| ->whereNot(['foo' => 1, 'bar' => 2]), |
There was a problem hiding this comment.
This also relates to my earlier comment about query optimization.
…$not` (#13) `Query\Builder::whereNot` was simply ignoring the "not" and breaking the built query.
…$not` (#13) `Query\Builder::whereNot` was simply ignoring the "not" and breaking the built query.
…$not` (#13) `Query\Builder::whereNot` was simply ignoring the "not" and breaking the built query.
Fix PHPORM-49
Query\Builder::whereNotwas simply ignoring the "not" and breaking the built query.