Insufficient autodetection of query parameter type

[enumag]

We're using a custom UtcDateTimeType. Recently we found a bug in our application that queries with a condition using such column do not return correct results.

It can be easily fixed by specifying the parameter type in the query:

// Buggy
$qb
    ->andWhere('invoice.date <= :today')
    ->setParameter('today', new \DateTime())

// Fixed
$qb
    ->andWhere('invoice.date <= :today')
    ->setParameter('today', new \DateTime(), 'datetime_utc')

I don't like this solution because programmer can easily forget about this or other members of the team may not be aware of the issue. So I looked into Doctrine internals to check why the type is not autodetected from the metadata as I would expect.

The problem is in Query::processParameterMappings(). Instead of getting the parameter type from metadata the type is guessed by static ParameterTypeInferer which of course only considers built-in Doctrine types.

Is it possible to improve Doctrine to guess the parameter type correctly based on which column is the parameter used for? If not can you make the ParameterTypeInferer non-static and replaceable?

[sensorario]

Sorry the question, but should this fix your issue?

new DateTime('now', new DateTimeZone("UTC"));

[enumag]

No, our system uses UTC as the default timezone so this will not change anything.

Actually that was a mistake. The default timezone of our system can be different than UTC which is when the bug occurs. $date->setTimezone(new DateTimeZone("UTC")); will actually fix the issue. It is far from ideal though becuase the setTimezone can be easily forgotten and can lead to more errors because the DateTime object is changed (I wish I could use DateTimeImmutable).

[Ocramius]

Inferring types is actually very risky, so I'd stop going down that way.

Assuming you have a dozen of different types that map to DateTime instances: what should they do?

A good alternative could be to enforce the type parameter to be given at all times (BC break)

[sensorario]

@Ocramius are you suggesting a sort of ->set[typeA|typeB|...]Parameter()?

[Ocramius]

@sensorario no, setParameter(string $key, $parameter, Type $type)

[m-radzikowski]

Setting parameter type in every use of setParameter(), if I understood correctly, for every value? Well, it would be annoying, as in 99% you use standard types.

But we also use own type and I was suprised that I have to set it's type by hand. We use own type for our own class, so there is no issue with multiple types for the same class, like in @enumag case.

Wouldn't be better if in Type implementation we could set binding class and then ParameterTypeInferer would check if class of given $value has registered Type? And if class would have multiple Type classes assigned, like @enumag \DateTime, exception could be thrown.

[Ocramius]

That would mean O(n) on every call to setParameter(), where n is the number of registered types.

Also, the order of registered types would be authoritative.

IMO, that is both a performance and a security nightmare.

[m-radzikowski]

Well, not necessary O(n), as mapped classes could be stored (by name from ::class) in assoc array with types they use. As PHP uses hash tables, I believe it would not iterate over all registered types. Also, as this works only for objects, it could be the last step in inferType() method, if any of the build-in types could not be found.

Authoritative order - no, if we assume that build-in types have priority (you have to set type directly as it is now to override it) and then if more then one type maps the same class - throw exception. It will work without providing type by hand in most cases.

Security - well, I will not speak in this matter. If you say so, ok. But with priority of build-in Doctrine types our new "magic" code will work only when you put custom object as parameter, so you should be aware of Type detection. And with exception if more then one Type is given for class noone will override your mapping I think. But again, there may be more on this subject so I'm leaving it under your consideration.

[Ocramius]

Well, not necessary O(n), as mapped classes could be stored (by name from ::class) in assoc array with types they use. As PHP uses hash tables, I believe it would not iterate over all registered types. Also, as this works only for objects, it could be the last step in inferType() method, if any of the build-in types could not be found.

It's a slippery slope IMO, as the opinionated approach of className <-> type entry is very restrictive, and isn't really compatible with types producing a single class instance on conversion.
Another example is the opposite: DateTime may be a date, a time, a datetime, a datetime + timezone. I think that we always had it mapped to datetime, but there's always a risk, and I had previous software project where incorrect parameter binding caused wrong date range filtering (security issue, since $$$ stealing was involved).

[ammont]

I do agree with @enumag , When you're working on a multi-international project it's very important to store all the dates in UTC, doctrine handles this well by defining a new type, but it would make no sense that it doesn't use the same type when querying the same Entity.