Better PDO::inTransaction #4996
Conversation
|
Would that also work with libmysqlclient? |
|
is libmysql also available in PHP7? |
|
ext/mysqli and ext/pdo_mysql can still be built either against mysqlnd or libmysql (see https://externals.io/message/106086, though). Until we decide to drop support for libmysql, we have to support it (at least in a minimal way, e.g. |
|
I can only say that I support dropping support for building against libmysql :) |
| default: | ||
| assert(0); | ||
| } | ||
| var_dump($db->inTransaction()); |
There was a problem hiding this comment.
does consequent calls hit the server again and again?
There was a problem hiding this comment.
does consequent calls hit the server again and again?
Do you mean that whether this call requires network IO? The MySQL server will tell the client if it is in a transaction in every response by a flag, so we can get the latest status no matter what command is executed
There was a problem hiding this comment.
Do you mean that whether this call requires network IO?
yes
The MySQL server will tell the client if it is in a transaction in every response by a flag, so we can get the latest status no matter what command is executed
so does this query this local flag or did it require network IO every time?
There was a problem hiding this comment.
not required, before sending a new request, the state of the transaction will not change, so the state we got from the last response is reliable
There was a problem hiding this comment.
@twose Thanks, I already hit this several times, your PR looks very good, 👍
|
For me, it seems almost like a bug fix, @cmb69, can we merge this? |
|
@johannes, @andreyhristov, any objections to merge this PR? |
|
I haven't gone through this in depth, but I wonder about the practical value. You will know that you are in some transaction, but not exactly which. There could have been an implicit commit (i.e. due to a I wonder if this has to be a fatal error though ... that seems weird to me. I will try to think a bit about this. |
|
Ah, one thing I didn't realized on my first look: This is a common PDO method, so yeah, adding this seems fine. Still I think we should think about lowering away from fatal error ... |
|
So, can anyone approve it? or any more suggestions? |
|
I've done the necessary changes to support libmysqlclient and committed this patch. |
|
I opened #6355 as a followup fix for https://bugs.php.net/bug.php?id=80260. |
This addresses an issue introduced by #4996 and reported in https://bugs.php.net/bug.php?id=80260. Now that PDO::inTransaction() reports the real transaction state of the connection, there may be a mismatch with PDOs internal transaction state (in_tcx). This is compounded by the fact that MySQL performs implicit commits for DDL queries. This patch fixes the issue by making beginTransaction/commit/rollBack work on the real transaction state provided by the driver as well (or falling back to in_tcx if the driver does not support it). This does mean that writing something like $pdo->beginTransaction(); $pdo->exec('CREATE DATABASE ...'); $pdo->rollBack(); // <- illegal will now result in an error, because the CREATE DATABASE already committed the transaction. I believe this behavior is both correct and desired -- otherwise, there is no indication that the code did not behave correctly and the rollBack() was effectively ignored. However, this is also a BC break. Closes GH-6355.
It looks good for me...
Is there any historical reason to prevents us from doing this?
and we can't do such things now (although it may not make sense):