Widen the return type of Result::rowCount()

[derrabus]

Q	A
Type	improvement
Fixed issues	N/A

Summary

We currently allow Connection::exec() to return the number of affected rows as a string, mainly to accommodate the MySQLi driver. However, we don't allow Result::rowCount() or Statement::execute() to do the same. Here we document int as native return type.

In the scenario where MySQLi would report the number of rows as string, we would currently cause a type error because strict types are enabled on 4.0.

I realize that this change is painful for projects that run a static analyzer on code that interfaces with DBAL. However, we have decided that we want to leverage drivers that report large numbers of affected rows as strings for Connection::exec() and I believe we should be consistent about that throughout the API.

[greg0ire]

 The largest integer supported in this build of PHP. Usually int(2147483647) in 32 bit systems and int(9223372036854775807) in 64 bit systems. 

I realise I should have brought this up while reviewing #5274, but how widespread are 32 bit systems nowadays? I'm wondering if this is the right move.

[derrabus]

I realise I should have brought this up while reviewing #5274, but how widespread are 32 bit systems nowadays? I'm wondering if this is the right move.

Okay, but we still support 32bit, do we. So let's assume we are on 32bit, use the MySQLi driver and execute a statement that affects 3G rows. What should we do?

• Cause a TypeError? Probably not.
• Return some magic value like 0 or -1?
• Return PHP_INT_MAX?
• Cast to a float? int|float is probably a better return type, but it's still wider than we might want it to be.
• Throw an exception? Probably okay for rowCount() but not for methods like execute() where userland code often doesn't even check the returned value.

[greg0ire]

There are 3 conditions, the likelihood of each seems low. I'm hesitating between throwing every time, and throwing on rowCount() only, returning PHP_INT_MAX otherwise.

[greg0ire]

Looks good code-wise. Not sure what to think about the overall move, it feels we're pandering to a corner case of a corner case of a corner case. I'd like to know what others think.

[derrabus]

it feels we're pandering to a corner case of a corner case of a corner case.

Yeah, I agree. Still, this PR is consistent with the rest of the codebase.

If we want to revert the decision made in #5274, we need to pick one of the options in #5879 (comment) and fix all return types accordingly.

[phansys]

I'm not aware about the general concerns or impact it may have, but from a distant point, I agree that a extreme corner case (ideally) should not push the library to relax types that at first sight seem the sanest and the right ones.
That said, if string is allowed for countable methods, I guess we could at least use the numeric-string type check.

[SenseException]

MySQLi and PHP_INT_MAX are two different things though, that are handled with the string type. Supporting 32bit systems is similar like supporting old PHP versions that aren't supported anymore. I hope there aren't many servers out there being 32bit.

[GromNaN]

Triggering a warning with trigger_error is not an option I suppose. Otherwise return PHP_INT_MAX + log warning.

Adding (int) in all places we use the returned value of execute() is noisy. I would prefer to return int everywhere.

[greg0ire]

Then let's revert #5274 and #5269? I couldn't find complaints about us returning an integer, but the opposite is not true: #5318

[derrabus]

Triggering a warning with trigger_error is not an option I suppose.

No.

Otherwise return PHP_INT_MAX + log warning.

Our drivers don't have access to a logger, so not an option either.

Adding (int) in all places we use the returned value of execute() is noisy.

Then don't? In what situations do you actually need this value to be an integer anyway?

[derrabus]

Then let's revert #5274 and #5269?

With what alternative?

I couldn't find complaints about us returning an integer, but the opposite is not true: #5318

People complain because their static analysis tool tells them to complain. I don't work complaint-driven; I need a good argument to revert those changes.

How does that fact that the number of rows could be expressed as a numeric string actually impede anyone? Or in other words: What do you usually do with the row count?

• Display the number in debug output and logs? Works perfectly with a string.
• Check if the number is greater than zero? You can do that: https://3v4l.org/akbPq
• Calculate a sum over multiple operations? Also works fine (as long as you can rule out integer overflows anyway)

So what actual problem do we solve by limiting the return type to integers again?

[GromNaN]

Adding (int) in all places we use the returned value of execute() is noisy.

Then don't? In what situations do you actually need this value to be an integer anyway?

An illustration of the type of code that I have to update: https://phpstan.org/r/7985f9bc-3d65-4b21-baa3-f29636645ff6

[derrabus]

An illustration of the type of code that I have to update:

It's funny by the way that the static analysis tool can be silenced with an int cast here since integer + integer is not guaranteed to yield an integer. If the sum overflows, you'll still get a float.

Also note that the row count returned here is more or less informational. Some drivers return 0 or even negative values if they could not determine the number of affected rows.

[greg0ire]

With what alternative?

None, to be frank. I mean I wouldn't write any code to handle this particular case, since we are not going to be able to test it. At best, I'd put a comment saying "If you get something else than an integer here, please report this at #5879 , you might win an iPad."

[derrabus]

None, to be frank. I mean I wouldn't write any code to handle this particular case

That's basically the first option listed in #5879 (comment): A TypeError.

[greg0ire]

Oh right, I understood that as: let's provide a custom error message, but still throw a TypeError to be compatible with what happened before. But yes, option 1 then.

[derrabus]

I feel like this is better discussed on an actual PR. I'm going to merge this one as it is consistent with previous changes to 4.0.x. Someone who wants to revert those types, please open a PR so we can discuss how we want to deal with MySQLi returning strings.

But to be honest, I still think such change would only please static analyzers and not fix a real problem.