-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[6.x] Add existsOr and doesntExistOr to the querybuilder #30495
Conversation
@SjorsO can you rebase with the 6.x branch? StyleCI updated their default checks today. Rebasing should make the checks pass again. |
@SjorsO It's possible to use the null coalescing operator (??), produces the same return // Before
return $this->exists() ? true : $callback();
// After
return $this->exists() ?? $callback(); |
I think you're thinking about |
I'm not sure about this, abort_if(
$user->dossiers()->whereNull('closed_at')->exists(),
422, 'User already has an open dossier'
); method which is way more expressive. The Little unrelated but I think one should not throw 422 errors manually if the controller already has validation it can be solved with a nice rule or an after callback |
Queries often don't fit neatly on a single line, in that case you can't use |
I would argue than it probably should be moved to a method or better to the FormRequest object, but that's not in the scope of this pull request. I consider these methods in the same category than the findBy methods (#30442). Also this works too: $user->dossiers()
->whereNull('closed_at')
->doesntExist() ?: abort(422, 'User already has an open dossier'); |
/** | ||
* Execute the given callback if no rows exist for the current query. | ||
* | ||
* @param \Closure $callback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to typehint Closure|bool
* @param \Closure $callback | ||
* @return mixed | ||
*/ | ||
public function existsOr(Closure $callback) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allow all types here (should only be closure and bool, but there is some time till union types will be available).
*/ | ||
public function existsOr(Closure $callback) | ||
{ | ||
return $this->exists() ? true : $callback(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And return $this->exists() ?: value($callback)
I prefer ?:
over ? true :
but maybe that's just me.
When Model use SoftDeletes, existsOr / doesntExistOr didn't check deleted_at columns
How to fix it? |
Resubmit of #30479
In my controllers, I often use something like this to validate requests:
With this PR, I can do this instead:
This way I don't have to create a temporary variable, and the code is more logically grouped together (which is especially nice in more complex projects where you can have three of these checks in a row).
I would almost always use this feature for aborting. It could also be used for other things, but to be honest I can't really think of any. (maybe adding a
doesntExistOrAbort
macro to my projects would be a better solution).An alternative to adding two new methods is adding an optional
$callback
argument to theexists()
anddoesntExist()
methods. I think adding two new methods is a cleaner way of doing it, mainly because it keeps the parameters and return type ofexists()
anddoesntExist()
simple.