phpstan-doctrine icon indicating copy to clipboard operation
phpstan-doctrine copied to clipboard

Query result type inference too generic for certain function results

Open janedbal opened this issue 2 years ago • 9 comments

  • See result of functions here and here. I hope I'm not mistaken, but those functions (COUNT, SUM, AVG, MIN, MAX, LENGTH) should always return int, do they not? Those are now infered as int<0, max>|numeric-string.
  • Also, this CASE should always be int imo.
  • And IDENTITY function seems to be infered as mixed even though the type of the FK should be known.

janedbal avatar Jan 27 '22 12:01 janedbal

/cc @arnaud-lb

ondrejmirtes avatar Jan 27 '22 12:01 ondrejmirtes

Hi @janedbal

Thank you for your detailed bug reports :)

  • See result of functions here and here. I hope I'm not mistaken, but those functions (COUNT, SUM, AVG, LENGTH) should always return int, do they not? Those are now infered as int<0, max>|numeric-string.
  • Also, this CASE should always be int imo.

Doctrine does a good job at consistently returning the right type for entity fields. However, for expressions or function calls it just returns the raw value from the database.

So the type of function calls and arithmetic expressions really depends on the database driver (and the driver version).

For instance, the pdo_mysql and pdo_sqlite drivers return integers and floats as strings in PHP < 8.1, or as integer/float in PHP 8.1 (unless PDO::ATTR_STRINGIFY_FETCHES is set, and unless the integer/float is too large to fit in PHP's integer/float types).

Since Doctrine 2.8, the functions COUNT and LENGTH are casted as int by Doctrine, though, so phpstan-doctrine infers these expressions as just int<0,max> when using Doctrine >= 2.8.

For other expressions, I chose to infer types that are correct in all environments, for now. It may be possible to refine these in a future version, although it may be challenging in some cases, and also lead to type instability (it depends on too many factors that may change depending on the environment).

In short, depending on many factors including the PHP version, drivers, and driver options, integer expressions in DQL queries may be returned as int or numeric-string, so phpstan-doctrine infers them as int|numeric-string for now.

  • And IDENTITY function seems to be infered as mixed even though the type of the FK should be known.

IDENTITY is one of the few features that was not supported in the initial PR, but it should be easy to add it now.

arnaud-lb avatar Jan 27 '22 12:01 arnaud-lb

Thank you, now I see the huge complexity you were dealing when implementing this. Good work!

Anyway, shouldnt you be able to detect all those settings from EM when provided (parameters.doctrine.objectManagerLoader)? Driver should be known, PHP version too, Doctrine version as well. Problematic might be PDO setup (if you dont want to query the database).

janedbal avatar Jan 27 '22 15:01 janedbal

Anyway, shouldnt you be able to detect all those settings from EM when provided

We could definitely, but there is the question of the type stability: It can actually be valuable that inferred types don't change with the environment, as it makes the resulting code more portable. Maybe we could add a config parameter for this.

arnaud-lb avatar Jan 27 '22 17:01 arnaud-lb

Maybe we could add a config parameter for this.

I think that would be beneficial.

janedbal avatar Jan 28 '22 08:01 janedbal

it makes the resulting code more portable

That might be super important for libraries, but not for company codebase, where you use single driver and single setup. So as said, some config might be handy so that everybody could decide what is better for his codebase.

janedbal avatar Jan 31 '22 12:01 janedbal