Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

SakiTakamachi
Copy link
Member

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 SakiTakamachi mentioned this pull request Oct 31, 2023
Open
@SakiTakamachi SakiTakamachi force-pushed the feature/gh-12576 branch 2 times, most recently from 8445b31 to 42868db Compare October 31, 2023 16:30
@SakiTakamachi
Copy link
Member Author

SakiTakamachi commented Oct 31, 2023 •
edited
Loading

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

@SakiTakamachi SakiTakamachi force-pushed the feature/gh-12576 branch 2 times, most recently from c41ce6b to 35fb0af Compare November 1, 2023 14:56
Girgias
Girgias reviewed Nov 1, 2023
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Tests need to be updated

@SakiTakamachi
Added transaction check process
832c30f 
fix macro

fix report flags

fix tests

fix tests
@SakiTakamachi
Copy link
Member Author

SakiTakamachi commented Nov 1, 2023 •
edited
Loading

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
Copy link
Member Author

@Girgias
Tests fix seems to have worked.

mvorisek
mvorisek reviewed Nov 2, 2023
View reviewed changes
ext/mysqli/tests/mysqli_rollback.phpt
@@ -53,6 +53,14 @@ if (!have_innodb($link))
echo $exception->getMessage() . "\n";
}

$link = my_mysqli_connect($host, $user, $passwd, $db, $port, $socket);
mysqli_report(MYSQLI_REPORT_ERROR|MYSQLI_REPORT_STRICT);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mysqli_report(MYSQLI_REPORT_ERROR|MYSQLI_REPORT_STRICT);

should be not needed because of https://wiki.php.net/rfc/mysqli_default_errmode

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I thought at first, but it seems like this is a global setting. If I didn't set it like this here, I ended up in warning mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

Warning mode is set at the beginning of this test case, and if I do not change the mode with this line, the warning mode will be inherited even if I create a new mysqli instance.

Copy link
Contributor

@mvorisek mvorisek Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see - https://github.com/php/php-src/blob/832c30f38df/ext/mysqli/tests/connect.inc#L10 - new mysqli_driver is an instance of the monostate pattern - https://www.php.net/manual/en/class.mysqli-driver.php

Copy link
Member Author

Choose a reason for hiding this comment

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

right.

mvorisek
mvorisek reviewed Nov 2, 2023
View reviewed changes
ext/mysqli/tests/mysqli_begin_transaction.phpt
Comment on lines +109 to +110

mysqli_rollback($link);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mysqli_rollback($link);
mysqli_commit($link);

to test if the first mysqli_begin_transaction can still be commit

Copy link
Member Author

Choose a reason for hiding this comment

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

The result is the same no matter which one you use. This is because the only errors that occur are due to the checks added this time.

Personally, I'm fine with either.

@mvorisek mvorisek mentioned this pull request Nov 2, 2023
Open
@kamil-tekiela
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

SakiTakamachi commented Dec 11, 2023 •
edited
Loading

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvorisek mvorisek mvorisek left review comments

@Girgias Girgias Girgias left review comments

Assignees
No one assigned
Labels
Extension: mysqli
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No exception on mysqli::rollback when transaction is no longer active
4 participants
@SakiTakamachi @kamil-tekiela @mvorisek @Girgias