Skip to content

Add failing tests and TODOs for doctrine types #342

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add failing tests and TODOs for doctrine types #342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e972564
Add failing tests and TODOs for doctrine types
Seldaek May 18, 2022
5e37fec
Allow serializing DateTime objects (partial impl from #339)
Seldaek May 18, 2022
8b0541c
WIP for bound parameter type forwarding
Seldaek May 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/Extensions/DoctrineConnectionExecuteQueryDynamicReturnTypeExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,28 +54,33 @@ public function getTypeFromMethodCall(MethodReflection $methodReflection, Method
}

$params = null;
$paramTypes = null;
if (\count($args) > 1) {
$params = $args[1]->value;
}
if (\count($args) > 2) {
$paramTypes = $args[2]->value;
}

$resultType = $this->inferType($args[0]->value, $params, $scope);
$resultType = $this->inferType($args[0]->value, $params, $paramTypes, $scope);
if (null !== $resultType) {
return $resultType;
}

return $defaultReturn;
}

private function inferType(Expr $queryExpr, ?Expr $paramsExpr, Scope $scope): ?Type
private function inferType(Expr $queryExpr, ?Expr $paramsExpr, ?Expr $paramTypesExpr, Scope $scope): ?Type
{
if (null === $paramsExpr) {
$queryReflection = new QueryReflection();
$queryStrings = $queryReflection->resolveQueryStrings($queryExpr, $scope);
} else {
$parameterTypes = $scope->getType($paramsExpr);
$boundParameterTypes = $scope->getType($paramsExpr);

$queryReflection = new QueryReflection();
$queryStrings = $queryReflection->resolvePreparedQueryStrings($queryExpr, $parameterTypes, $scope);
$queryStrings = $queryReflection->resolvePreparedQueryStrings($queryExpr, $parameterTypes, $boundParameterTypes, $scope);
}

$doctrineReflection = new DoctrineReflection();
Expand Down
2 changes: 1 addition & 1 deletion src/Extensions/DoctrineConnectionFetchDynamicReturnTypeExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ private function inferType(MethodReflection $methodReflection, Expr $queryExpr,
$queryStrings = $queryReflection->resolveQueryStrings($queryExpr, $scope);
} else {
$parameterTypes = $scope->getType($paramsExpr);
$queryStrings = $queryReflection->resolvePreparedQueryStrings($queryExpr, $parameterTypes, $scope);
$queryStrings = $queryReflection->resolvePreparedQueryStrings($queryExpr, $parameterTypes, null, $scope);
}

return $doctrineReflection->createFetchType($queryStrings, $methodReflection);
Expand Down
2 changes: 1 addition & 1 deletion src/Extensions/DoctrineStatementExecuteDynamicReturnTypeExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ private function inferType(MethodReflection $methodReflection, MethodCall $metho
}

$queryReflection = new QueryReflection();
$queryStrings = $queryReflection->resolvePreparedQueryStrings($queryExpr, $parameterTypes, $scope);
$queryStrings = $queryReflection->resolvePreparedQueryStrings($queryExpr, $parameterTypes, null, $scope);

return $doctrineReflection->createGenericResult($queryStrings, QueryReflector::FETCH_TYPE_BOTH);
}
Expand Down
2 changes: 1 addition & 1 deletion src/Extensions/PdoStatementExecuteTypeSpecifyingExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ private function inferStatementType(MethodReflection $methodReflection, MethodCa
$parameterTypes = $scope->getType($args[0]->value);

$queryReflection = new QueryReflection();
$queryStrings = $queryReflection->resolvePreparedQueryStrings($queryExpr, $parameterTypes, $scope);
$queryStrings = $queryReflection->resolvePreparedQueryStrings($queryExpr, $parameterTypes, null, $scope);

$reflectionFetchType = QueryReflection::getRuntimeConfiguration()->getDefaultFetchMode();

Expand Down
20 changes: 10 additions & 10 deletions src/QueryReflection/QueryReflection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,12 @@ public function getResultType(string $queryString, int $fetchType): ?Type
*
* @throws UnresolvableQueryException
*/
public function resolvePreparedQueryStrings(Expr $queryExpr, Type $parameterTypes, Scope $scope): iterable
public function resolvePreparedQueryStrings(Expr $queryExpr, Type $parameterTypes, ?Type $boundParameterTypes, Scope $scope): iterable
{
$type = $scope->getType($queryExpr);

if ($type instanceof UnionType) {
$parameters = $this->resolveParameters($parameterTypes);
$parameters = $this->resolveParameters($parameterTypes, $boundParameterTypes);
if (null === $parameters) {
return null;
}
Expand All @@ -103,15 +103,15 @@ public function resolvePreparedQueryStrings(Expr $queryExpr, Type $parameterType
*
* @throws UnresolvableQueryException
*/
public function resolvePreparedQueryString(Expr $queryExpr, Type $parameterTypes, Scope $scope): ?string
public function resolvePreparedQueryString(Expr $queryExpr, Type $parameterTypes, ?Type, $boundParameterTypes, Scope $scope): ?string
{
$queryString = $this->resolveQueryExpr($queryExpr, $scope);

if (null === $queryString) {
return null;
}

$parameters = $this->resolveParameters($parameterTypes);
$parameters = $this->resolveParameters($parameterTypes, $boundParameterTypes);
if (null === $parameters) {
return null;
}
Expand Down Expand Up @@ -211,20 +211,20 @@ public static function getQueryType(string $query): ?string
*
* @throws UnresolvableQueryException
*/
public function resolveParameters(Type $parameterTypes): ?array
public function resolveParameters(Type $parameterTypes, ?Type $boundParameterTypes): ?array
{
$parameters = [];

if ($parameterTypes instanceof UnionType) {
foreach (TypeUtils::getConstantArrays($parameterTypes) as $constantArray) {
$parameters = $parameters + $this->resolveConstantArray($constantArray, true);
$parameters = $parameters + $this->resolveConstantArray($constantArray, $boundParameterTypes, true);
}

return $parameters;
}

if ($parameterTypes instanceof ConstantArrayType) {
return $this->resolveConstantArray($parameterTypes, false);
return $this->resolveConstantArray($parameterTypes, $boundParameterTypes, false);
}

return null;
Expand All @@ -235,7 +235,7 @@ public function resolveParameters(Type $parameterTypes): ?array
*
* @throws UnresolvableQueryException
*/
private function resolveConstantArray(ConstantArrayType $parameterTypes, bool $forceOptional): array
private function resolveConstantArray(ConstantArrayType $parameterTypes, ?Type $boundParameterTypes, bool $forceOptional): array
{
$parameters = [];

Expand All @@ -259,7 +259,7 @@ private function resolveConstantArray(ConstantArrayType $parameterTypes, bool $f
$param = new Parameter(
$placeholderName,
$valueTypes[$i],
QuerySimulation::simulateParamValueType($valueTypes[$i], true),
QuerySimulation::simulateParamValueType($valueTypes[$i], true), // TODO pass $i of $boundParameterTypes here if a resolved constant type is available?
$isOptional
);

Expand All @@ -268,7 +268,7 @@ private function resolveConstantArray(ConstantArrayType $parameterTypes, bool $f
$param = new Parameter(
null,
$valueTypes[$i],
QuerySimulation::simulateParamValueType($valueTypes[$i], true),
QuerySimulation::simulateParamValueType($valueTypes[$i], true), // TODO pass $i of $boundParameterTypes here if a resolved constant type is available?
$isOptional
);

Expand Down
4 changes: 4 additions & 0 deletions src/QueryReflection/QuerySimulation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ public static function simulateParamValueType(Type $paramType, bool $preparedPar
return null;
}

if ($paramType instanceof ObjectType && $paramType->isInstanceOf(\DateTimeInterface::class)) {
return date(self::DATE_FORMAT, 0);
}

$stringType = new StringType();
$isStringableObjectType = $paramType instanceof ObjectType
&& $paramType->isInstanceOf(Stringable::class)->yes();
Expand Down
2 changes: 1 addition & 1 deletion src/Rules/PdoStatementExecuteMethodRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ private function checkErrors(MethodReflection $methodReflection, MethodCall $met
}

try {
$parameters = $queryReflection->resolveParameters($parameterTypes) ?? [];
$parameters = $queryReflection->resolveParameters($parameterTypes, null) ?? [];
} catch (UnresolvableQueryException $exception) {
return [
RuleErrorBuilder::message($exception->asRuleMessage())->tip(UnresolvableQueryException::RULE_TIP)->line($methodCall->getLine())->build(),
Expand Down
46 changes: 46 additions & 0 deletions tests/default/data/doctrine-dbal.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Doctrine\DBAL\Cache\QueryCacheProfile;
use Doctrine\DBAL\Connection;
use Doctrine\DBAL\Types\Types;
use function PHPStan\Testing\assertType;
use staabm\PHPStanDba\Tests\Fixture\StringableObject;

Expand Down Expand Up @@ -218,4 +219,49 @@ public function dateParameter(Connection $conn)
$fetchResult = $conn->fetchOne($query, [date('Y-m-d', strtotime('-3hour'))]);
assertType('int|false', $fetchResult);
}

public function customTypeParameters(Connection $conn, int $adaid)
{
$stmt = $conn->prepare('SELECT count(*) AS c FROM typemix WHERE c_datetime=:last_dt');
$result = $stmt->execute(['dt' => new \DateTime()], ['dt' => Types::DATETIME_MUTABLE]);
Copy link
Owner

@staabm staabm May 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does doctrine allow to pass in a DateTime object without also giving a parameter-type as a 2nd arg?

or should phpstan-dba error when arg1 is DateTime and arg2 is either missing or not equal to Types::DATETIME_MUTABLE?

edit: I see, there are a few types arround dates in doctrine: https://github.com/doctrine/dbal/blob/007223cfe9dc5cf37420a6a8d8ecd48d520b69d4/src/Types/Types.php#L18-L24

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if it lets you do it without the param type, but I'd think not.

assertType('Doctrine\DBAL\Result', $result);

$stmt = $conn->prepare('SELECT count(*) AS c FROM typemix WHERE c_datetime=:last_dt');
$result = $stmt->execute(['dt' => new \DateTime()], ['dt' => Types::DATETIME_IMMUTABLE]);
Comment on lines +229 to +230
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this test also use $conn->executeQuery() instead? it seems there is no prepare+execute api including bound-parameter-types...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DBAL supports this on all connection methods it seems https://github.com/doctrine/dbal/blob/3.3.x/src/Connection.php#L529

Also on quote https://github.com/doctrine/dbal/blob/3.3.x/src/Connection.php#L811

So I guess ideally this would be supported on all the things, but executeStatement/executeQuery would be the must-haves IMO as it's the lowest common denominator and the rest is kinda decorating this.

// TODO should probably fail as DateTime is passed to a DATETIME_IMMUTABLE type
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think doctrine will fail though, the distinction between datetime/datetime_immutable is only made there when deserializing data from DB back into PHP AFAIK, so maybe we can let it go through..

assertType('Doctrine\DBAL\Result', $result);
}

/**
* @param array<int> $ids
* @param array<string> $vals
*/
public function boundArrays(Connection $conn, array $ids, array $vals)
{
$result = $conn->executeQuery(
'SELECT count(*) AS c FROM ada WHERE adaid IN (?)',
[$ids],
[\Doctrine\DBAL\Connection::PARAM_INT_ARRAY]
);
assertType('Doctrine\DBAL\Result<array{c: int, 0: int}>', $result);

$result = $conn->executeQuery(
'SELECT count(*) AS c FROM ada WHERE email IN (?)',
[$vals],
[\Doctrine\DBAL\Connection::PARAM_STR_ARRAY]
);
assertType('Doctrine\DBAL\Result<array{c: int, 0: int}>', $result);

$result = $conn->executeQuery(
'SELECT count(*) AS c FROM ada WHERE adaid IN (?)',
[$vals], // TODO should error as $vals is not int array
[\Doctrine\DBAL\Connection::PARAM_INT_ARRAY]
);

$result = $conn->executeQuery(
'SELECT count(*) AS c FROM ada WHERE email IN (?)',
[$ids], // TODO should error as $ids is not str array
[\Doctrine\DBAL\Connection::PARAM_STR_ARRAY]
);
}
}