EntityFieldQuery will throw a PDOException with MySQL if you give an empty IN (or NOT IN) condition:
$ drush ev '
$query = new EntityFieldQuery();
$query->entityCondition("entity_type", "node")
->fieldCondition("body", "value", [], "in");
print_r($query->execute());
There is another ticket for this in #1976666: db_select()->condition() doesn't guard against empty set conditions, but that one is assigned to the mysql database component and I'm not sure where this belongs. If it can't or won't be fixed in the database system, maybe it can at least be fixed in the EFQ system.
Comment | File | Size | Author |
---|---|---|---|
#1 | drupal7.entity-system.2177247-1-tests.patch | 1.67 KB | Paul B |
#4 | drupal7.entity-system.2177247-4-tests.patch | 2.78 KB | Paul B |
#5 | drupal7.entity-system.2177247-5.patch | 4.25 KB | Paul B |
#22 | drupal8.entity-system.2177247-22.patch | 2.69 KB | David_Rothstein |
#26 | 2177247_26.patch | 4.47 KB | chx |
Comments
Comment #1
Paul B CreditAttribution: Paul B commentedThe patch adds some failing tests.
Comment #2
Paul B CreditAttribution: Paul B commentedComment #4
Paul B CreditAttribution: Paul B commentedTwo more tests.
Comment #5
Paul B CreditAttribution: Paul B commentedThe patch should fix it.
Comment #7
dawehnerthe question is whether the caller of the API should take care about not passing in arbitrary values.
Comment #8
chx CreditAttribution: chx commentedThe patch looks good and makes a sensible implementation of empty IN / empty NOT IN . That SQL doesnt happen to support such things is immaterial to EFQ.
Comment #10
chx CreditAttribution: chx commented5: drupal7.entity-system.2177247-5.patch queued for re-testing.
Comment #12
Paul B CreditAttribution: Paul B commented5: drupal7.entity-system.2177247-5.patch queued for re-testing.
Comment #13
chx CreditAttribution: chx commentedFinally.
Comment #14
David_Rothstein CreditAttribution: David_Rothstein commentedThis looks like it would need to go into EntityQuery in Drupal 8 first.
I can see the arguments for and against the patch (and for/against keeping this consistent with the database API, i.e. merging the two issues)... but overall I would definitely find it convenient. Not sure it counts as a major bug, though.
Comment #15
chx CreditAttribution: chx commentedThis is not a D8 issue as EFQ doesn't exist in D8. EQ does but that's a totally different animal.
Comment #17
David_Rothstein CreditAttribution: David_Rothstein commentedHm, not sure about that... but moving back to RTBC for now since that fail is likely testbot glitch.
Comment #18
David_Rothstein CreditAttribution: David_Rothstein commentedI tested this in Drupal 8 and really do not understand why the issue wouldn't be equally relevant for Drupal 8.
I tried this code:
And it threw an exception.
This is pretty much the exact same as the equivalent code in Drupal 7:
The goal of the patch in this issue is to make those kinds of entity queries work without an exception being thrown....
Comment #19
chx CreditAttribution: chx commentedI hate Drupal process. Really, really, really hate. What would've been the harm in committing the fix to D7 and reopening it for D8? The two subsystems while superficially similar have absolutely nothing to do with each other and the fix most likely will be entirely different.
So now we need to find someone to fix it in D8, get it committed (roughly half a year) and then reopen for D7, reroll the patch for a year until it gets committed. Awesome.
Comment #20
chx CreditAttribution: chx commentedNope, I refuse to acknowledge these same bugs as they are not. EQ is written from ground up and as it has condition groups the meaning of the bug, the fix, the test, the result are all very different. #2367675: EntityQuery condition functions don't guard against empty set conditions is filed for 8.x and this is RTBC.
Comment #21
chx CreditAttribution: chx commentedI added a patch to the other issue just to demo how utterly different the two are.
Comment #22
David_Rothstein CreditAttribution: David_Rothstein commentedOK, can we agree to get feedback from a Drupal 8 committer on this one, then?
I do not want to make the decision to commit something to Drupal 7 that potentially breaks the backport policy because the main point of the backport policy is to avoid regressions in future versions (Drupal 8) compared to the current version (Drupal 7). But if a Drupal 8 maintainer is OK with considering them separate issues, then OK with me too.
(I do realize "regressions" matters more for actual bugs than something like this which is an API addition/improvement/whatever. But I don't want to randomly decide on my own that the backport policy can be changed that way either.)
I tried to port this patch to Drupal 8 (before seeing your issue) and a relatively quick and straightforward port (minus the tests) looks like the attached and seems to work. Of course it's definitely possible that it's wrong or that there's a better way to do it.
I am tagging this issue for committer feedback but I'm not sure anyone pays attention to that... Feel free to take other steps to get a Drupal 8 committer's attention :)
Comment #23
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, while trying to see what it would look like in Drupal 8, I realized that the Drupal 7 patch is incomplete... it doesn't work for COUNT queries?
In other words, this works:
But this doesn't:
Comment #24
chx CreditAttribution: chx commentedI will get a D8 committer to chime in. #22 is definitely wrong -- as I mentioned in #20 we support groups now and I meant condition groups, with AND/OR support. You can not (easily) deduce what the end result is!
I maintain these are two different bugs even if they are related.
Setting to CNW because of #23.
Comment #25
David_Rothstein CreditAttribution: David_Rothstein commentedSo is it possible the approach in the Drupal 8 patch (in #2367675: EntityQuery condition functions don't guard against empty set conditions) would be better for Drupal 7 also? I think the equivalent for Drupal 7 would be to add that code in the addCondition() method instead... Seems like it might be a more compact patch, plus solve the count query issue as well.
Comment #26
chx CreditAttribution: chx commentedLet's try.
Comment #28
alexpott@chx
Is hyperbole and it does not help your argument. The 10 line patch in #2367675: EntityQuery condition functions don't guard against empty set conditions will not take this long to get done in D8. Also the fact that #26 is so similar to #2367675: EntityQuery condition functions don't guard against empty set conditions leads me to conclude this is the same bug. EFQ became EQ. Backporting improvements from EQ to EFQ seems possible. Therefore I think we should follow our backport policy.
Setting this issue to 8.0.x. Closing the other issue.
Patch attached fixes the patch from #2367675: EntityQuery condition functions don't guard against empty set conditions -
addExpression()
does not do what you think it does and adds tests.Comment #29
alexpottFixed unnecessary code comment.
Comment #30
catchDo we really want to make that a legal query? Or should we throw an exception on empty array in EFQ so it's consistent across storage?
When I've hit this exception, it's often been a case where the query didn't even need to be run if there were no results, so in those cases it was a useful exception and saved unnecessary queries.
Comment #31
aspilicious CreditAttribution: aspilicious commentedWell, on the other side I have cases where my array can be empty (valid).
And at the moment I have to do something like this:
Comment #32
catchShouldn't that just be:
?
edit: no that logic is reversed. Still feels odd to bake this in.
Comment #35
samuel.mortensonRe-rolled patch from #29. Interdiff is messy as things have moved around in 10 months but attaching anyway.
Comment #36
samuel.mortensonComment #39
cilefen CreditAttribution: cilefen commentedIn a meeting with the core maintainers and the entity team it was decided to make this a normal task.
Echoing comment #30, it is better to have an exception thrown on an illegal query.
(edited)
Comment #40
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedIf not an exception, then it should short-cut and return no results.
Comment #51
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
Tagging for steps to reproduce this issue
Tagging for IS update as a lot has changed in 7 years for D9 + D10