Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[DDC-2224] convertToDatabaseValueSQL() is not honored for DQL query parameters #574

Closed
wants to merge 2 commits into
from
Jump to file or symbol
Failed to load files and symbols.
+84 −1
Split
@@ -179,7 +179,7 @@ public function getParameters()
*
* @param mixed $key The key (index or name) of the bound parameter.
*
- * @return mixed The value of the bound parameter.
+ * @return Query\Parameter|null The value of the bound parameter.
*/
public function getParameter($key)
{
@@ -2072,6 +2072,13 @@ public function walkInputParameter($inputParam)
{
$this->parserResult->addParameterMapping($inputParam->name, $this->sqlParamIndex++);
+ $parameter = $this->query->getParameter($inputParam->name);
+
+ if ($parameter && Type::hasType($parameter->getType())) {
@BenMorel

BenMorel Feb 8, 2013

Contributor

I would advocate to remove the hasType() test as well.
IMO, failing silently in the case of an unknown type is a bad idea!

@BenMorel

BenMorel Feb 9, 2013

Contributor

@Ocramius I can't see why, can you explain?

@Ocramius

Ocramius Feb 9, 2013

Owner

There are no db types 101 and 102

@BenMorel

BenMorel Feb 9, 2013

Contributor

Ok, you're right! I had not realized that you could use non-Type types in the ORM as well (obviously....) so hasType() is necessary.
However, it would be good to avoid failing silently if we pass an invalid type string. Maybe the following code would solve the problem:

if (is_string($parameter)) {
    $parameterType = Type::getType($parameter->getType());

That would filter out all PDO_PARAM_* and Connection::PARAM_* integer values, as well as the NULL value when the parameter does not exist.

By the way, should we accept getParameter() to be NULL as well?

@mnapoli

mnapoli Feb 10, 2013

Contributor

@BenMorel: Well AbstractQuery::getParameter() can return null:

    public function getParameter($key)
    {
        // ...
        return count($filteredParameters) ? $filteredParameters->first() : null;
    }
@BenMorel

BenMorel Feb 10, 2013

Contributor

@mnapoli Yes I know it can return null, I'm just wondering how it could be null in this very situation, and thus if we should allow it or throw an exception if it's null?

@BenMorel

BenMorel Feb 13, 2013

Contributor

I've double-checked the code, and it can only return null if the parameter name does not exist for this query, which is an error IMO. So I'd change the code for:

$parameter = $this->query->getParameter($inputParam->name);

if (! $parameter) {
    throw QueryException::unknownParameter($inputParam->name);
}

if (is_string($parameter->getType())) {
    $parameterType = Type::getType($parameter->getType());
    return $parameterType->convertToDatabaseValueSQL('?', $this->platform);
}

This code has an extra consequence then:
If your DQL contains a parameter such as :name, and you don't call ->setParameter('name', ...), it will throw a QueryException. Right now, this is not the case. @beberlei , could you please comment on this?

@beberlei

beberlei Apr 1, 2013

Owner

@BenMorel this is fine, just moving from PDOException to QueryException here

@BenMorel

BenMorel Apr 2, 2013

Contributor

@beberlei Should @mnapoli update his PR with the code above?

@beberlei

beberlei Apr 4, 2013

Owner

yes your code is better for this, i like it very much.

@mnapoli

mnapoli Apr 4, 2013

Contributor

@BenMorel, @beberlei Noted, I'll update the PR

+ $parameterType = Type::getType($parameter->getType());
+ return $parameterType->convertToDatabaseValueSQL('?', $this->platform);
+ }
+
return '?';
}
@@ -0,0 +1,76 @@
+<?php
+
+namespace Doctrine\Tests\ORM\Functional\Ticket;
+
+use Doctrine\DBAL\Types\Type;
+use Doctrine\DBAL\Platforms\AbstractPlatform;
+
+/**
+ * @group DDC-2224
+ */
+class DDC2224Test extends \Doctrine\Tests\OrmFunctionalTestCase
+{
+ public static function setUpBeforeClass()
+ {
+ parent::setUpBeforeClass();
+
+ \Doctrine\DBAL\Types\Type::addType('DDC2224Type', __NAMESPACE__ . '\DDC2224Type');
+ }
+
+ public function testIssue()
+ {
+ $dql = 'SELECT e FROM ' . __NAMESPACE__ . '\DDC2224Entity e WHERE e.field = :field';
+ $query = $this->_em->createQuery($dql);
+ $query->setParameter('field', 'test', 'DDC2224Type');
+
+ $this->assertStringEndsWith('.field = FUNCTION(?)', $query->getSQL());
+ }
+}
+
+class DDC2224Type extends Type
+{
+ /**
+ * {@inheritdoc}
+ */
+ public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $platform)
+ {
+ return $platform->getVarcharTypeDeclarationSQL($fieldDeclaration);
+ }
+
+ public function getName()
+ {
+ return 'DDC2224Type';
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function canRequireSQLConversion()
+ {
+ return true;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function convertToDatabaseValueSQL($sqlExpr, AbstractPlatform $platform)
+ {
+ return sprintf('FUNCTION(%s)', $sqlExpr);
+ }
+}
+
+/**
+ * @Entity
+ */
+class DDC2224Entity
+{
+ /**
+ * @Id @GeneratedValue @Column(type="integer")
+ */
+ public $id;
+
+ /**
+ * @Column(type="DDC2224Type")
+ */
+ public $field;
+}