Feature GH-12576: [mysqli] Added transaction check process

[SakiTakamachi]

Closes #12576

Added checks similar to PDO. Personally, this is not a bug but a feature addition, so I set the target branch to master.

cc: @kamil-tekiela

[SakiTakamachi]

Some tests are failing, so I'll fix them later.

[Girgias]

Tests need to be updated

[SakiTakamachi]

The scope of influence on the test was larger than I expected. I think it's probably okay because I passed it on local, but I'll wait for CI to pass.

[SakiTakamachi]

@Girgias
Tests fix seems to have worked.

[kamil-tekiela]

Do you know why it doesn't work for me? I tried with this code:

$mysqli->autocommit(false);
$mysqli->commit();

and I always get the error.

[SakiTakamachi]

What happens if I start a transaction before committing?

It seems like correct behavior that a transaction cannot be committed before it has started.

[kamil-tekiela]

What happens if I start a transaction before committing?

It seems like correct behavior that a transaction cannot be committed before it has started.

Starting the transaction using begin_transaction would get rid of the error, but it cannot work like that. Setting autocommit mode to off is a very common way of starting a "permanent" transaction.

I am also concerned that if MySQL has autocommit disabled then PHP will always throw an error. Is that right? If that's the case then we cannot introduce this change.

[SakiTakamachi]

Starting the transaction using begin_transaction would get rid of the error, but it cannot work like that. Setting autocommit mode to off is a very common way of starting a "permanent" transaction.

Ah I see. In fact, MySQL's CLI tools behave that way. But it seems that the behavior is a little different when using the API, as the transaction is started after executing sql (like an insert statement), etc.

I am also concerned that if MySQL has autocommit disabled then PHP will always throw an error. Is that right? If that's the case then we cannot introduce this change.

That's not my intention, but there may be some hidden use cases that I haven't envisioned.

[SakiTakamachi]

I tried implementing it for the time being, but mysqli is different from PDO, and I personally don't think it's necessary to forcefully add this feature.

[kamil-tekiela]

Starting the transaction using begin_transaction would get rid of the error, but it cannot work like that. Setting autocommit mode to off is a very common way of starting a "permanent" transaction.

Ah I see. In fact, MySQL's CLI tools behave that way. But it seems that the behavior is a little different when using the API, as the transaction is started after executing sql (like an insert statement), etc.

I am also concerned that if MySQL has autocommit disabled then PHP will always throw an error. Is that right? If that's the case then we cannot introduce this change.

That's not my intention, but there may be some hidden use cases that I haven't envisioned.

Ahh, that explains it. But I wonder if this could problems for anyone though. I will have to think about it more.

[kamil-tekiela]

It seems I have forgotten about this PR. It looks good to me after your clarifications. Please only adjust the test file. FYI, I had to rebase to test it locally so you may want to do it too before pushing.

I see that you have opted to make this throw a mysqli_sql_exception regardless of the exception mode. It aligns with PDO even though it doesn't follow the design. I believe that aligning the behaviour is more important here than following the mysqli error model. Otherwise, we'd need to use a different exception type and I don't think we have a suitable one other than a generic Error. After this PR gets merged, I think it would even be a good idea to look into fixing this by moving the check into mysqlnd and making it a proper client error. This would make it obey the error mode setting in both PDO and mysqli automatically without code duplication. EDIT: I forgot PDO may still use libmysql, so it probably still requires code duplication, but the error should still be moved to mysqlnd.

---

I have decided to approve this feature despite knowing that there will be people who won't like this. I believe that the advantages outweigh the disadvantages. PDO has had it forever, and since it was fixed in PHP 8.0, it only seems like it is helping users catch more bugs 1.

There is also an ongoing discussion regarding validation of incorrect values for built-in functions, but I believe this validation does not apply to that discussion for 2 reasons: it validates state not the inputs, and it aligns the behaviour with the more-widely used PDO extension.

Even though we add a new error here, this is not a true BC break. Valid code should still function as it did before. It only creates a BC break when it comes to dead or invalid code, which is a net benefit, especially when it comes to such a critical feature as transactions.

[kamil-tekiela]

Please add mysqli_begin_transaction($link); before the mysqli_multi_query($link, "SELECT 1; FOO;"); call. This test is supposed to verify that the mysqli_report triggers correctly for mysqli_commit and mysqli_rollback

[kamil-tekiela]

Please also add a descriptive NEWS entry.