-
-
Notifications
You must be signed in to change notification settings - Fork 541
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
[5.x] Ensure no queries are run when entries field has no values #11184
base: 5.x
Are you sure you want to change the base?
[5.x] Ensure no queries are run when entries field has no values #11184
Conversation
Nice! There's also another place where this would come in handy, and I'd like to be able to handle it. I can't quite remember where though. I'll try to track it down. I'm pretty sure it's when you're within a query already somewhere and a certain clause would trigger no results, so avoiding an actual query would be good there. 🤔 It would be good to nail the pattern first before rolling it out to other places. |
Ah, it's here:
|
This reverts commit 1ab9585.
@jasonvarga I don't love it, but this gets around it being inside a where function clause. Though returning the empty builder isnt even needed, it could just not do the orWhere() entirely |
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.
Maybe a bit of a pain, but I think we'll need multiple empty builder classes.
e.g. an EmptyEntryQueryBuilder
that implements Statamic\Contracts\Entries\EntryQueryBuilder
. One for assets, terms, etc.
Otherwise, someone might be expecting or typehinting EntryQueryBuilder
and would now fail if they're given an EmptyQueryBuilder
.
They can probably be empty classes and all share a trait with no-op methods.
I'm happy to take over this PR if you want.
Always happy to make it better. I've refactored it to have a base Happy to move to a trait if you think thats better, but the EmptyQueryBuilder class might be useful in and of itself. |
I apologize for being persistent, but I need to launch a website for a major regional bank next week. Will the PR be completed by then, or should I proceed with refactoring the code? |
Sorry but we can't promise an ETA unfortunately. You might be able to workaround it by overriding cms/src/Fieldtypes/Entries.php Lines 369 to 371 in ed9161d
Something like if (empty($values)) {
return null;
} |
Following the suggested approach here, this PR introduces an
EmptyQueryBuilder
which is returned by the Entries field type when there are no results to return.This ensures no query is actually run, which is a performance win.
@jasonvarga If this is what you intended then should I roll it out to the terms and assets fieldtypes?
Closes #11152.