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

janedbal · 2023-12-13T14:22:37Z

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.

src/Type/Doctrine/Query/QueryResultTypeWalker.php

ondrejmirtes · 2023-12-13T20:13:09Z

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 · 2023-12-20T09:58:13Z

src/Type/Doctrine/Query/QueryResultTypeWalker.php

@@ -812,43 +844,39 @@ public function walkSelectExpression($selectExpression)
 			$resultAlias = $selectExpression->fieldIdentificationVariable ?? $this->scalarResultCounter++;
 			$type = $this->unmarshalType($expr->dispatch($this));

-			if (class_exists(TypedExpression::class) && $expr instanceof TypedExpression) {


This was dead block of code as class_exists(TypedExpression::class) is always false.

phpstan/phpstan-src#2838

janedbal · 2023-12-20T10:22:47Z

src/Type/Doctrine/Query/QueryResultTypeWalker.php

@@ -1097,8 +1127,12 @@ public function walkLiteral($literal)
 				break;

 			case AST\Literal::BOOLEAN:
-				$value = strtolower($literal->value) === 'true' ? 1 : 0;
-				$type = new ConstantIntegerType($value);


This was just wrong for postgres, that returns real booleans, but we were infering 1|0|'1'|'0'

janedbal · 2023-12-20T12:11:50Z

src/Type/Doctrine/Query/QueryResultTypeWalker.php

+	private function shouldStringifyExpressions(Type $type): TrinaryLogic
+	{
+		$driver = $this->em->getConnection()->getDriver();
+		$nativeConnection = $this->getNativeConnection();


Sadly, this causes connection initialization.

janedbal · 2024-01-15T08:58:51Z

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 · 2024-01-17T10:26:37Z

Hi @janedbal, I need this PR to move on mine #520.

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

ondrejmirtes · 2024-01-17T10:28:11Z

@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 · 2024-01-17T10:34:12Z

@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 · 2024-01-17T11:51:55Z

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 · 2024-01-17T12:21:23Z

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

VincentLanglet · 2024-01-17T13:19:00Z

src/Type/Doctrine/Descriptors/DoctrineTypeDescriptor.php

+	 * - bigint:   int         int        int      int
+	 * - bool:     int         int       bool     bool
+	 */
+	public function getDatabaseInternalType(Driver $driver): Type;


This change will be a BC break since the interface has @api annotation.

Not sure which strategy should be followed to avoid this...
Should another interface be introduced ?

Yeah, another interface somehow.

VincentLanglet · 2024-02-14T16:50:37Z

@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 · 2024-02-14T17:18:00Z

@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 · 2024-02-14T17:49:01Z

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

phpstan-doctrine/src/Type/Doctrine/Query/QueryResultTypeWalker.php

Line 931 in 5ca0f53

public function walkAggregateExpression($aggExpression): string

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.

QueryResultTypeWalker: add option not to stringify expressions

1a72c65

JanTvrdik reviewed Dec 13, 2023

View reviewed changes

src/Type/Doctrine/Query/QueryResultTypeWalker.php Outdated Show resolved Hide resolved

src/Type/Doctrine/Query/QueryResultTypeWalker.php Outdated Show resolved Hide resolved

janedbal added 3 commits December 13, 2023 15:33

Tests not to fail on PHP 8 and below

1dd6fd7

stripos

8c726d8

Better explain mysql's decimal literal and float literal

6a271d9

janedbal mentioned this pull request Dec 18, 2023

Unable to check PDO::ATTR_STRINGIFY_FETCHES configuration php/php-src#12969

Closed

Autodetect stringification behaviour

661c370

janedbal changed the title ~~QueryResultTypeWalker: add option not to stringify expressions~~ QueryResultTypeWalker: resolve if int/float/bool is fetched as native type or stringified Dec 18, 2023

janedbal added 3 commits December 20, 2023 10:41

Fix test for <= PHP8.1 and lowest deps

3f3a066

cs fix: sprintf

3057e41

Ignore old dbal missing driver classes

f6a34b3

janedbal commented Dec 20, 2023

View reviewed changes

janedbal marked this pull request as draft December 21, 2023 08:28

This was referenced Jan 17, 2024

Handle all hydration mode in QueryResultDynamicReturnTypeExtension #453

Merged

Improve QueryResultDynamicReturnTypeExtension #520

Open

WIP

6439355

VincentLanglet reviewed Jan 17, 2024

View reviewed changes

VincentLanglet mentioned this pull request Feb 17, 2024

WIP - Introduce tests on platforms #549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

janedbal commented Dec 13, 2023 •

edited

ondrejmirtes commented Dec 13, 2023

janedbal Dec 20, 2023

janedbal Dec 20, 2023

janedbal Dec 20, 2023

janedbal Dec 20, 2023

janedbal commented Jan 15, 2024

VincentLanglet commented Jan 17, 2024

ondrejmirtes commented Jan 17, 2024

VincentLanglet commented Jan 17, 2024

janedbal commented Jan 17, 2024

janedbal commented Jan 17, 2024

VincentLanglet Jan 17, 2024 •

edited

janedbal Jan 17, 2024

VincentLanglet commented Feb 14, 2024

janedbal commented Feb 14, 2024

VincentLanglet commented Feb 14, 2024

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

Are you sure you want to change the base?

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

Conversation

janedbal commented Dec 13, 2023 • edited

ondrejmirtes commented Dec 13, 2023

janedbal Dec 20, 2023

Choose a reason for hiding this comment

janedbal Dec 20, 2023

Choose a reason for hiding this comment

janedbal Dec 20, 2023

Choose a reason for hiding this comment

janedbal Dec 20, 2023

Choose a reason for hiding this comment

janedbal commented Jan 15, 2024

VincentLanglet commented Jan 17, 2024

ondrejmirtes commented Jan 17, 2024

VincentLanglet commented Jan 17, 2024

janedbal commented Jan 17, 2024

janedbal commented Jan 17, 2024

VincentLanglet Jan 17, 2024 • edited

Choose a reason for hiding this comment

janedbal Jan 17, 2024

Choose a reason for hiding this comment

VincentLanglet commented Feb 14, 2024

janedbal commented Feb 14, 2024

VincentLanglet commented Feb 14, 2024

janedbal commented Dec 13, 2023 •

edited

VincentLanglet Jan 17, 2024 •

edited