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

[mnapoli]

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]

Missing newline before this line

[Ocramius]

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

[mnapoli]

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]

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

[mnapoli]

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.

[BenMorel]

Works like a charm for me, thanks!

[BenMorel]

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

[Ocramius]

@BenMorel Not sure if it works with https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Connection.php#L69-L88

[BenMorel]

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

[Ocramius]

There are no db types 101 and 102

[BenMorel]

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?

[BenMorel]

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]

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

[BenMorel]

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

[beberlei]

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

[mnapoli]

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

[beberlei]

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]

@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]

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

[beberlei]

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]

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

[beberlei]

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.

[BenMorel]

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

[beberlei]

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

[mnapoli]

+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]

@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]

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]

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]

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]

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]

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]

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.