-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[8.x] Add method to dump the SQL query replacing all bindings #36918
[8.x] Add method to dump the SQL query replacing all bindings #36918
Conversation
if (is_object($i)) { | ||
$i = (string) $i; |
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.
This will throw a fatal if the object in question does not implement __toString(), is that desired or should it be catched and turned into a logic exception?
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 tested it with SQLite, SQL Server, MySQL and PostgreSQL, and the tests still passing (i'll add the tests as a separate commit and we can drop if later if it's unnecesssary). Can you provide an example where you think the object does not implement __toString()
?
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.
Sure:
User::query()->where('updated_at', '>', new DateTime('yesterday'))->get();
Fatal error: Uncaught Error: Object of class DateTime could not be converted to 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.
Just to clarify:
The query from the first snippet above works on a stock Laravel installation.
The link was to show the \DateTime
class fails to be cast to a string.
I would look into previous attempts to see why they got rejected
Also you need to account for:
Another suggestion is to look how these packages handle this:
They all have some sort of full query display. Good luck |
This PR won't be reviewed until it's out of draft. |
…String() or toString()
'?', | ||
collect($this->connection->prepareBindings($this->getBindings())) | ||
->map(function ($i) { | ||
return (is_string($i)) ? "'{$i}'" : $i; |
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 guess it would be safer to call some existing functionality to quote query parameters, otherwise this would not work when one of the bound strings contains a single quote.
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.
@thiagorb I think PDO quote
method solve this problem. Can you check it again?
…n bound strings contains single quotes
@@ -2275,7 +2275,7 @@ public function toSqlWithBindings() | |||
'?', | |||
collect($this->connection->prepareBindings($this->getBindings())) | |||
->map(function ($i) { | |||
return (is_string($i)) ? "'{$i}'" : $i; | |||
return (is_string($i)) ? $this->connection->getPdo()->quote($i) : $i; |
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.
$this->connection
is type-hinted as a Illuminate\Database\ConnectionInterface
, which does not have a getPdo()
method.
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.
@rodrigopedra Do you think something like this would be better?
$search = ["\\", "\x00", "\n", "\r", "'", '"', "\x1a"];
$replace = ["\\\\","\\0","\\n", "\\r", "\'", '\"', "\\Z"];
return "'" . str_replace($search, $replace, $value) . "'";
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 think a combination of both, similar to what debugbar does:
Although I would use method_exists
instead of try...catch
to check for PDO
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.
@rodrigopedra done.
Thanks for your pull request to Laravel! Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include. If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions! If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response. |
I suggest checking out tools like Telescope and DebugBar. |
About this PR
This PR adds a new
toSqlWithBindings
method to the Query Builder class.It allows developers to dump the query with all binding replaced with it actual value, for example:
With the actual
->toSql()
method, it returns this:Of course you can use the
->dd()
method, and it returns your bindings as an array, which is useful but it kinda take some time to figure out the place for each binding within your query if you have a query with too many bindings:"select * from `posts` where `title` like ?"
Using the new proposed
->toSqlWithBindings()
method, you can easily see your query with all bindings:Questions
I don't know if my tests were added to the correct class. If not, tell me what class they should be in and I make the correction.