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

Explain why the number of affected rows can be a string #5872

Merged
merged 2 commits into from
Jan 29, 2023

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Jan 27, 2023

Q A
Type improvement
Fixed issues #5802 (comment)

Summary

Mysqli is the only driver that can return a string when the number of affected rows is greater that PHP_INT_MAX (doc)

Adding a comment in order to make clear for users they can safely cast the result of Connection::executeStatement() to (int) when they don't use this driver, or the executed statement cannot update so many rows.

@@ -21,7 +21,7 @@ public function __construct(private readonly PDO $connection)
$connection->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
}

public function exec(string $sql): int|string
public function exec(string $sql): int
Copy link
Member Author

Choose a reason for hiding this comment

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

PDO::exec() returns int|false.

@@ -49,7 +49,7 @@ public function quote(string $value): string
return "'" . str_replace("'", "''", $value) . "'";
}

public function exec(string $sql): int|string
public function exec(string $sql): int
Copy link
Member Author

Choose a reason for hiding this comment

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

sqlsrv_rows_affected returns int|false.

@@ -78,7 +78,7 @@ public function quote(string $value): string
* @throws Exception
* @throws Parser\Exception
*/
public function exec(string $sql): int|string
public function exec(string $sql): int
Copy link
Member Author

Choose a reason for hiding this comment

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

oci_num_rows returns int|false.

Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

You are correct that only MySQLi supports row counts > PHP_INT_MAX at the moment. But we should not close the door here: If one of the other extensions at some point decides to support this as well, we should not be forced to issue a new major release of DBAL. Since all driver connection classes are final, I think your return type changes are fine.

UPGRADE.md Outdated Show resolved Hide resolved
src/Driver/Connection.php Outdated Show resolved Hide resolved
Copy link
Member Author

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

we should not close the door here: If one of the other extensions at some point decides to support this as well, we should not be forced to issue a new major release of DBAL.

It is wise to anticipate this. I'm OK to revert the type to int|string on all the driver connections, including SQLite3\Connection which is currently int.

With your last sentence, I'm not sure what's your thoughts.

String return type is conditional on the driver used.

Co-authored-by: Alexander M. Turek <[email protected]>
@GromNaN
Copy link
Member Author

GromNaN commented Jan 28, 2023

Seen #5879. Thank you.

@GromNaN GromNaN closed this Jan 28, 2023
@GromNaN GromNaN reopened this Jan 28, 2023
@derrabus derrabus merged commit 8c327e2 into doctrine:4.0.x Jan 29, 2023
@derrabus
Copy link
Member

Thanks!

@GromNaN GromNaN deleted the number-string branch January 29, 2023 18:48
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants