Skip to content
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

Parameter parsing fixes #309

Merged
merged 1 commit into from
May 9, 2013
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 42 additions & 10 deletions lib/Doctrine/DBAL/SQLParserUtils.php
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ static public function getPlaceholderPositions($statement, $isPositional = true)
* @param string $query The SQL query to execute.
* @param array $params The parameters to bind to the query.
* @param array $types The types the previous parameters are in.
*
*
* @throws SQLParserUtilsException
* @return array
*/
static public function expandListParameters($query, $params, $types)
Expand All @@ -103,7 +104,7 @@ static public function expandListParameters($query, $params, $types)
$arrayPositions[$name] = false;
}

if (( ! $arrayPositions && $isPositional) || (count($params) != count($types))) {
if (( ! $arrayPositions && $isPositional)) {
return array($query, $params, $types);
}

Expand All @@ -130,9 +131,9 @@ static public function expandListParameters($query, $params, $types)

$types = array_merge(
array_slice($types, 0, $needle),
$count ?
$count ?
array_fill(0, $count, $types[$needle] - Connection::ARRAY_PARAM_OFFSET) : // array needles are at PDO::PARAM_* + 100
array(),
array(),
array_slice($types, $needle + 1)
);

Expand All @@ -152,16 +153,16 @@ static public function expandListParameters($query, $params, $types)
$paramsOrd = array();

foreach ($paramPos as $pos => $paramName) {
$paramLen = strlen($paramName) + 1;
$value = $params[$paramName];
$paramLen = strlen($paramName) + 1;
$value = static::extractParam($paramName, $params, true);

if ( ! isset($arrayPositions[$paramName])) {
if ( ! isset($arrayPositions[$paramName]) && ! isset($arrayPositions[':' . $paramName])) {
$pos += $queryOffset;
$queryOffset -= ($paramLen - 1);
$paramsOrd[] = $value;
$typesOrd[] = $types[$paramName];
$typesOrd[] = static::extractParam($paramName, $types, false, \PDO::PARAM_STR);
$query = substr($query, 0, $pos) . '?' . substr($query, ($pos + $paramLen));

continue;
}

Expand All @@ -170,7 +171,7 @@ static public function expandListParameters($query, $params, $types)

foreach ($value as $val) {
$paramsOrd[] = $val;
$typesOrd[] = $types[$paramName] - Connection::ARRAY_PARAM_OFFSET;
$typesOrd[] = static::extractParam($paramName, $types, false) - Connection::ARRAY_PARAM_OFFSET;
}

$pos += $queryOffset;
Expand Down Expand Up @@ -199,4 +200,35 @@ static private function getUnquotedStatementFragments($statement)

return $fragments[1];
}

/**
* @param string $paramName The name of the parameter (without a colon in front)
* @param array $paramsOrTypes A hash of parameters or types
* @param bool $isParam
* @param mixed $defaultValue An optional default value. If omitted, an exception is thrown
*
* @throws SQLParserUtilsException
* @return mixed
*/
static private function extractParam($paramName, $paramsOrTypes, $isParam, $defaultValue = null)
Copy link
Member

Choose a reason for hiding this comment

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

Could you break it into extractParam and extractType ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought about that as well but it isn't really worth introducing another redirection (another method) or duplicating the extraction code.

Copy link
Member

Choose a reason for hiding this comment

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

I'd vote for using two methods here.
This one breaks the single responsibility principle.

{
if (isset($paramsOrTypes[$paramName])) {
return $paramsOrTypes[$paramName];
}

// Hash keys can be prefixed with a colon for compatibility
if (isset($paramsOrTypes[':' . $paramName])) {
Copy link
Member

Choose a reason for hiding this comment

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

wait, so PDO supports $stmt->bindParam(':foo', $val); instead of $stmt->bindParam('foo', $val);

Copy link
Member

Choose a reason for hiding this comment

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

yes .. it supports both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that was the root cause of the compatibility break with the Jackalope DBAL component.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't know that, hence that code was never tested. :)

return $paramsOrTypes[':' . $paramName];
}

if (null !== $defaultValue) {
return $defaultValue;
}

if ($isParam) {
throw SQLParserUtilsException::missingParam($paramName);
} else {
Copy link
Member

Choose a reason for hiding this comment

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

no need for else as the if throws an exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mutual exclusiveness is better communicated that way. That's why there is an else.

Copy link
Member

Choose a reason for hiding this comment

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

but it does not follow our coding standards

Copy link
Member

Choose a reason for hiding this comment

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

throw SQLParserUtilsException::missingType($paramName);
}
}
}
43 changes: 43 additions & 0 deletions lib/Doctrine/DBAL/SQLParserUtilsException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<?php
/*
* $Id: $
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*
* This software consists of voluntary contributions made by many individuals
* and is licensed under the MIT license. For more information, see
* <http://www.doctrine-project.org>.
*/

namespace Doctrine\DBAL;

/**
* Doctrine\DBAL\ConnectionException
*
* @license http://www.opensource.org/licenses/lgpl-license.php LGPL
* @link www.doctrine-project.org
* @since 2.4
* @author Lars Strojny <lars@strojny.net>
*/
class SQLParserUtilsException extends DBALException
{
public static function missingParam($paramName)
{
return new self(sprintf('Value for :%1$s not found in params array. Params array key should be "%1$s"', $paramName));
}

public static function missingType($typeName)
{
return new self(sprintf('Value for :%1$s not found in types array. Types array key should be "%1$s"', $typeName));
}
}
98 changes: 98 additions & 0 deletions tests/Doctrine/Tests/DBAL/SQLParserUtilsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,55 @@ static public function dataExpandListParameters()
array(),
array()
),
array(
"SELECT * FROM Foo WHERE foo IN (:foo) OR bar = :bar OR baz = :baz",
array('foo' => array(1, 2), 'bar' => 'bar', 'baz' => 'baz'),
array('foo' => Connection::PARAM_INT_ARRAY, 'baz' => 'string'),
'SELECT * FROM Foo WHERE foo IN (?, ?) OR bar = ? OR baz = ?',
array(1, 2, 'bar', 'baz'),
array(\PDO::PARAM_INT, \PDO::PARAM_INT, \PDO::PARAM_STR, 'string')
),
array(
"SELECT * FROM Foo WHERE foo IN (:foo) OR bar = :bar",
array('foo' => array(1, 2), 'bar' => 'bar'),
array('foo' => Connection::PARAM_INT_ARRAY),
'SELECT * FROM Foo WHERE foo IN (?, ?) OR bar = ?',
array(1, 2, 'bar'),
array(\PDO::PARAM_INT, \PDO::PARAM_INT, \PDO::PARAM_STR)
),
// Params/types with colons
array(
"SELECT * FROM Foo WHERE foo = :foo OR bar = :bar",
array(':foo' => 'foo', ':bar' => 'bar'),
array(':foo' => \PDO::PARAM_INT),
'SELECT * FROM Foo WHERE foo = ? OR bar = ?',
array('foo', 'bar'),
array(\PDO::PARAM_INT, \PDO::PARAM_STR)
),
array(
"SELECT * FROM Foo WHERE foo = :foo OR bar = :bar",
array(':foo' => 'foo', ':bar' => 'bar'),
array(':foo' => \PDO::PARAM_INT, 'bar' => \PDO::PARAM_INT),
'SELECT * FROM Foo WHERE foo = ? OR bar = ?',
array('foo', 'bar'),
array(\PDO::PARAM_INT, \PDO::PARAM_INT)
),
array(
"SELECT * FROM Foo WHERE foo IN (:foo) OR bar = :bar",
array(':foo' => array(1, 2), ':bar' => 'bar'),
array('foo' => Connection::PARAM_INT_ARRAY),
'SELECT * FROM Foo WHERE foo IN (?, ?) OR bar = ?',
array(1, 2, 'bar'),
array(\PDO::PARAM_INT, \PDO::PARAM_INT, \PDO::PARAM_STR)
),
array(
"SELECT * FROM Foo WHERE foo IN (:foo) OR bar = :bar",
array('foo' => array(1, 2), 'bar' => 'bar'),
array(':foo' => Connection::PARAM_INT_ARRAY),
'SELECT * FROM Foo WHERE foo IN (?, ?) OR bar = ?',
array(1, 2, 'bar'),
array(\PDO::PARAM_INT, \PDO::PARAM_INT, \PDO::PARAM_STR)
),
);
}

Expand All @@ -257,4 +306,53 @@ public function testExpandListParameters($q, $p, $t, $expectedQuery, $expectedPa
$this->assertEquals($expectedParams, $params, "Params dont match");
$this->assertEquals($expectedTypes, $types, "Types dont match");
}

public static function dataQueryWithMissingParameters()
{
return array(
array(
"SELECT * FROM foo WHERE bar = :param",
array('other' => 'val'),
array(),
),
array(
"SELECT * FROM foo WHERE bar = :param",
array(),
array(),
),
array(
"SELECT * FROM foo WHERE bar = :param",
array(),
array('param' => Connection::PARAM_INT_ARRAY),
),
array(
"SELECT * FROM foo WHERE bar = :param",
array(),
array(':param' => Connection::PARAM_INT_ARRAY),
),
array(
"SELECT * FROM foo WHERE bar = :param",
array(),
array('bar' => Connection::PARAM_INT_ARRAY),
),
array(
"SELECT * FROM foo WHERE bar = :param",
array('bar' => 'value'),
array('bar' => Connection::PARAM_INT_ARRAY),
),
);
}

/**
* @dataProvider dataQueryWithMissingParameters
*/
public function testExceptionIsThrownForMissingParam($query, $params, $types = array())
{
$this->setExpectedException(
'Doctrine\DBAL\SQLParserUtilsException',
'Value for :param not found in params array. Params array key should be "param"'
);

SQLParserUtils::expandListParameters($query, $params, $types);
}
}