Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert to using int as a return type #5901

Closed
wants to merge 1 commit into from

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Feb 3, 2023

The only known case where this might affect users of this libary is:

  • if more than PHP_INT_MAX rows are returned
  • if mysqli is in use

The docs about PHP_INT_MAX state:

Usually int(2147483647) in 32 bit systems and int(9223372036854775807)
in 64 bit systems.

If you have 2 billion rows in your database, you're either doing good enough to afford a 64 bit system, or you have a serious issue in your code.

One might say this is to satisfy static analysis tools, but in general, I'd say that more often that not, static analysis improvements makes things clearer for humans doing static analysis with their eyes as well. In our case, one might wonder why on earth we would return a string here.

Follow-up to #5879 (comment) , cc @derrabus @GromNaN

The only known case where this might affect users of this libary is:

- if more than PHP_INT_MAX rows are returned
- if mysqli is in use

The docs about PHP_INT_MAX state:

> Usually int(2147483647) in 32 bit systems and int(9223372036854775807)
in 64 bit systems.

If you have 2 billion rows in your database, you're either doing good
enough to afford a 64 bit system, or you have a serious issue in your
code.

One might say this is to satisfy static analysis tools, but in general,
I'd say that more often that not, static analysis improvements makes
things clearer for humans doing static analysis with their eyes as well.
In our case, one might wonder why on earth we would return a string
here.
@derrabus
Copy link
Member

derrabus commented Feb 3, 2023

One might say this is to satisfy static analysis tools

As someone who does say that: What problems other than static analysis does this change solve?

@greg0ire
Copy link
Member Author

greg0ire commented Feb 3, 2023

None, but IMO it's already a bigger problem than missing support for the situation above.

@derrabus
Copy link
Member

derrabus commented Feb 3, 2023

it's already a bigger problem

Is it? I rarely check the return value of those methods, and if I do, it's mostly something like:

if ($affectedRows > 0) {
    $logger->debug('{rows} have been updated,', ['rows' => $affectedRows]);
}

The fact that a numeric string was returned has zero impact here.

The reason for the union type might be rare, but it's explainable. Moreover, it's almost always safe to int-cast the returned value. The only case in which it isn't is the one you choose to ignore. A piece of documentation, a little note on the doc block should resolve any confusion.

And even if we revert the types: The effect of the current implementation is that the caller in that very rare case would get a TypeError from the depths of the MySQLi driver, without having done anything wrong. And even worse: The caller might not even care about the returned number of affected rows, but the error is triggered anyway.

@greg0ire
Copy link
Member Author

greg0ire commented Feb 3, 2023

Tried your code snippet and indeed, no static analysis issue with this. Also, the fact that you don't need to check the return type to get a TypeError is indeed bad. Let's close this.

@greg0ire greg0ire closed this Feb 3, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants