Closed (fixed)
Project:
Drupal core
Version:
5.x-dev
Component:
database system
Priority:
Normal
Category:
Bug report
Assigned:
Reporter:
Created:
21 Nov 2006 at 21:18 UTC
Updated:
18 Dec 2006 at 11:46 UTC
Jump to comment: Most recent file
Comments
Comment #1
robertdouglass commentedupdate doxygen comments
Comment #2
chx commentedAlso, I consider db_* just a wrapper against mysql_*/pgsql_* and mysql_result never returns NULL so we should not either.
Comment #3
chx commentedalso... the API change is extremely small as if(db_result()) does not change. you need to put the results in an array and check with isset -- but then this is the desired.
Comment #4
dries commentedComment #5
dries commentedI don't understand why this is considered a bug.
The reason for this patch ...
Otherwise it returns NULL which is too ambiguous and makes many comparisons harder.... doesn't seem to fly. With the patch, we'd be able to say:
When it returns FALSE, it is too ambiguous and makes many comparisons harder.Comment #6
chx commentedThe problem is that when you store these in an array, because you will store NULLs and require the slow array_key_exists to be used.
But, as i said, this makes the db_result inconsistent with its PHP origins which is also a wrong.
Comment #7
robertdouglass commentedRight. The reasons for this patch are 1) consistency, 2) performance. Getting FALSE back from this function will have a direct effect on how efficient we can make drupal_lookup_path.
Comment #8
drummWith 81 calls to db_result() in Drupal, I fear that something which depends on db_result returning NULL will break in some subtle way. Same applies to contributions. While the change does make sense, I don't want to make subtle API changes at this point in the release process.
Comment #9
chx commentedI read each and every === and !== in core -- they are very rare, especially if you take out the strpos related ones, 11 and 9 -- and none of them is related to a strpos.
Once again: you need a _strict typed check_ to make this any difference. Who uses strict type checks on database data?
Comment #10
chx commentedobviously the above should read: "and none of them relates to a db_result".
Comment #11
robertdouglass commentedA similar audit of modules tagged with DRUPAL-5 would be possible, Neil, if you ask for it. I think it is most important to get it right in core... especially so close to the heart of core, our database layer, and then document it and let contrib sort itself out.
Comment #12
drummThis also matters for more tests than === or !==, like isset(), as #97317 has demonstrated.
I do think this should happen, but am not sure if it is something for 5.0 or 6. I don't want to waste contrib module authors' time as they "sort it out." I would expect this sort of thing to be rather annoying to debug if it caused a problem.
I'll leave this as RTC so other committers may see this and help decide when this should happen.
Comment #13
dries commentedThe reasons for this patch are 1) consistency, 2) performance. Getting FALSE back from this function will have a direct effect on how efficient we can make drupal_lookup_path.
So far, the only valid reason for this patch is 'better performance', not? If this is about performance, we should back up our claims and measure the gain. I'm not convinced it will be measurable, in which case we want to postpone this patch. If the gain is significant (>= 2%), I suggest we go ahead and commit this.
Comment #14
drummI was thinking this patch was good because of consistency with low-level database APIs.
Comment #15
dries commentedI was thinking this patch was good because of consistency with low-level database APIs.
Which low-level database API. I don't think many low-level database APIs use mixed value returns, but I might be wrong. Url?
Comment #16
robertdouglass commentedhttp://de.php.net/manual/en/function.mysql-result.php
Comment #17
dries commentedThanks for the URL. I'm convinced now -- the patch can go in. :)
Comment #18
dries commentedCommitted to CVS HEAD. Thanks.
Comment #19
(not verified) commented