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

[6.x] Add existsOr and doesntExistOr to the querybuilder #30495

Merged
merged 3 commits into from
Nov 5, 2019
Merged

[6.x] Add existsOr and doesntExistOr to the querybuilder #30495

merged 3 commits into from
Nov 5, 2019

Conversation

SjorsO
Copy link
Contributor

@SjorsO SjorsO commented Nov 1, 2019

Resubmit of #30479

In my controllers, I often use something like this to validate requests:

$hasOpenDossier = $user->dossiers()
    ->whereNull('closed_at')
    ->exists();

if ($hasOpenDossier) {
    abort(422, 'User already has an open dossier');
}

With this PR, I can do this instead:

$user->dossiers()
    ->whereNull('closed_at')
    ->doesntExistOr(function () {
        abort(422, 'User already has an open dossier');
    });

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 the exists() and doesntExist() methods. I think adding two new methods is a cleaner way of doing it, mainly because it keeps the parameters and return type of exists() and doesntExist() simple.

@driesvints
Copy link
Member

@SjorsO can you rebase with the 6.x branch? StyleCI updated their default checks today. Rebasing should make the checks pass again.

@chuckrincon
Copy link

@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();

@SjorsO
Copy link
Contributor Author

SjorsO commented Nov 2, 2019

@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();

?? will only call the callback if exists is null or not set, which is never the case.

I think you're thinking about ?:, which would be slightly shorter than the current code. But i prefer not being fancy and just using a simple shorthand if statement.

@netpok
Copy link
Contributor

netpok commented Nov 5, 2019

I'm not sure about this,
one can already use the:

abort_if(
    $user->dossiers()->whereNull('closed_at')->exists(),
    422, 'User already has an open dossier'
);

method which is way more expressive.

The findOr method is used to provide a default when the record not exists, but I don't see a use-case when one want to do that with an exists() to use the return value and the …->exists() ?: randomStatement is not enough.


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

@SjorsO
Copy link
Contributor Author

SjorsO commented Nov 5, 2019

Queries often don't fit neatly on a single line, in that case you can't use abort_if like that

@netpok
Copy link
Contributor

netpok commented Nov 5, 2019

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
Copy link
Contributor

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)
Copy link
Contributor

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();
Copy link
Contributor

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.

@taylorotwell taylorotwell merged commit 3f60972 into laravel:6.x Nov 5, 2019
@s51983
Copy link

s51983 commented Nov 30, 2019

When Model use SoftDeletes, existsOr / doesntExistOr didn't check deleted_at columns

 User::where('id', $id)->exists();

SQL: select exists(select * from `users` where `id` = ? and `users`.`deleted_at` is null) as `exists`
 User::where('id', $id)->existsOr(function(){
    abort(404,'User not exists!')
});

SQL: select exists(select * from `users` where `id` = ?) as `exists`

How to fix it?

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.

None yet

6 participants