Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

explicitly display mysqli error #269

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ConorSheehan1
Copy link

Minor change to show mysqli errno and error explicitly. Without this, mysqli errors do not get shown in some debuggers when preparing sql statements, for example whoops.

Here is an example with whoops before
original_schema_error

and after the change.
schema_error_corrected

I think some components already use this style, for example zend-http/src/Client/Adapter/Socket.php line 278.

@@ -206,7 +206,8 @@ public function prepare($sql = null)
$this->resource = $this->mysqli->prepare($sql);
if (!$this->resource instanceof \mysqli_stmt) {
throw new Exception\InvalidQueryException(
'Statement couldn\'t be produced with sql: ' . $sql,
' Error #' . $this->mysqli->errno . ': ' . $this->mysqli->error .
Copy link
Member

Choose a reason for hiding this comment

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

Please use sprintf.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for the late update. Should be sorted now.

Copy link
Contributor

@mwillbanks mwillbanks left a comment

Choose a reason for hiding this comment

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

Please add in unit tests for this new behavior.

@ConorSheehan1
Copy link
Author

@mwillbanks I'm having some trouble creating a mock connection to a database. I've pushed up my attempt in the last commit. Sorry, it's a bit messy. I tried following the Pdo test setup as an example. Could you give me some suggestions on how to create an appropriate mock or stub for the db connection?

@ezimuel ezimuel added this to the 3.0.0 milestone Nov 28, 2017
@weierophinney
Copy link
Member

This repository has been moved to laminas/laminas-db. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-db to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-db.
  • In your clone of laminas/laminas-db, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

@michalbundyra
Copy link
Member

This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at laminas/laminas-db#64.

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.

6 participants