->condition('foo', NULL) does not translate to foo IS NULL. Boo. Edit: the use case is the patch #813492-3: HTTPS sessions use an invalid merge query where I needed to write special cases for NULL.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Sounds like a good idea from a DX perspective, but I'm slightly worried by the fact it's easy to get a NULL from PHP.

Status: Needs review » Needs work

The last submitted patch, null.patch, failed testing.

Crell’s picture

Hm. If we do that, do we still need isNull() and isNotNull()? Those were added precisely for DX purposes.

chx’s picture

Status: Needs work » Needs review
FileSize
885 bytes

I see no harm in having a wrapper for the explicit IS NULL cases.

jbrown’s picture

I like #4.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

This is pretty much what I would expect from condition('foo', NULL) so RTBC.

Dries’s picture

Crell -- any thoughts?

Crell’s picture

It's a minor issue either way, but looking at the issue chx linked it seems like a nice cleanup. I'd almost rather see the existing isNull() implementation left alone, though, lest someone later try to "clean up" the code by removing isNull() as redundant. (It's important for DX and gives good parallelism with isNotNull().)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Alright. Committed to CVS HEAD. Feel free to re-open when you want to follow-up on #8.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

jbrown’s picture

Title: Handle NULL values nicely » Comparisons involving NULL must never return true
Version: 7.x-dev » 8.x-dev
Category: task » bug
Status: Closed (fixed) » Needs review
FileSize
2.23 KB

So this patch actually introduced a massive bug.

In SQL comparing a value with NULL and using IS NULL and two _entirely_ different comparisons.

If you compare any value (including NULL) with NULL the answer is always false. This is what happens when tables are joined together (making NULL values an early-out of the comparison). If you use the IN() function the same rule applies.

This is a fundamental rule of SQL.

If a module is requesting that a field be compared with NULL this means that the condition is always false.

From http://dev.mysql.com/doc/refman/5.6/en/problems-with-null.html :

In SQL, the NULL value is never true in comparison to any other value, even NULL. An expression that contains NULL always produces a NULL value unless otherwise indicated in the documentation for the operators and functions involved in the expression.

However, DatabaseCondition::condition() now checks if the parameter is NULL and silently changes the operator to IS NULL.

This behaviour is completely incorrect and will result in hard to track down bugs. If you want to search for NULLs, you have to do it explicitly!

sun’s picture

+1 for #11 -- understanding the special case of NULL values is hard enough in plain SQL alone, so it doesn't really help to have a DB layer that obfuscates the special meaning even more.

Crell’s picture

Sigh. I hate SQL.

#11 is correct, I believe. My only concern is if this might break something elsewhere, where people are assuming that a null value => IS NULL (wrong as that may be). It looks like nothing in core is. Is it possible to do a quick check in contrib to see if we need to document this? (It's a very slight API change; I'm OK with making it since the current behavior is wrong, but we may want to notify people of it.)

jbrown’s picture

I regard this as fixing a bug in our implementation of the API, rather than an API change, but it should still be mentioned in the module upgrade instructions.

I found this bug because a module was acting strangely. I reached the conclusion that core must be silently converting comparisons with NULL to an IS NULL function. Examining the code confirmed my suspicions. I was later pointed to this issue and I discovered that I had actually endorsed the original change...

I had to patch my module to work around the bug: http://drupalcode.org/project/storage_api.git/commitdiff/353457e

In most (all?) cases it's better to not execute or to rearrange the query rather than specifying a condition that will always be false, but I think that the API should work correctly even if the query hasn't been optimized in this way.

Fixing this will fix bugs more than it will introduce them, but there may be queries in contrib modules that will need to switch to using isNull(). I'm not sure if it's feasible to find these ahead of time.

jbrown’s picture

jbrown’s picture

I think this should just be applied to 8 as it will almost certainly break some contrib modules.

jbrown’s picture

Crell’s picture

I'm not certain that it will break any contribs. I'm inclined to go with "follow the SQL standard, dude!" unless someone can demonstrate that there would be wide-spread breakage.

jbrown’s picture

Status: Needs review » Needs work

The last submitted patch, null-sql-comparison-never-true.patch, failed testing.

jbrown’s picture

Status: Needs work » Needs review
FileSize
2.28 KB
chx’s picture

No need to change any doxygen?

jbrown’s picture

I think this patch brings the implementation in line with the interface: http://api.drupal.org/api/drupal/includes--database--query.inc/function/...

The interface doxygen has no mention of converting NULL values to IS NULL.

jbrown’s picture

No one wants to have the responsibility of RTBCing this for D7 in case a contrib module blows up. Can't this just go into 8?

Crell’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

This looks good to me for Drupal 8 at least. Per comments above I would support applying it to Drupal 7 as well, although the patch would need to be rerolled, of course. The final decision there is up to webchick.

Dries’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs work

Committed to 8.x. Moving to 7.x for webchick to consider; looks like it will need a re-roll though.

jbrown’s picture

Status: Needs work » Needs review
FileSize
2.23 KB
Crell’s picture

Status: Needs review » Reviewed & tested by the community

Angie, your call.

Damien Tournoud’s picture

I'm in support.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Hm. The fact that nothing else in the code had to change apart from new tests bodes well for this backwards-compatibility-wise, even though it looks a bit risky. I think I'm cool with committing this, as if there are problems with it we should hopefully have time to find them before next release.

Committed and pushed to 7.x. Thanks!

xjm’s picture

Status: Fixed » Needs review

I think we need to reconsider this backport, because contrib modules might unknowingly rely on the pre-patch behavior. If nothing else it will need a change notice, but I am not sure.

chx’s picture

Title: Comparisons involving NULL must never return true » Comparisons involving NULL must never return true [roll back]
Priority: Normal » Critical
Status: Needs review » Active

This change is a hard API change which can not be done in Drupal 7. If someone was relying on this behavivour which is arguably indeed incorrect then the module breaks. We don't do that in minor releases unless security demands it. In this case it's the other way around -- this can cause security issues!

xjm’s picture

Status: Active » Needs review
Issue tags: +Needs change record
FileSize
1.94 KB

Here's a patch to roll it back. We should also add a change notice for this for D8, because some contrib developers, not naming names, might have considered the original behavior a feature of the API. http://drupal.org/node/310086 should probably be updated/clarified as well.

Damien Tournoud’s picture

I'm still in support. More often then not you are going to be bitten by this. NULLable columns are arguably usually rare, but I still believe that the behavior should be reverted. As I mentioned in #1, it's actually really easy to get a NULL from PHP.

Crell’s picture

xjm: Can you indicate an actual problem without naming names, or just the theory of a problem? I'm inclined to favor "follow the spec" over "support our bugs" by default unless there's a specific reason not to.

xjm’s picture

There are existing queries in contributed modules that rely on $query->condition(field, NULL) returning matches for field IS NULL, and changing the API behavior--even if it's a bad one--will break those queries silently, making them return different results, on production websites. Edit: I have a documented instance of this in an actual module. You can ping me on IRC for details if you like.

Dave Reid’s picture

Yes, looks like this could also affect the i18n module as well:
http://drupalcode.org/project/i18n.git/blob/refs/heads/7.x-1.x:/i18n_str...

jbrown’s picture

Well this is a tough one. It is very easy to get NULLs from PHP. They shouldn't be matching to anything, so the fix committed to 7 will eradicate obscure bugs. But there may be a period of pain. Stable releases of modules may have to marked as unsupported to force upgrades.

xjm’s picture

Stable releases of modules may have to marked as unsupported to force upgrades.

Hmm, I am concerned this directly contradicts the backport policy:

Don't break (known) existing contrib modules and themes or (unknown to us) potentially existing custom modules and themes.

This patch causes bugs with at least two contributed modules, and that's just what has been found within one business day of the patch being committed. I am concerned that there are likely many more. Furthermore, marking a module as unsupported does not necessarily cause sites to upgrade immediately. Finally, this is not something that will start throwing a PDO exception for modules with code we no longer support--it will fail silently, so sites may not even have any indication something is broken when testing the 7.11 upgrade.

If we do not roll back this API change for 7.x, we are deliberately and knowingly breaking production sites with 7.11. We didn't know that in #25 - #30, and that's okay, but we do now.

jbrown’s picture

Yeah - I support this being rolled back for 7.

We need to put a big warning in the documentation for 7 and create a change notification for 8.

Crell’s picture

OK, so xjm showed me the security hole that this opens up in a somewhat widely used module. Blargh.

At this point, I think we have two options:

1) Keep it in D7 and use the rarely-used module maintainer email list to blast a notice to module maintainers warning them of this change.

2) Revert this commit in D7. (Note: Not apply a rollback patch. Git has a "undo this particular commit, as a new commit" command. Use it.)

I suspect webchick is going to opt for option 2.

If we go with option 2, I recommend we still mark that usage as deprecated so people know they're not supposed to do that.

chx’s picture

Crell, if you want webchick to execute some fancy git command please supply it :)

xjm’s picture

Well, it's not exactly fancy. :)

git revert 06f31080
:wq
git status
git push

:D

xjm’s picture

I've updated the DBTNG docs to correct this point. Old text:

Null values

In some cases, one wants to filter a database field on whether the value is or is not NULL. Although it is possible to do using condition(), the following utility methods are far preferred as they are self-documenting:

$query->isNull('myfield');
// Results in (myfield IS NULL)

$query->isNotNull('myfield');
// Results in (myfield IS NOT NULL)

Both methods may be chained and combined with condition() and where() as desired.

New text:

Null values

To filter a database field on whether the value is or is not NULL, use the following methods:

$query->isNull('myfield');
// Results in (myfield IS NULL)

$query->isNotNull('myfield');
// Results in (myfield IS NOT NULL)

Both methods may be chained and combined with condition() and where() as desired.

Note: Although it is possible in Drupal 7 to check for NULL values using condition('field', NULL), that usage is deprecated and should not be used. Use the methods above instead. See #813540: Comparisons involving NULL must never return true for more information.

As is pointed out above, there's no mention of this "feature" in the docs for QueryConditionInterface::condition(). Should we explicitly add a comment that that condition() should not be used, with @see to isNull() and isNotNull()?

Aside: I noticed something else when I checked the API docs. The signature is still:
public function condition($field, $value = NULL, $operator = NULL);
I can't see in the patch that this was changed, but shouldn't we remove the default value of NULL in D8?

Crell’s picture

Thanks, xjm.

Yes, I think we should add such a comment to the condition() docblock.

I don't recall off hand if condition($field) is ever a legitimate action other than for NULL. If not, then we probably should make $value required. (A quick glance at DatabaseCondition should tell us if that's the case; I just don't have the time to do so right now between phone calls. :-( )

xjm’s picture

jbrown’s picture

Status: Needs review » Reviewed & tested by the community

Yes - $value should be required in D8. In D8 there is no point in issuing a ->condition($field) because it will always return FALSE.

It is too late to change this in D7 - modules may already be using the default value to test for NULLs.

Marking #43 as RTBC.

webchick’s picture

Title: Comparisons involving NULL must never return true [roll back] » Document that comparisons involving NULL must never return true
Category: bug » task
Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work

Cool, thanks for the copy/paste commands in #43. Learned something new. :)

Reverted this change to 7.x. Sorry, Damien. But I'm not comfortable busting widely-used contributed modules (and goodness knows how many custom modules) in a stable point release. Thanks, folks, for catching this.

Back to... needs work, for the deprecated comments? That doesn't seem like a critical bug anymore. :)

xjm’s picture

Category: task » bug
Priority: Normal » Major
Status: Needs work » Fixed

@webchick: The comment corrections should be handled in #1371256: Document that QueryConditionInterface::condition() should not be used to add NULL conditions., so I think we are fixed here. :)

xjm’s picture

Title: Document that comparisons involving NULL must never return true » Comparisons involving NULL must never return true

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

xjm’s picture

Title: Comparisons involving NULL must never return true » Change notice: Comparisons involving NULL must never return true
Version: 7.x-dev » 8.x-dev
Category: bug » task
Status: Closed (fixed) » Active

Whoops, still need Mr. Change Notification. I think.

xjm’s picture

Title: Change notice: Comparisons involving NULL must never return true » Comparisons involving NULL must never return true
Category: task » bug
Status: Active » Fixed
Issue tags: -Needs backport to D7, -Needs change record

chx wrote https://drupal.org/node/2107873. back to fixed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.