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

Clearer Connection return types for affectedRows #5334

Closed
wants to merge 1 commit into from

Conversation

Seldaek
Copy link
Member

@Seldaek Seldaek commented Mar 25, 2022

Q A
Type improvement
Fixed issues Alternative to/closes #5317

Summary

Per

dbal/src/Connection.php

Lines 1157 to 1160 in 83f779b

return $result->rowCount();
}
return $connection->exec($sql);
the two ways executeStatement can return is via Result::rowCount and DriverConnection::exec but those two have an int return type, so it is currently impossible for executeStatement to return a string.

@Seldaek
Copy link
Member Author

Seldaek commented Mar 25, 2022

Found some problems in PHPStan fixed in phpstan/phpstan-src#1116 - similar issues are present in Psalm still but I'd rather not waste more time if this change isn't going to be accepted, so waiting for feedback first.

$count = $this->connection->affected_rows;

if (is_string($count)) {
$count = PHP_INT_MAX;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is by no means acceptable. The library must not alter the data returned by the underlying driver in order to fit it into its own API which is incorrectly defined.

Copy link
Member

@derrabus derrabus Mar 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the alternative? Right now, if the number of affected rows is larger than PHP_INT_MAX and thus returned as a string by ext-mysqli, we will run into a TypeError here.

https://3v4l.org/03d4D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type should be int|string as it is in 4.0.x:

public function exec(string $sql): int|string

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, but don't we need a solution for DBAL 3 as well? After all, this is our maintained branch. Raising type errors seems like a mistake to me.

Copy link
Member

@morozov morozov Mar 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in #5317 (comment), it seems acceptable to remove the int return type declaration in 3.x since it's invalid.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let's do that.

@Seldaek Seldaek closed this Mar 28, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 29, 2023
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.

None yet

3 participants