Otherwise it returns NULL which is too ambiguous and makes many comparisons harder.

CommentFileSizeAuthor
#1 dbresult_0.patch2.17 KBrobertdouglass
dbresult.patch1.49 KBrobertdouglass

Comments

robertdouglass’s picture

StatusFileSize
new2.17 KB

update doxygen comments

chx’s picture

Assigned: Unassigned » chx
Status: Active » Reviewed & tested by the community

Also, I consider db_* just a wrapper against mysql_*/pgsql_* and mysql_result never returns NULL so we should not either.

chx’s picture

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

dries’s picture

Priority: Critical » Normal
dries’s picture

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

chx’s picture

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

robertdouglass’s picture

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

drumm’s picture

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

chx’s picture

I 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?

chx’s picture

obviously the above should read: "and none of them relates to a db_result".

robertdouglass’s picture

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

drumm’s picture

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

dries’s picture

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.

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.

drumm’s picture

I was thinking this patch was good because of consistency with low-level database APIs.

dries’s picture

I 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?

robertdouglass’s picture

http://de.php.net/manual/en/function.mysql-result.php

Return Values

The contents of one cell from a MySQL result set on success, or FALSE on failure.

dries’s picture

Thanks for the URL. I'm convinced now -- the patch can go in. :)

dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Anonymous’s picture

Status: Fixed » Closed (fixed)