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

Files: 
CommentFileSizeAuthor
#33 rollback-d7-813540-33.patch1.94 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 37,345 pass(es).
[ View ]
#27 null-sql-comparison-never-true.patch2.23 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 37,049 pass(es).
[ View ]
#21 null-sql-comparison-never-true.patch2.28 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 33,994 pass(es).
[ View ]
#11 null-sql-comparison-never-true.patch2.23 KBjbrown
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch null-sql-comparison-never-true_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#4 null.patch885 byteschx
PASSED: [[SimpleTest]]: [MySQL] 20,545 pass(es).
[ View ]
null.patch1.02 KBchx
FAILED: [[SimpleTest]]: [MySQL] 20,542 pass(es), 0 fail(s), and 1 exception(es).
[ View ]

Comments

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.

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

Status:Needs work» Needs review
StatusFileSize
new885 bytes
PASSED: [[SimpleTest]]: [MySQL] 20,545 pass(es).
[ View ]

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

I like #4.

Status:Needs review» Reviewed & tested by the community

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

Crell -- any thoughts?

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

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.

Title:Handle NULL values nicelyComparisons involving NULL must never return true
Version:7.x-dev» 8.x-dev
Category:task» bug
Status:Closed (fixed)» Needs review
StatusFileSize
new2.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch null-sql-comparison-never-true_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

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!

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

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

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.

#11: null-sql-comparison-never-true.patch queued for re-testing.

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

#11: null-sql-comparison-never-true.patch queued for re-testing.

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.

#11: null-sql-comparison-never-true.patch queued for re-testing.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.28 KB
PASSED: [[SimpleTest]]: [MySQL] 33,994 pass(es).
[ View ]

No need to change any doxygen?

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.

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?

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.

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.

Status:Needs work» Needs review
StatusFileSize
new2.23 KB
PASSED: [[SimpleTest]]: [MySQL] 37,049 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Angie, your call.

I'm in support.

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!

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.

Title:Comparisons involving NULL must never return trueComparisons 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!

Status:Active» Needs review
Issue tags:+Needs change record
StatusFileSize
new1.94 KB
PASSED: [[SimpleTest]]: [MySQL] 37,345 pass(es).
[ View ]

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.

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.

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.

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.

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

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.

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.

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.

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.

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

Well, it's not exactly fancy. :)

git revert 06f31080
:wq
git status
git push

:D

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:

<?php
$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:

<?php
$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?

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. :-( )

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.

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

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

Title:Document that comparisons involving NULL must never return trueComparisons involving NULL must never return true

Status:Fixed» Closed (fixed)

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

Title:Comparisons involving NULL must never return trueChange 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.

Title:Change notice: Comparisons involving NULL must never return trueComparisons 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.