Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

DebugPDOStatement on execute() #770

Merged
merged 1 commit into from

5 participants

@jmglsn

If using FirePHP to view executed statements, I got warnings regarding missing array index, but only if parameters are passed through $statement->execute().

This happens because those parameters are not kept in DebugPDOStatement::execute(). So they won't be available in getExecutedQueryString() (boundValues).

@staabm
Collaborator

this may not work in case you execute the statement multiple times with paramters given to execute?

@jmglsn

Yes, that's right. But it prevents a notice about wrong index. Another fix would be to check for index at getExecutedQueryString().

$sql = str_replace($pos, (isset($this->boundValues[$pos]) ? $this->boundValues[$pos] : ''), $sql);

I'm currently only looking for a fast patch to keep it running without a php notice.

@jmglsn

This should now work as expected, even for multiple calls.

@thedave80

I can confirm that this works with current FirePHP implementation.

runtime/lib/connection/DebugPDOStatement.php
((10 lines not shown))
{
$sql = $this->queryString;
+ $boundValues = $this->boundValues + $values;
@willdurand Owner

array_merge rather than + would be nice

@jmglsn
jmglsn added a note

I'll check this comment http://www.php.net/manual/de/pdostatement.execute.php#97531 first, if the behavior is still the same we don't need any merge.

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

Removed parameter merge, PDOStatement don't support this.

@willdurand
Owner

Looks good to me, anyone else?

@staabm
Collaborator

so, when someone uses parameter with execute($params) you need to store those parameters somewhere to get the proper query string using getExecutedQueryString?

this is something which would be imported to note?

@jmglsn

Shall I add it as a comment to getExecutedQueryString()?

@jaugustin jaugustin commented on the diff
runtime/lib/connection/DebugPDOStatement.php
((6 lines not shown))
* @return string
*/
- public function getExecutedQueryString()
+ public function getExecutedQueryString(array $values = array())
@jaugustin Collaborator

forcing an array could be a BC break ?

@staabm Collaborator
staabm added a note

hm before there was not parameter and this one now is optional?

@willdurand Owner

no potential BC break here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
runtime/lib/connection/DebugPDOStatement.php
@@ -61,17 +61,20 @@ protected function __construct(PropelPDO $pdo)
}
/**
+ * @param array $values parameters passed without using bind*
@staabm Collaborator
staabm added a note

I would make this more clear: parameters which were passed to execute(), if any.

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

Shall I add it as a comment to getExecutedQueryString()?

yes I think there would be a good spot to describe it.

@staabm
Collaborator

Lgtm :+1:

@willdurand
Owner

@jmglsn please squash your commits.

@jmglsn

Hope that was ok, never squashed already pushed stuff before.

@willdurand willdurand merged commit 4dde6da into from
@willdurand
Owner

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 12, 2013
  1. @jmglsn
This page is out of date. Refresh to see the latest.
Showing with 6 additions and 3 deletions.
  1. +6 −3 runtime/lib/connection/DebugPDOStatement.php
View
9 runtime/lib/connection/DebugPDOStatement.php
@@ -61,17 +61,20 @@ protected function __construct(PropelPDO $pdo)
}
/**
+ * @param array $values Parameters which were passed to execute(), if any. Default: bound parameters.
+ *
* @return string
*/
- public function getExecutedQueryString()
+ public function getExecutedQueryString(array $values = array())
@jaugustin Collaborator

forcing an array could be a BC break ?

@staabm Collaborator
staabm added a note

hm before there was not parameter and this one now is optional?

@willdurand Owner

no potential BC break here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
{
$sql = $this->queryString;
+ $boundValues = empty($values) ? $this->boundValues : $values;
$matches = array();
if (preg_match_all('/(:p[0-9]+\b)/', $sql, $matches)) {
$size = count($matches[1]);
for ($i = $size - 1; $i >= 0; $i--) {
$pos = $matches[1][$i];
- $sql = str_replace($pos, $this->boundValues[$pos], $sql);
+ $sql = str_replace($pos, $boundValues[$pos], $sql);
}
}
@@ -91,7 +94,7 @@ public function execute($input_parameters = null)
$debug = $this->pdo->getDebugSnapshot();
$return = parent::execute($input_parameters);
- $sql = $this->getExecutedQueryString();
+ $sql = $this->getExecutedQueryString($input_parameters?:array());
$this->pdo->log($sql, null, __METHOD__, $debug);
$this->pdo->setLastExecutedQuery($sql);
$this->pdo->incrementQueryCount();
Something went wrong with that request. Please try again.