No obvious way to do "IS (NOT) NULL" in a built query?

Yeah, 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. :-)

I 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_logic

I 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).

Well 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.

I 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.

and then we special case those operators just as we special case LIKE, BETWEEN, IN, etc.

I don't like "special cases".

That would be closest to the internal implementation, I believe.

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.

Well, 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.

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.

well, i said i don't like "special cases", but what i really meant was that i don't like *exposed* "special cases".

If we decided we wanted to use null($field, TRUE) instead, switching over to that should be a trivial front-end change now.

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.

Yes, 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.

we should at least have a small docblock about having, but that's indeed another issue.

I'm not rerolling this until the extenders patch is done, as they conflict and that one is bigger. :-)

Ah ha!

I only did SelectQueryExtender not SelectQueryInterface. Fixed.

OK, 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.

you forgot The Swedish Chef... Bork, bork, bork!

Cool, 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.

tagging.

Documentation added, finally.