Fixes #330 current NULL for mysqli

[ezimuel]

This PR fixes #330 and replaces #334. I focused the fix directly on mysqli adapter, and not anymore in the Zend\Db\ResultSet\AbstractResultSet::current(), changing the default $currentData value of Zend\Db\Adapter\Driver\Mysqli\ResultSet to NULL. I added an integration test to reproduce the issue, as reported in #330.

[ezimuel]

@bartmcleod I revert the change of #334, your commit 8276cf6#diff-86b3a79320eb564e46456a1b75003271R197 has been reinsert, now the solution is directly in the mysqli adapter.

[WillyBaldy]

Good work. I'm just wondering if ResultSet/AbstractResultSet-->current() should not at least enforce that the resulting value is null or an array, or else it would throw an exception? We use null in the method signature but we actually don't enforce it? Seems a bit fishy to me.

[WillyBaldy]

(The idea of a ResultSet and pretty much the whole Zend\Db is to abstract the driver you're using, so "programming errors" in some drivers should not lead to different result to the current() method on ResultSet)

[WillyBaldy]

The reason I'm picky on this is that there's literally hundreds of places in my code where I do if ($row === null) to know if it's empty after calling ResultSet-->current(), so I feel the bug could come back any time in the future and I would not have a clear explanation of the error, the code keeps on going. At least, an exception "The driver is returning an invalid value" or something like that would allow me to know right away it's an internal error to Zend\Db.

[ezimuel]

@willy0275 you right, there are some inconsistencies in these adapters return types. We can think to uniform everything and introduces Exceptions in ver. 3.0.0 since this will be a BC break.

[willy-opale]

Any schedule on this important bug fix? We're still using a private repo which is a copy of zend db 2.9 with a temporary fix in the source code.

[Ocramius]

Do we have any stricter type that can be used here?

[ezimuel]

This can be null or array, depending on the current() state. Moreover, I just reviewed all the adapter results and this is the scenario:

Adapter\Driver\Pgsql - current(): array|false
Adapter\Driver\Mysqli - current(): array|null
Adapter\Driver\Oci8 - current(): array|false
Adapter\Driver\Pdo - current(): array|object|false
Adapter\Driver\Sqlsrv - current(): array|false|null

I think we should try to normalize it. Maybe, we can throw an Exception if the adapter returns an error during the fetch process, and use null as empty value. Of course, this will be released with zend-db 3.0.0
@Ocramius and @willy0275 what do you think?

[Ocramius]

This is a BC break for anything inheriting from this class. Sadly, the class is not final, so these changes land in future releases (develop)

[WillyBaldy]

The thing is, 2.9 introduced a BC break from 2.8 because recent previous versions used to return null on empty result sets. I remember more than a year ago I was using false (even though signature said null) and it was "fixed" and I had to change all of my code to use null instead of false. So I think a BC break here from 2.9 would be acceptable to put things back to "normal". If not, I'll have to wait for 3.0 for a "simple" bug fix. I can't even go back to 2.8 because there are requirements that are not met with PHP 7.2.

[ezimuel]

Yes, I'm planning it for zend-db 3.0.0. I'll point to develop, thanks!

[ezimuel]

I'm going to release this in 2.10.0 since it's a rollback of 2.8 behaviour.

[WillyBaldy]

Thanks! We are still on our local patched version of 2.9, so it's going to be great to be able to switch back to the main repo.

[Ocramius]

Let's please not use traits for stashing out duplicated code: this is static code, and should be kept as such (static utility)

[ezimuel]

Can you elaborate why? Moreover, you mentioned this is static code?! Thanks.

[ezimuel]

Merged in develop.