QueryResultTypeWalker: resolve if int/float/bool is fetched as native type or stringified

[janedbal]

Currently, inferring numeric types (mainly results of AVG, SUM etc) is imperfect as it is currently implemented to serve all configurations resulting in union types like int|numeric-string. This is painful to work with in real codebase with some specific configuration where you know it is always int.

~~To match setup of modern PHP versions (8.1+) using most used driver (PDO), I added option not to stringify such expressions.~~

Rewritten to autodetct driver & PHP version & PDO setup to properly infer types.

[ondrejmirtes]

To match setup of modern PHP versions (8.1.25+) using most used driver (PDO), I added option not to stringify such expressions.

What about auto-detecting this from PhpVersion, and also from objectManagerLoader if one is provided?

[janedbal]

Note: ~~this is still WIP and I still plan to finalize this, but~~ implementing all the built-in SQL functions is non-trivial when considering all edge-cases. To be able to implement it properly, I created test repository to verify all differences: https://github.com/janedbal/php-database-drivers-fetch-test/

[VincentLanglet]

Hi @janedbal, I need this PR to move on mine https://github.com/phpstan/phpstan-doctrine/pull/520.

What is missing here (maybe writing tests ?) ; do you need help ?

[ondrejmirtes]

@VincentLanglet Jan already drowned dozens of hours to get this right, it's a very complex topic as you can see in here https://github.com/janedbal/php-database-drivers-fetch-test/. Don't rush this :)

[VincentLanglet]

@VincentLanglet Jan already drowned dozens of hours to get this right, it's a very complex topic as you can see in here janedbal/php-database-drivers-fetch-test. Don't rush this :)

Sorry if my message was heard like this, wasn't my intention. I thought one of the issue with this big PR was to write tests for all the build-in method, and that maybe I could find and offer time to this.

Anyway, it will take the time it need to take ! :)

[janedbal]

The green CI here is misleading, there is still ton of work awaiting (walkFunction is wrong, DoctrineTypeDescriptor::getDatabaseInternalType has no access to driver so it cannot return proper type, tests need to be MUCH better). I just didnt push my local WIP (maybe I should) :)

[janedbal]

@VincentLanglet Pushed the WIP so that you can check the direction I'm leaning towards.

[VincentLanglet]

@VincentLanglet Pushed the WIP so that you can check the direction I'm leaning towards.

Thanks, can I help you on this ? If I understand correctly the goal is to uncomment the tests and make them pass or is there something else ? Do you need more test to be written ?

I notice there was conflict with the 1.3.x branch, do you have time to solve them or do you want me to try ? :)

[janedbal]

@VincentLanglet I've some stuff in backlog to do first before returning to this. But the outline what needs to be done:

• (1) Decide how to deal with untested drivers. For example how BOOL column is fetched. Do we want to:
  • Produce best guess?
    • Currently only postgre uses bool, any other driver is just 1|0 (or '1'|'0' when stringified). So what do we produce for "other driver"? Sidenote: I think bool is currently completely broken for postgre.
  • Return mixed?
  • Return benevolent-union of some reasonable value? What is reasonable value, those other drivers ever used?
• (2) Carefully finalize the implementation (mainly all the aggregate functions, which are tricky)
  • There are plenty of edge-cases to decide.
    • For example SQRT(negative) leads either to NULL or a failure depending on a driver. But I assume we dont want to add NULL to the result just because of this. Or do we?
• (3) Test every supported driver
  • This requires completely new set of tests and new CI runs as it need to start real database to test our PHPStan types against (just like here, but for much bigger matrix of PHP versions, drivers and their configs)
  • Basically, we need very similar CI run as used in https://github.com/janedbal/php-database-drivers-fetch-test/blob/master/test-all-php-versions.sh
• (4) Solve the BC break introduced

And maybe more stuff I already forgot. I think you can easily help with (3) as such test can be added in standalone MR and possibly merged beforehand. The rest is just digging in code.

[VincentLanglet]

@VincentLanglet I've some stuff in backlog to do first before returning to this. But the outline what needs to be done:

• (1) Decide how to deal with untested drivers. For example how BOOL column is fetched. Do we want to:

  • Produce best guess?

    • Currently only postgre uses bool, any other driver is just 1|0 (or '1'|'0' when stringified). So what do we produce for "other driver"? Sidenote: I think bool is currently completely broken for postgre.

  • Return mixed?

  • Return benevolent-union of some reasonable value? What is reasonable value, those other drivers ever used?

I would take the little step by little step approach: untested drivers will returns the same type than before. This way, when this PR is merged:

• tested drivers users will have a better type
• untested drivers won't have anything changed

And if a untested-drivers user complains about the type, he/we just have test this drivers and update the code to support it.

• (2) Carefully finalize the implementation (mainly all the aggregate functions, which are tricky)

  • There are plenty of edge-cases to decide.

    • For example SQRT(negative) leads either to NULL or a failure depending on a driver. But I assume we dont want to add NULL to the result just because of this. Or do we?

What about starting with only the existing implemented aggregate functions ? https://github.com/phpstan/phpstan-doctrine/blob/5ca0f53c60494fa3f2f4792e385b8233bda474fd/src/Type/Doctrine/Query/QueryResultTypeWalker.php#L931

It could be easier to first, make a PR with those aggregate functions and then adding other aggregate one by one ?

• (4) Solve the BC break introduced

The options I see are

• Changing the signature to

public function getDatabaseInternalType(/** Driver $driver */): Type;

and passing the driver. In next major the param will be required. This is often done by Symfony.

• Creating an interface with a different method

interface DriverAwareDoctrineTypeDescriptor {
     public function getDatabaseInternalTypeFromDriver(Driver $driver): Type;
}

and implementing the interface ; then the check

$typeDescriptor instanceof DriverAwareDoctrineTypeDescriptor
    ?  $typeDescriptor->getDatabaseInternalTypeFromDriver($driver);
    :  $typeDescriptor->getDatabaseInternalType();

could be made.

I think the decision is more on @ondrejmirtes about how he wants to proceed in order to evolve such method signature.

[VincentLanglet]

Awesome, thanks a lot for this @janedbal

[ruudk]

Thanks @janedbal for your continued work on making this smarter and smarter 💙

[janedbal]

After discussion with @ondrejmirtes we decided to separate some stuff to other PRs:

• https://github.com/phpstan/phpstan-doctrine/pull/577
• https://github.com/phpstan/phpstan-doctrine/pull/578
• https://github.com/phpstan/phpstan-doctrine/pull/579
• https://github.com/phpstan/phpstan-doctrine/pull/580
• https://github.com/phpstan/phpstan-doctrine/pull/581
• https://github.com/phpstan/phpstan-doctrine/pull/582
• https://github.com/phpstan/phpstan-doctrine/pull/583
• https://github.com/phpstan/phpstan-doctrine/pull/584

To minimize conflicts upon rebase, I squashed this MR. Here is a link to old commits if I ever need to check the real history: https://github.com/phpstan/phpstan-doctrine/commits/8c333fe

[janedbal]

@ruudk, @arnaud-lb, @VincentLanglet Would you mind testing this in your codebases? It would be very helpful to have this verified by more real-world codebases. Thank you!

{
    "require-dev": {
        "phpstan/phpstan-doctrine": "dev-option-not-to-stringify-numeric-types"
    },
    "repositories": [
        {
            "type": "git",
            "url": "[email protected]:janedbal/phpstan-doctrine.git"
        }
    ]
}

[ruudk]

I tested it out on our large application (182 repositories, 16k files) and it works. Unfortunately, it could only remove 3 ignored errors 😅. I guess we don't use that many expressions, or we used Assert.

[arnaud-lb]

I won't be able to test on a meaningful code base, but maybe @Kmecnin can?

[VincentLanglet]

I tested it out on our large application (182 repositories, 16k files) and it works. Unfortunately, it could only remove 3 ignored errors 😅. I guess we don't use that many expressions, or we used Assert.

It may remove more ignored errors with https://github.com/phpstan/phpstan-doctrine/pull/520.

I also tried the branch @janedbal and

• getting 0 new errors
• removed 0 ignored errors

It's mainly because we already typed our code with things like int|string or numeric.

But this PR will help us to change the numeric type by the right type from the database. :)

[ondrejmirtes]

Thank you so much! Let's see how well this fares in real-world projects :)

[janedbal]

It's mainly because we already typed our code with things like int|string or numeric.

Yeah, because PHPStan generally allows type widening.

In our case, we have:

• Custom rule for direct return of Doctrine type where we deny widening (https://gist.github.com/janedbal/08ec3d5f45dad8627d2cf6499b082512)
  • e.g. return $query->getResult()
• Custom rule that enforces inline vardoc when Doctrine type is assigned to a variable (vardoc allows only narrowing by default)
  • e.g. $result = $query->getResult()

Those two rules ensure that I fix the types at least near the source and I can verify that the changes I made here are valid all over our codebase.