Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
There doesn't seem to be a proper way to do an "IS NULL" or "IS NOT NULL" condition in a built query. I asked on IRC and DamZ suggested $condition->where($field . ' IS NOT NULL');
with the caveat that it is horrible, which I agree it is - the query builder is supposed to avoid the need for concatenating strings like this, isn't it?
Comment | File | Size | Author |
---|---|---|---|
#18 | condition-is-null.patch | 9.39 KB | chx |
#16 | condition-is-null.patch | 10 KB | chx |
#15 | condition-is-null.patch | 9.39 KB | chx |
#14 | condition-is-null.patch | 9.05 KB | chx |
#12 | condition-is-null.patch | 9.05 KB | chx |
Comments
Comment #1
Crell CreditAttribution: Crell commentedYeah, we should provide a better mechanism there. I guess the knee-jerk suggestion would be
$condition->isNull($fieldname)
and$condition->isNotNull($fieldname)
, but something in me doesn't like having 2 methods like that as we could easily start ballooning our method count. I don't like$condition->null($fieldname, TRUE)
either, though, because of the magic boolean.Any DX input? Someone go find bjaspan and ask him. :-)
Comment #2
alexanderpas CreditAttribution: alexanderpas commentedI personally like
$condition->isNull($fieldname)
and$condition->isNotNull($fieldname)
as they're of the same category as the isTrue and isFalse functions, which are normally generally accepted, and SQL, just like some other parts of Drupal (hook_access()
for example) uses Ternary logic: http://en.wikipedia.org/wiki/Ternary_logicI only would go for
$condition->null($fieldname, TRUE)
if it was easy to negate, which isn't on the database layer without the use of a magic boolean (which is just not nice).Comment #3
Crell CreditAttribution: Crell commentedWell I figured
$condition->null($fieldname, TRUE)
would mean "$fieldname IS NULL" and$condition->null($fieldname, FALSE)
would mean "$fieldname IS NOT NULL", so it's nice and easy to negate that way. I'm still not totally convinced of either mechanism yet, though.Either way we'd probably have the same internal operator handling, which will be annoying to write either way but is, I agree, necessary.
Comment #4
Crell CreditAttribution: Crell commentedI suppose another option would be
$condition->condition($fieldname, NULL, "NULL")
and$condition->condition($fieldname, NULL, "NOT NULL")
, and then we special case those operators just as we special case LIKE, BETWEEN, IN, etc. That would be closest to the internal implementation, I believe. So that's three possible approaches.Comment #5
alexanderpas CreditAttribution: alexanderpas commentedI don't like "special cases".
afaik it's not nice to see internals dripping outside ;)
also, on a documentation side, it's nice to see short, clear and consistent methods, instead of big clunky functions that have several magic inputs.
also, if
$condition->where($field . ' IS NOT NULL');
is ugly,$condition->condition($fieldname, NULL, "NULL")
and$condition->condition($fieldname, NULL, "NOT NULL")
are also ugly.Comment #6
Crell CreditAttribution: Crell commentedWell, here's a version using isNull() and isNotNull(), complete with unit tests.
In order to make it work, I had to introduce a new operator property in the conditional builder, use_value', which is a flag indicating if the conditional fragment should care about the value of the value field. Then we use the built in operator mapping (the special casing mechanism, sorry there's no way around it and it's there for a very good reason) to flag the IS NULL and IS NOT NULL operators to have that flag be false, so that we skip the placeholder after the operator.
That in turn allowed us to use the existing conditions array, which in turn means that ->condition($field, NULL, 'IS NULL') will work perfectly. isNull() is just a wrapper around it. That's good, because it simplifies the code and ensures that alter mechanisms will work properly. So we get two ways of handling NULLs for the price of one. Score. :-) (If we decided we wanted to use null($field, TRUE) instead, switching over to that should be a trivial front-end change now.
I also did something a little unconventional in the unit tests. I split off the table creation to a separate utility method that gets called for each unit test explicitly rather than implicitly. That reduces the number of tables that we have to create and destroy for every single unit test, and hopefully reduces the performance drain on simpletest. If webchick is cool with it then we'll probably switch other unit test over to this mechanism later, but for now this is a decent test test.
Comment #7
alexanderpas CreditAttribution: alexanderpas commentedwell, i said i don't like "special cases", but what i really meant was that i don't like *exposed* "special cases".
and that's a thing i like, don't let the internals drip out.
also i seems to be missing the havingIsNull documentation
talk to webchick about the unit tests, afaik simpletest always does setUp and tearDown before and after respectively each test, in order to have a proper enviroment for each test.
Comment #8
Crell CreditAttribution: Crell commentedYes, but the DB tests setUp routine is very expensive. That's why I want to break it out into separate routines, because there's no reason to load every DB test table up for every single unit test, since most only use one or two of them (or none).
All of the having conditional methods (havingConditions(), havingArguments(), having()) are documented by the comment above them as being identical to the QueryConditionalInterface, but for having. None of them have their own docblocks either. Perhaps they should, but that would be a separate patch.
Comment #9
alexanderpas CreditAttribution: alexanderpas commentedwe should at least have a small docblock about having, but that's indeed another issue.
Comment #11
Crell CreditAttribution: Crell commentedI'm not rerolling this until the extenders patch is done, as they conflict and that one is bigger. :-)
Comment #12
chx CreditAttribution: chx commentedComment #14
chx CreditAttribution: chx commentedAh ha!
Comment #15
chx CreditAttribution: chx commentedComment #16
chx CreditAttribution: chx commentedI only did SelectQueryExtender not SelectQueryInterface. Fixed.
Comment #17
Crell CreditAttribution: Crell commentedOK, I think I misled chx. SelectQueryInterface extends QueryConditionalInterface, so there's no need to define the methods a second time. Otherwise I think it looks good, bot pending.
Comment #18
chx CreditAttribution: chx commentedComment #19
alexanderpas CreditAttribution: alexanderpas commentedyou forgot The Swedish Chef... Bork, bork, bork!
Comment #20
webchickCool, committed this to HEAD with a couple adjustments to the NOT NULL assertion messages.
This change should be reflected in the DBTNG and upgrade documentation.
Comment #21
webchicktagging.
Comment #22
Crell CreditAttribution: Crell commentedDocumentation added, finally.