Insufficient autodetection of query parameter type #5536

New Issue

admin · 2026-01-22T15:10:21+01:00

admin commented

2026-01-22 15:10:21 +01:00

Originally created by @enumag on GitHub (May 15, 2017).

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?

Originally created by @enumag on GitHub (May 15, 2017). We're using a custom [UtcDateTimeType](https://gist.github.com/enumag/eb933758473bb8fc6af0e204b58ae3e2). 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: ```php // 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()](https://github.com/doctrine/doctrine2/blob/e9b54de488accfb3c08cc8efe629722099f2c507/lib/Doctrine/ORM/Query.php#L403-L405). Instead of getting the parameter type from metadata the type is guessed by static [ParameterTypeInferer](https://github.com/doctrine/doctrine2/blob/e9b54de488accfb3c08cc8efe629722099f2c507/lib/Doctrine/ORM/Query/ParameterTypeInferer.php#L56) 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?

admin commented

2026-01-22 15:10:23 +01:00

@sensorario commented on GitHub (May 15, 2017):

Sorry the question, but should this fix your issue?

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

@sensorario commented on GitHub (May 15, 2017): Sorry the question, but should this fix your issue? new DateTime('now', new DateTimeZone("UTC"));

admin commented

2026-01-22 15:10:23 +01:00

@enumag commented on GitHub (May 15, 2017):

~~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).

@enumag commented on GitHub (May 15, 2017): ~~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).

admin commented

2026-01-22 15:10:25 +01:00

@Ocramius commented on GitHub (May 16, 2017):

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)

@Ocramius commented on GitHub (May 16, 2017): 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)

admin commented

2026-01-22 15:10:25 +01:00

@sensorario commented on GitHub (May 16, 2017):

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

@sensorario commented on GitHub (May 16, 2017): @Ocramius are you suggesting a sort of ->set[typeA|typeB|...]Parameter()?

admin commented

2026-01-22 15:10:25 +01:00

@Ocramius commented on GitHub (May 16, 2017):

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

@Ocramius commented on GitHub (May 16, 2017): @sensorario no, `setParameter(string $key, $parameter, Type $type)`

admin commented

2026-01-22 15:10:25 +01:00

@m-radzikowski commented on GitHub (May 29, 2017):

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.

@m-radzikowski commented on GitHub (May 29, 2017): 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.

admin commented

2026-01-22 15:10:27 +01:00

@Ocramius commented on GitHub (May 29, 2017):

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.

@Ocramius commented on GitHub (May 29, 2017): 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.

admin commented

2026-01-22 15:10:27 +01:00

@m-radzikowski commented on GitHub (May 29, 2017):

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.

@m-radzikowski commented on GitHub (May 29, 2017): Well, not necessary `O(n)`, as mapped classes could be stored ([by name from ::class](http://php.net/manual/en/language.oop5.constants.php)) 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.

admin commented

2026-01-22 15:10:27 +01:00

@Ocramius commented on GitHub (May 29, 2017):

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).

@Ocramius commented on GitHub (May 29, 2017): > Well, not necessary `O(n)`, as mapped classes could be stored ([by name from ::class](http://php.net/manual/en/language.oop5.constants.php)) 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).

admin commented

2026-01-22 15:10:28 +01:00

@ammont commented on GitHub (May 16, 2018):

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.

@ammont commented on GitHub (May 16, 2018): 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.

Sign in to join this conversation.

Branches Tags

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: doctrine/archived-orm#5536