Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[9.x] Add whereNot() to Query Builder and Eloquent Builder #41096

Merged
merged 5 commits into from
Feb 24, 2022
Merged

[9.x] Add whereNot() to Query Builder and Eloquent Builder #41096

merged 5 commits into from
Feb 24, 2022

Conversation

marcovo
Copy link
Contributor

@marcovo marcovo commented Feb 18, 2022

Summary
From time to time I encounter the situation I want to negate a relatively complicated query builder expression, often hidden away in a (somewhat large) scope. One could start writing the negated version of that scope, or just use ->whereNot(fn ($q) => $q->someScope()), except that laravel does not currently provide whereNot() functionality. Until now each time I encountered this I have added this method by defining macros, but I think it is well worth to add this function to laravel itself.

This pull request proposes to add whereNot() to both the Query Builder and Eloquent Builder. Using this new method, one can now easily add a negated (nested) condition to the query builder, or e.g. define a negated scope in terms of a positive scope:

public function scopeIsNotFoo($query)
{
    $query->whereNot(fn ($q) => $q->isFoo());
}

Added functionality
This pull request adds 4 methods:

  • whereNot() to the Query Builder
  • orWhereNot() to the Query Builder
  • whereNot() to the Eloquent Builder
  • orWhereNot() to the Eloquent Builder

I decided to only include support for passing closures as parameters, because whereNot('column', '=', 'foo') to me seems inferior to just using where('column', '!=', 'foo')

As far as I can tell, all necessary tests are included.

Backwards compatibility
One minor incompatibility may be introduced by this pull request, which would be if a developer has named a database column not and uses whereNot(...) to add conditions based on this column. I think it is unlikely someone will name a column not as it is not descriptive, so I suppose this incompatibility can be accepted.

Considerations
This is my first pull request towards Laravel, I hope everything is alright. A few points of attention:

  1. The two added tests in DatabaseEloquentBuilderTest.php are practically copies of testNestedWhere() in that same file, with minor modifications in two places. Should these be generalized using a data provider?
  2. I think I have covered all tests that should be added. Can anyone check?

Thanks!

@marcovo marcovo changed the title Add whereNot() to Query Builder and Eloquent Builder [9.x] Add whereNot() to Query Builder and Eloquent Builder Feb 18, 2022
@shadoWalker89
Copy link
Contributor

I attempted same thing before and was rejected for no reason #36522
I don't understand how something like this gets rejected, hope yours get merged.

@marcovo
Copy link
Contributor Author

marcovo commented Feb 23, 2022 via email

@pascalbaljet
Copy link
Contributor

I thought it was strange no-one had come up with this before, good to see it turns out someone had.

I also needed this, so I created a package, but I'd be glad if this got merged into the framework.

@taylorotwell taylorotwell merged commit 0f8624f into laravel:9.x Feb 24, 2022
@shadoWalker89
Copy link
Contributor

@marcovo First of all i'm happy that yours got merged.

You said my PR focused on doing a whereNot on a value but yours on a callback.
But no my PR also supports callback the same way a where() supports a callback.
In laravel you can do this.

$query->where(function($query){
    $query->...
})

Since my PR was using the existing where() but just modifies the $boolean, it inherits all the functionality of where()
Actually i have been using the code from my PR as a macro since it got rejected :D

@marcovo
Copy link
Contributor Author

marcovo commented Feb 28, 2022

@shadoWalker89 Yeah did read through your code when I wrote my reply, and saw your implementation follows the same lines as mine, albeit my implementation is a limited version of what your version provided.

I did not mean to say your code did not support passing closures. What I meant is you did not mention (=focus on) the closure use-case in your initial post. I think that in a pull request, the description is at least as important as the actual code, in order to properly communicate the intentions of the PR. Therefore, I suspect if you had mentioned in your first post the details on the closure usage you did mention in your last post there, your PR may have had a better chance of being merged.

In order words, I suspect (but this is purely speculation from my side) the maintainers may often make decisions based on the description alone without looking at the committed code. Therefore it is important the description covers your intents. I suspect only after they decide the described new functionality is useful, they may go and look at the implementing code. (But then again, this is pure speculation.)

@jasonmccreary
Copy link
Contributor

I wish this included support for passing a column/value pair… I always type whereNot('status', 5) before remembering that's not supported and I have write where('status', '!=', 5). Now I feel it'll be even more confusing that a whereNot method exists.

@marcovo
Copy link
Contributor Author

marcovo commented Mar 1, 2022

You're always free to propose a pull request of course. Beware though, a naive implementation would amount to a query like WHERE NOT a = b, or more general WHERE NOT a {operator} b. While it should be possible to add this, I think precedence rules should be checked to make sure for any {operator} the meaning is equivalent to WHERE NOT (a {operator} b), not WHERE (NOT a) {operator} b. This may or may not be trivial.

Note that within the scope of this pull request precedence is not an issue because closures always add parentheses.

@jasonmccreary
Copy link
Contributor

jasonmccreary commented Mar 1, 2022

@marcovo, I understand. I may try to take a look in the coming weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants