Database::selectSQLText interprets $conds = '0' as “no conditions”, i. e. a query without WHERE clause, due to its use of the PHP empty function. I assume this is mainly meant to match [], so perhaps a check against that (=== []) would be better and other cases should throw exceptions instead.
Part of https://wikitech.wikimedia.org/wiki/Incident_documentation/20180226-WikibaseQualityConstraints.
I think passing in the empty string (and maybe null or false) should also be considered to be an empty where clause.
I note that the string "0" for $conds would not produce correct SQL anyway. MySQL and SQLite do accept a query like SELECT * FROM foo WHERE 0, but PostgreSQL, MSSQL, and Oracle all reject it.
It's probably not appropriate for MediaWiki's Database class to try to detect whatever random bad SQL someone might throw at it. People should test their code.
I agree. Strings are specifically allowed by the method' documentation, so empty string should have the intended effect of generating a query with no WHERE conditions.
I wouldn't be surprised if existing code somewhere is passing null or false intending the same effect.
Fair enough, but I’d assume it’s not appropriate for the Database class either to turn bad SQL into unexpected, good SQL :) especially if that good SQL (without WHERE) is likely to return a huge result set.
Strings are specifically allowed by the method' documentation, so empty string should have the intended effect of generating a query with no WHERE conditions.
I wouldn't be surprised if existing code somewhere is passing null or false intending the same effect.
Okay. What about 0 or 0.0? Those are the other values where empty() returns true.
@Anomie wrote
It's probably not appropriate for MediaWiki's Database class to try to detect whatever random bad SQL someone might throw at it. People should test their code.
That's my fault for assuming that WHERE 0 would work in other versions of SQL. I suppose WHERE 0 = 1 would work in all SQL dialects? The only systems I have for testing are MySQL and SQLite. That's also the only systems we run CI against.
Anyway - while you are right that it's not appropriate for MediaWiki's Database class to try to detect whatever random bad SQL someone might throw at it, it should also not silently ignore it, causing the where condition to be ignored entirely. Treating "0" in the $where parameter the same as no $where parameter is clearly a bug.
@Bawolff wrote
I think passing in the empty string (and maybe null or false) should also be considered to be an empty where clause.
The empty string yes. As per the method documentation, only string and arrays are allowed, so false should not be accepted, and raise an exception. So should null, at least in theory.
Change 415069 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Pass '' instead of false for the $conds parameter in select calls.
so false should not be accepted, and raise an exception. So should null, at least in theory.
I mildly disagree here. We use loose typing all over the place. I think false being equivalent to no where clause is behaviour I would expect in this context.
The problem is that $conds = false is really easy to mis-interpret. Does it mean "no conditions" or "the condition that is always false"?
We should really stop using loose typing, it's causing all kinds of headaches. If we want to allow it, it should be documented. Interface requirements should be taken seriously.
But the interface spec is younger than some of the code calling the method, so I have changed my patch to raise a warning instead of causing a hard failure.
Do you know of anyone who has actually been confused by that? Or is this a "someone might maybe be confused, possibly, for a moment or two"?
Change 415069 merged by jenkins-bot:
[mediawiki/core@master] Pass '' instead of false for the $conds parameter in select calls.