-
-
Notifications
You must be signed in to change notification settings - Fork 21
PgSql now shows text of failed query in Exception #26
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.
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
Conversation
src/PgSqlHandle.php
Outdated
| * @throws QueryError | ||
| */ | ||
| private function createResult($result) | ||
| private function createResult($result, &$sql) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for making this a reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just want to save memory by preventing large SQL string copy.
| foreach (self::DIAGNOSTIC_CODES as $fieldCode => $desciption) { | ||
| $diagnostics[$desciption] = \pg_result_error_field($result, $fieldCode); | ||
| } | ||
| $diagnostics['sql'] = $sql; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for adding this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$diagnostics will be used in QueryExecutionError for error message. 'sql' key will contain failed query
| public function __construct(string $message, array $diagnostics, \Throwable $previous = null) | ||
| { | ||
| parent::__construct($message, 0, $previous); | ||
| parent::__construct($message, $diagnostics['sql'], $previous); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A string should not be used for the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? parent constructor have exactly string parameter at this place? Any ideas how to pass this without string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it does, I didn't realize QueryError had a custom constructor taking the SQL query. Not providing it to QueryExecutionError was certainly an oversight before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should I do?
| { | ||
| return call(function () use ($name, $params) { | ||
| return $this->createResult(yield from $this->send("pg_send_execute", $name, $params)); | ||
| return $this->createResult(yield from $this->send("pg_send_execute", $name, $params), $name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$name is not the query SQL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought name of statement may be more usefull than nothing. I can remove it
All PgSQL queries didn't show full SQL ext of failed query.
This PR add functionality to show failed query text instead of "Current query was 0"