Skip to content

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

Closed
wants to merge 2 commits into from

5 participants

@mnapoli
mnapoli commented Feb 8, 2013

DDC-2224: convertToDatabaseValueSQL() is not honored for DQL query parameters

Fix + test

Please note this is the first time I get into SqlWalker, I hope I inserted the fix at the right place.

@doctrinebot

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DDC-2240

@Ocramius Ocramius and 1 other commented on an outdated diff Feb 8, 2013
lib/Doctrine/ORM/Query/SqlWalker.php
@@ -2072,6 +2072,12 @@ public function walkInputParameter($inputParam)
{
$this->parserResult->addParameterMapping($inputParam->name, $this->sqlParamIndex++);
+ $parameter = $this->query->getParameter($inputParam->name);
+ if ($parameter && Type::hasType($parameter->getType())) {
@Ocramius
Doctrine member
Ocramius added a note Feb 8, 2013

Missing newline before this line

@Ocramius
Doctrine member
Ocramius added a note Feb 8, 2013

Also: this would fail on unknown types anyway. Is the check for Type::hasType necessary?

@mnapoli
mnapoli added a note Feb 8, 2013

hasType does isset(self::$_typesMap[$name]) whereas getType throw DBALException::unknownColumnType($name) if it's not found, so I guess the check is necessary and it will not fail if the type is not found.

@Ocramius
Doctrine member
Ocramius added a note Feb 8, 2013

My point is that a failure on not found type is acceptable, but binding allows custom parameter type anyway... guess you're right :)

@mnapoli
mnapoli added a note Feb 8, 2013

OK I get it. Yes it's possible a user specify a wrong type in setParameter($key, $value, $type), in that case is it better to silently ignore the type given (actual) or have an error of type not found (if the check was removed)?

I'd go for the second actually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@BenMorel
BenMorel commented Feb 8, 2013

Works like a charm for me, thanks!

@BenMorel BenMorel commented on the diff Feb 8, 2013
lib/Doctrine/ORM/Query/SqlWalker.php
@@ -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 added a note Feb 8, 2013

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 added a note Feb 9, 2013

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

@Ocramius
Doctrine member
Ocramius added a note Feb 9, 2013

There are no db types 101 and 102

@BenMorel
BenMorel added a note Feb 9, 2013

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 added a note Feb 10, 2013

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

    public function getParameter($key)
    {
        // ...
        return count($filteredParameters) ? $filteredParameters->first() : null;
    }
@BenMorel
BenMorel added a note Feb 10, 2013

@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 added a note Feb 13, 2013

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
Doctrine member
beberlei added a note Apr 1, 2013

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

@BenMorel
BenMorel added a note Apr 2, 2013

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

@beberlei
Doctrine member
beberlei added a note Apr 4, 2013

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

@mnapoli
mnapoli added a note Apr 4, 2013

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

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

The problem with this change is, that its breaking backwards compatibility, currently the following code is valid for a "datetime" field:

$query->setParameter(1, "2012-01-01");

In the new approach it would lead to a fatal error, because the datetime Type expects a DateTime instance.

I am wondering if the check should only transform if the value is an object.

@mnapoli
mnapoli commented Feb 13, 2013

@beberlei Don't you mean this?

$query->setParameter(1, "2012-01-01", "datetime");

If the parameter type is not set (like in the example you gave), then the parameter type inferred will be "string" (cf. ParameterTypeInferer::inferType). So it shouldn't be a problem right?

@BenMorel

@beberlei Yes, with the example you gave, the type will actually be PDO::PARAM_STR, so will be filtered out by the code above.

@beberlei
Doctrine member
beberlei commented Apr 1, 2013

@BenMorel @mnapoli not sure what my head was thinking here, this should work.

@beberlei
Doctrine member
beberlei commented Apr 4, 2013

Edit: Wrong not merged into master.

There is a problem with this patch and caching, which makes it unmergable:

Passing a type into the parameters is not recognized during caching, that means, using a DQL cache, the same DQL statement with:

$query->setParameter(1, "foo", "sometype"); // or
$query->setParameter(1, "foo");

will lead to the same SQL being generated, depending on if the first or the second set parameter query is executed first.

@beberlei
Doctrine member
beberlei commented Apr 4, 2013

The conversion here has to be done in side the Query object, not inside SQLWalker

@beberlei
Doctrine member
beberlei commented Apr 4, 2013

Just verified in the code, there is no way to fix this issue, you need to create a custom DQL function to allow this converting to work.

@beberlei beberlei closed this Apr 4, 2013
@BenMorel
BenMorel commented Apr 4, 2013

@beberlei So we're just abandoning like that..?

@beberlei
Doctrine member
beberlei commented Apr 4, 2013

@BenMorel you can convert the values before putting them to setParameter(). You have the right context information to do so.

@mnapoli
mnapoli commented Apr 4, 2013

+1 I really see that as a bug because it's really not the expected behavior for the end-user.

Juste to be clear I'm not complaining about the fact that there's a bug, but the fact that it's closed as "Invalid"

@BenMorel
BenMorel commented Apr 4, 2013

@beberlei It's not that easy, sometimes you rely on SQL functions such as geometry functions that are hard (and useless) to implement in PHP, and as @mnapoli pointed out, the current behaviour is not the one expected by the user of the ORM (the type behaves differently when persisting an entity as opposed to when querying entities with DQL).

I must support the fact that it's a bit sad to close this is as Invalid, instead of leaving it open for further thought.

@beberlei
Doctrine member
beberlei commented Apr 4, 2013

The assumptions in this PR are completely wrong, how can i leave it open for thought?

While looking at this issue i realized why it wasnt this way beforehand and this is because the context is unknown here for the SqlWalker, there is knowing if we want to convet or not.

Additionally to setParameter() yoiu can also use a DQL function to do the conversion:

SELECT m FROM Foo m WHERE m.id = CONVERT(?)
@BenMorel
BenMorel commented Apr 4, 2013

I was talking about the issue, not this PR in particular (I agree this wasn't the best place to talk about this, then).

Because it's not feasible with how the ORM is currently architectured doesn't mean it's impossible, does it? My point is, leaving it open reminds us that this is an issue, and if someone rethinks the architecture in the future, he'll have that in mind as well to see if his new ideas fit all the needs (I know that there are other missing features in Doctrine such as value objects, and I suspect that these might bring similar issues).

@mnapoli
mnapoli commented Apr 5, 2013

I was talking about the issue, not this PR in particular (I agree this wasn't the best place to talk about this, then).

same for me, no problem with rejecting the PR (I didn't want to pollute too much the Jira issue with meta-discussions)

@beberlei
Doctrine member
beberlei commented Apr 5, 2013

I have a solution that could help. We could add a generic DQL function CONVERT(expression, type), so that you can do for example:

SELECT m FROM Foo m WHERE m.value = CONVERT(?, "datetime")

This would then wrap around expression the convertToDatabaseValueSQL() result.

Would that help you two?

@BenMorel
BenMorel commented Apr 5, 2013

TBH, I have already changed my code to not rely on this functionality anymore (my type doesn't need convertToDatabaseValueSQL() anymore), so I'd personally leave it as it is, until the next big rewrite of Doctrine (if any). Again, to me, the only "correct" solution is to have the SQL function automatically inserted.
But maybe @mnapoli is interested by this solution!

@mnapoli
mnapoli commented Apr 5, 2013

I'm not sure that would help much too, the main problem I have is that it's not symmetrical between entity insertion and querying, so that wouldn't help with that if I understand correctly.

@mnapoli mnapoli deleted the myclabs:DDC-2224 branch Nov 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.