Skip to content

Unifying mysqli exceptions #6157

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
Closed
wants to merge 2 commits into from
Closed

Unifying mysqli exceptions #6157

wants to merge 2 commits into from

Conversation

kamil-tekiela
Copy link
Member

Mysqli should be treating all client errors the same, however some functions are not converting client errors to warnings/exceptions. The reason for this is a missing call to either MYSQLI_REPORT_MYSQL_ERROR() or MYSQLI_REPORT_STMT_ERROR()

mysqli_set_charset() fix has been addressed in a separate PR #6142

@nikic
Copy link
Member

nikic commented Sep 18, 2020

You need to base your branch on the current master branch. Running git rebase origin/master might fix it.

@nikic
Copy link
Member

nikic commented Sep 23, 2020

Seeing this difference when linking against libmysqlclient:

018+ Warning: mysqli_prepare(): All data must be fetched before a new statement prepare takes place in /home/nikic/php/php-src-libmysql/ext/mysqli/tests/mysqli_report.php on line 56
018- Warning: mysqli_store_result(): (%s/%d): You have an error in your SQL syntax; check the manual that corresponds to your %s server version for the right syntax to use near 'FOO' at line 1 in %s on line %d

@kamil-tekiela
Copy link
Member Author

kamil-tekiela commented Sep 23, 2020 •
edited
Loading

@nikic Which test case is this? It doesn't look like the one I changed.
How can I compile it against libmysqlclient myself on Windows? I downloaded the files but I don't know how to link it.

I have no idea what's wrong with libmysql but I would recommend removing that warning from mysqli. It seems completely out of place. I don't think even Andrey Hristov remembers why that warning is there. It is in mysqli_api.c on line 1785

@kamil-tekiela kamil-tekiela force-pushed the unifying-mysqli-exceptions branch from b6c991c to 48b70b2 Compare September 24, 2020 11:25
@kamil-tekiela
Copy link
Member Author

kamil-tekiela commented Sep 24, 2020 •
edited
Loading

Fixes bugs:

Bug #69656 | mysqli_commit does not commits with MYSQLI_ASYNC, mysqli_kill returns false
Bug #76525 | mysqli::commit does not throw if MYSQLI_REPORT_ERROR enabled and mysqlnd used

@kamil-tekiela kamil-tekiela force-pushed the unifying-mysqli-exceptions branch from 48b70b2 to e5199d4 Compare September 30, 2020 10:58
@kamil-tekiela
Trigger exceptions for autocommit, commit, rollback and mysqli_stmt_a…
a3a0f37 
…ttr_set

mysqli_report test cases for autocommit, commit, rollback and mysqli_stmt_attr_set
Updated test case with no warning control case
@kamil-tekiela
If mysqli_stmt_prepare fails then the error is populated in conn not …
f14ed7b 
…in stmt

However, mysqli_stmt_prepare only checks for errors in statement. If exception mode is enabled the exception is not triggered because of this flawed logic.
Fix: copy the error from conn to stmt. If mysqli_stmt_prepare is called from mysqli_prepare then it doesn't matter because we unset stmt immediately and check for errors in conn.
@kamil-tekiela kamil-tekiela force-pushed the unifying-mysqli-exceptions branch from e5199d4 to f14ed7b Compare October 2, 2020 23:38
@php-pulls php-pulls closed this in 990bb34 Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kamil-tekiela @nikic