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

[5.x] Ensure no queries are run when entries field has no values #11184

Open
wants to merge 7 commits into
base: 5.x
Choose a base branch
from

Conversation

ryanmitchell
Copy link
Contributor

@ryanmitchell ryanmitchell commented Nov 28, 2024

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.

@jasonvarga
Copy link
Member

jasonvarga commented Dec 2, 2024

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.

@jasonvarga
Copy link
Member

Ah, it's here:

$query->where('date', 'invalid'); // intentionally trigger no results.

@ryanmitchell
Copy link
Contributor Author

ryanmitchell commented Dec 2, 2024

@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

Copy link
Member

@jasonvarga jasonvarga left a 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.

@ryanmitchell
Copy link
Contributor Author

Always happy to make it better.

I've refactored it to have a base EmptyQueryBuilder with a new EmptyEntryQueryBuilder which implements Statamic\Contracts\Entries\QueryBuilder. For completeness I've also ensured the EmptyEntryQueryBuilder supports the taxonomy where clauses, though as no results are returned it doesn't actually do anything.

Happy to move to a trait if you think thats better, but the EmptyQueryBuilder class might be useful in and of itself.

@yurichudaev
Copy link

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?

@jasonvarga
Copy link
Member

Sorry but we can't promise an ETA unfortunately.

You might be able to workaround it by overriding Statamic\Fieldtypes\Entries and add an early return to the augment method between these lines.

}
$query = $this->queryBuilder($values);

Something like

if (empty($values)) {
    return null;
}

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.

Entries Fieldtype: Empty field triggers queries across all collections instead of configured scope
3 participants