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

[8.x] Add where helpers for boolean and where not #38437

Closed
wants to merge 1 commit into from

Conversation

taylorotwell
Copy link
Member

This PR adds a few small convenience methods to the query builder courtesy of inspiration by @simensen...

First, passing only a column name to the query builder will now add a where clause ensuring that the value of the column is true:

User::where('subscribed')->get();

// Equivalent...
User::where('subscribed', true)->get();

In addition, whereNot and orWhereNot methods have been added. Typically, this generate traditional <> clauses:

User::whereNot('name', 'Taylor')->get();

// Equivalent...
User::where('name', '<>', 'Taylor')->get();

In addition, passing only a column name to this method will add a "where false" clause to the query:

User::whereNot('subscribed')->get();

// Equivalent...
User::where('subscribed', false)->get();

Let me know what you think!

@mrpritchett
Copy link

Dumb question, will it work with any "evaluates as true" value? i.e. true, 1, "true" etc

@mrpritchett
Copy link

Hit enter too soon. I love it, regardless of the answer!

@taylorotwell
Copy link
Member Author

@mrpritchett that may depend on the database system? I know on MySQL I can do where foo = true and it will return the rows with a tiny integer column set to 1 just fine...

@pablius
Copy link

pablius commented Aug 18, 2021

How would it work in cases like this?
User::where('email_verified_at')->get()

I'd expect the result to be the same as:
User::where('email_verified_at','!=',null)->get()

@taylorotwell
Copy link
Member Author

@pablius - it doesn't work that way. Again, this is just for boolean comparisons. We already have a whereNotNull method for your situation.

@dennisprudlo
Copy link
Contributor

Looking at the code it would just translate into ->where('email_verified_at', true)

For your scenario there is always ->whereNotNull ('email_verified_at')->get()

@GrahamCampbell GrahamCampbell changed the title Add where helpers for boolean and where not [8.x] Add where helpers for boolean and where not Aug 18, 2021
@chasenyc
Copy link
Contributor

chasenyc commented Aug 18, 2021

I think its a great addition, my only concern is whether it should be whereNotEquals or something similar. I associate the api of where to include a second parameter that allows for an operator and I guess my intuition would be that whereNot would work similarly. The first place my head with this was using an operator other that equals. i.e.:

User::whereNot('points', '>', 10)->get();

// Equivalent... 
User::where('points', '<=', 10)->get();

or something along these lines. I don't feel super strongly about this but thought it was worth consideration.

Also a little curious if whereNot works as expected if the first parameter is not a string but an array. I'm not too familiar with using that pattern but I see it as a path that can occur, it looks as if the operator (<>) would be ignored in that case.

Would a non-string column work:

->whereNot([['foo', 1], ['bar', 2]]);

@devcircus
Copy link
Contributor

devcircus commented Aug 18, 2021 via email

@taylorotwell
Copy link
Member Author

If we want to be super explicit we could have whereTrue and whereFalse... 🤷‍♂️

@DarkGhostHunter
Copy link
Contributor

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for lol jk.

Seems fine to me, because they're lexically correct. Usually these can be chained against boolean columns, which by convention start with is_.

User::query()->where('is_admin')->first();

The thing is, should be this equivalent to adding the column name to a where clause?

User::query()->whereIsAdmin()->first();

@gocanto
Copy link
Contributor

gocanto commented Aug 19, 2021

If we want to be super explicit we could have whereTrue and whereFalse...

That would be more descriptive.

These kinds of helpers become really confusing at scale projects (1k+ models/files) and being descriptive is always better when debugging.

✌🏼

@sebastianvitterso
Copy link

sebastianvitterso commented Aug 19, 2021

I love the concept, but I'm not completely sold on the implementation.
I see two problems with it:

Problem 1:
whereNot currently doesn't negate nested queries (as mentioned by @cfroystad).
Example: whereNot( fn($query) => $query->where('is_cool') ) doesn't negate the inner query.

Problem 2:
Not possible to define which operator should be negated.
Example: Person::whereNot('age', '>', 72);

These would both be fixed by instead of using the <> operator, "simply" adding the NOT keyword to whatever query is input. Meaning Person::whereNot('age', '>', 72)->toSql(); should evaluate to SELECT * FROM persons WHERE NOT age > 72;,

A lot of programmers have some (or a lot of) knowledge of SQL, and seeing a whereNot-function, would at least make me think it evaluates to WHERE NOT and not WHERE column <> value.

@theVannu
Copy link

If we want to be super explicit we could have whereTrue and whereFalse... 🤷‍♂️

Love it, in my opinion these methods are more clean

@mrbenosborne
Copy link
Contributor

If we want to be super explicit we could have whereTrue and whereFalse... 🤷‍♂️

Love this too, I always go to type whereTrue or whereFalse and realise it doesn't exist!

A great addition.

@rakibdevs
Copy link

If we want to be super explicit we could have whereTrue and whereFalse... 🤷‍♂️

This could be a great addition! also a better understanding method for all!

@taylorotwell
Copy link
Member Author

@sebastiaanluca IMO that is just reading too much into the method. It is simply a shortcut that allows you to not have to specify the <> operator - nothing more.

@sebastiaanluca
Copy link
Contributor

@sebastiaanluca IMO that is just reading too much into the method. It is simply a shortcut that allows you to not have to specify the <> operator - nothing more.

I'm flattered by the mention, but you got the wrong Sebastiaan 😄 @sebastianvitterso

@taylorotwell
Copy link
Member Author

Too much confusion over this.

@taylorotwell taylorotwell deleted the where-helpers branch August 19, 2021 19:39
@shadoWalker89
Copy link
Contributor

IMHO whereNot and orWhereNot should be in the framework, cuz every other where has a not variant, on top of that it's a valid sql syntax
I made a PR #36522 not so long ago and it was rejected, also I implemented it differently from you and I think the way I did is better cuz I'm using NOT keyword instead of <>
I'm personally using my rejected pr as macros but I think this should be in the framework

@sebastianvitterso
Copy link

sebastianvitterso commented Aug 20, 2021

I realize you closed the issue, but I believe @shadoWalker89 has a good solution, and that his PR should be reconsidered.

IMO that is just reading too much into the method. It is simply a shortcut that allows you to not have to specify the <> operator - nothing more.

I disagree. When you write whereNotIn, Laravel currently writes WHERE <column> NOT IN (<values>), which makes it nice and familiar to users.
When I write whereNot, I'd expect Laravel to do the same thing: Use the NOT-keyword.

I acknowledge that with the implementation in this PR, the whereNot-method is a shorthand for using <>, but it would be really nice to have a whereNot that simply added a NOT ( $subQuery ) to the generated sql-query, because sometimes it makes more sense to say NOT( predicate_a OR predicate_b ) than saying NOT (predicate_a) AND NOT (predicate_b).

@simensen
Copy link
Contributor

Oh, sad! I didn't realize this wasn't merged and just now saw all the discussion around it. Shame, but I get it. :)

If we want to be super explicit we could have whereTrue and whereFalse... 🤷‍♂️ – @taylorotwell

I think this was my initial suggestion so I'd have dug this. I do like keeping things super explicit, especially if it turns out to be too confusing. If you'd consider a more explicit implementation, I may be up for trying to reproduce it myself sometime.

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.