Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
->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.
Comment | File | Size | Author |
---|---|---|---|
#33 | rollback-d7-813540-33.patch | 1.94 KB | xjm |
#27 | null-sql-comparison-never-true.patch | 2.23 KB | jbrown |
#21 | null-sql-comparison-never-true.patch | 2.28 KB | jbrown |
#11 | null-sql-comparison-never-true.patch | 2.23 KB | jbrown |
#4 | null.patch | 885 bytes | chx |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedSounds 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.
Comment #3
Crell CreditAttribution: Crell commentedHm. If we do that, do we still need isNull() and isNotNull()? Those were added precisely for DX purposes.
Comment #4
chx CreditAttribution: chx commentedI see no harm in having a wrapper for the explicit IS NULL cases.
Comment #5
jbrown CreditAttribution: jbrown commentedI like #4.
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedThis is pretty much what I would expect from condition('foo', NULL) so RTBC.
Comment #7
Dries CreditAttribution: Dries commentedCrell -- any thoughts?
Comment #8
Crell CreditAttribution: Crell commentedIt'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().)
Comment #9
Dries CreditAttribution: Dries commentedAlright. Committed to CVS HEAD. Feel free to re-open when you want to follow-up on #8.
Comment #11
jbrown CreditAttribution: jbrown commentedSo 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 :
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!
Comment #12
sun+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.
Comment #13
Crell CreditAttribution: Crell commentedSigh. 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.)
Comment #14
jbrown CreditAttribution: jbrown commentedI 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.
Comment #15
jbrown CreditAttribution: jbrown commented#11: null-sql-comparison-never-true.patch queued for re-testing.
Comment #16
jbrown CreditAttribution: jbrown commentedI think this should just be applied to 8 as it will almost certainly break some contrib modules.
Comment #17
jbrown CreditAttribution: jbrown commented#11: null-sql-comparison-never-true.patch queued for re-testing.
Comment #18
Crell CreditAttribution: Crell commentedI'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.
Comment #19
jbrown CreditAttribution: jbrown commented#11: null-sql-comparison-never-true.patch queued for re-testing.
Comment #21
jbrown CreditAttribution: jbrown commentedComment #22
chx CreditAttribution: chx commentedNo need to change any doxygen?
Comment #23
jbrown CreditAttribution: jbrown commentedI 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.
Comment #24
jbrown CreditAttribution: jbrown commentedNo 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?
Comment #25
Crell CreditAttribution: Crell commentedThis 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.
Comment #26
Dries CreditAttribution: Dries commentedCommitted to 8.x. Moving to 7.x for webchick to consider; looks like it will need a re-roll though.
Comment #27
jbrown CreditAttribution: jbrown commentedComment #28
Crell CreditAttribution: Crell commentedAngie, your call.
Comment #29
Damien Tournoud CreditAttribution: Damien Tournoud commentedI'm in support.
Comment #30
webchickHm. 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!
Comment #31
xjmI 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.
Comment #32
chx CreditAttribution: chx commentedThis 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!
Comment #33
xjmHere'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.
Comment #34
Damien Tournoud CreditAttribution: Damien Tournoud commentedI'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.
Comment #35
Crell CreditAttribution: Crell commentedxjm: 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.
Comment #36
xjmThere are existing queries in contributed modules that rely on
$query->condition(field, NULL)
returning matches forfield 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.Comment #37
Dave ReidYes, 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...
Comment #38
jbrown CreditAttribution: jbrown commentedWell 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.
Comment #39
xjmHmm, I am concerned this directly contradicts the backport policy:
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.
Comment #40
jbrown CreditAttribution: jbrown commentedYeah - 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.
Comment #41
Crell CreditAttribution: Crell commentedOK, 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.
Comment #42
chx CreditAttribution: chx commentedCrell, if you want webchick to execute some fancy git command please supply it :)
Comment #43
xjmWell, it's not exactly fancy. :)
:D
Comment #44
xjmI've updated the DBTNG docs to correct this point. Old text:
New text:
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 thatcondition()
should not be used, with@see
toisNull()
andisNotNull()
?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?Comment #45
Crell CreditAttribution: Crell commentedThanks, 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. :-( )
Comment #46
xjmI opened #1371256: Document that QueryConditionInterface::condition() should not be used to add NULL conditions. as a followup for the documentation issue.
Comment #47
jbrown CreditAttribution: jbrown commentedYes - $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.
Comment #48
webchickCool, 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. :)
Comment #49
xjm@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. :)
Comment #50
xjmComment #52
xjmWhoops, still need Mr. Change Notification. I think.
Comment #53
xjmchx wrote https://drupal.org/node/2107873. back to fixed.