Skip to content

feat: resolve phpstan errors method.nonObject in tests for Types #7253

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
shakaran wants to merge 5 commits into
base: 4.4.x
Choose a base branch
from
+48 −8
Open

feat: resolve phpstan errors method.nonObject in tests for Types #7253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b06a2ae
feat: resolve phpstan errors method.nonObject in tests for Types
shakaran Dec 9, 2025
1abb899
feat: force to use abstract method for convertToPHPValue
shakaran Dec 10, 2025
9e6dffa
fix: add remaining method convertToPHPValue missed in children class …
shakaran Dec 10, 2025
87cc91c
fix: remove inheritdoc detected by phpsniffer
shakaran Dec 10, 2025
c3c9416
fix: typo use integer instead string
shakaran Dec 10, 2025
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
5 changes: 5 additions & 0 deletions src/Types/AsciiStringType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,9 @@ public function getBindingType(): ParameterType
{
return ParameterType::ASCII;
}

public function convertToPHPValue(mixed $value, AbstractPlatform $platform): mixed
{
return $value;
}
}
5 changes: 5 additions & 0 deletions src/Types/EnumType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,9 @@ public function getSQLDeclaration(array $column, AbstractPlatform $platform): st
{
return $platform->getEnumDeclarationSQL($column);
}

public function convertToPHPValue(mixed $value, AbstractPlatform $platform): mixed
{
return $value;
}
}
5 changes: 5 additions & 0 deletions src/Types/GuidType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,9 @@ public function getSQLDeclaration(array $column, AbstractPlatform $platform): st
{
return $platform->getGuidTypeDeclarationSQL($column);
}

public function convertToPHPValue(mixed $value, AbstractPlatform $platform): mixed
{
return $value;
}
}
5 changes: 5 additions & 0 deletions src/Types/StringType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,9 @@ public function getSQLDeclaration(array $column, AbstractPlatform $platform): st
{
return $platform->getStringTypeDeclarationSQL($column);
}

public function convertToPHPValue(mixed $value, AbstractPlatform $platform): mixed
{
return $value;
}
}
5 changes: 1 addition & 4 deletions src/Types/Type.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,7 @@ public function convertToDatabaseValue(mixed $value, AbstractPlatform $platform)
*
* @throws ConversionException
*/
public function convertToPHPValue(mixed $value, AbstractPlatform $platform): mixed
{
return $value;
}
abstract public function convertToPHPValue(mixed $value, AbstractPlatform $platform): mixed;
Copy link
Member

@derrabus derrabus Dec 10, 2025

Choose a reason for hiding this comment

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

This is a breaking change. We can't do this in a minor release.

Copy link
Author

@shakaran shakaran Dec 10, 2025

Choose a reason for hiding this comment

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

So, we use the phpstan annotations for the minor only? and the other solution for a major?

Copy link
Member

@derrabus derrabus Dec 10, 2025

Choose a reason for hiding this comment

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

I don't think that this method needs to be abstract, tbh. And no, I'm not planning major releases for the sake of satisfying a static analyser.


/**
* Gets the SQL declaration snippet for a column of this type.
Expand Down
5 changes: 5 additions & 0 deletions tests/Functional/Schema/MySQL/PointType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,9 @@ public function getMappedDatabaseTypes(AbstractPlatform $platform): array
{
return ['point'];
}

public function convertToPHPValue(mixed $value, AbstractPlatform $platform): mixed
{
return $value;
}
}
11 changes: 7 additions & 4 deletions tests/Functional/Schema/PostgreSQLSchemaManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,12 +761,11 @@ public function testPartitionTable(): void
self::assertTrue($tableFinal->hasColumn('id'));
self::assertTrue($tableFinal->hasColumn('foo'));

$partitionedTableCount = (int) ($this->connection->fetchOne(
self::assertSame(1, $this->connection->fetchOne(
"select count(*) as count from pg_class where relname = 'partitioned_table' and relkind = 'p'",
));
self::assertSame(1, $partitionedTableCount);

$partitionsCount = (int) ($this->connection->fetchOne(
self::assertSame(1, $this->connection->fetchOne(
<<<'SQL'
select count(*) as count
from pg_class parent
Expand All @@ -777,7 +776,6 @@ public function testPartitionTable(): void
where parent.relname = 'partitioned_table' and parent.relkind = 'p';
SQL,
));
self::assertSame(1, $partitionsCount);
}

/** @link https://www.postgresql.org/docs/current/ddl-schemas.html#DDL-SCHEMAS-PUBLIC */
Expand All @@ -796,4 +794,9 @@ public function getSQLDeclaration(array $column, AbstractPlatform $platform): st
{
return 'MyMoney';
}

public function convertToPHPValue(mixed $value, AbstractPlatform $platform): mixed
{
return $value;
}
}
5 changes: 5 additions & 0 deletions tests/Platforms/AbstractPlatformTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,11 @@ public function getSQLDeclaration(array $column, AbstractPlatform $platform): st
{
return $platform->getDecimalTypeDeclarationSQL($column);
}

public function convertToPHPValue(mixed $value, AbstractPlatform $platform): mixed
{
return $value;
}
};

if (Type::hasType($type->getName())) {
Expand Down
3 changes: 3 additions & 0 deletions tests/Types/DateTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public function testDateConvertsToPHPValue(): void

public function testDateResetsNonDatePartsToZeroUnixTimeValues(): void
{
/** @var DateTime $date */
$date = $this->type->convertToPHPValue('1985-09-01', $this->platform);
Comment on lines +36 to 37
Copy link
Member

@derrabus derrabus Dec 10, 2025

Choose a reason for hiding this comment

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

The problem here is that PHPStan does not understand the type of $this->type. Your annotation is a workaround, not a solution.

Copy link
Author

@shakaran shakaran Dec 10, 2025

Choose a reason for hiding this comment

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

Which I see meanwhile I fix this, it is src/Types/Type.php is an abstract class.

And the problem using convertToPHPValue in that class, it is that return as mixed, but other clases like tests/Types/TimeTest.php are overriding the abstract return, for example with ?DateTime

In src/Types/VarDateTimeImmutableType.php is ?DateTimeImmutable.

So I think that the problem is that the method should be abtract in src/Types/Type.php and no contain body

That means which src/Types/TypeWithConstructor.php should have also the method to avoid:

Line TypeWithConstructor.php

:10 Non-abstract class Doctrine\DBAL\Tests\Types\TypeWithConstructor contains abstract method convertToPHPValue() from class
Doctrine\DBAL\Types\Type.

I made that changes at 1abb899

So phpstan is a static analyzer, it doesn't execute code or infer types based on which concrete subclass is instantiated at runtime.

In TimeTest.php, $this->type is declared as Type (the base class), even though it's initialized as new TimeType().

When you call $this->type->convertToPHPValue('01:23:34', $this->platform), phpstan sees the return as mixed because that's what the base class declares.

It doesn't "know" that $this->type is actually a TimeType instance that returns ?DateTime.

So I understan which this is a limitation of static analysis: it prioritizes declared types over inferred runtime types to avoid false positives.

And level 9 requires explicit types where ambiguity exists, even if the code is "obviously" correct at runtime.

Other options that I can try:

  1. Instead of $this->type being Type, declare it as the specific subclass (e.g., protected TimeType $type; in TimeTest). This tells PHPStan the exact type, so the return type is inferred correctly (but reduces polymorphism in tests and it will need separate properties for each type)
  2. Cast to the expected type: $time = (DateTime) $this->type->convertToPHPValue
  3. Use assert (already explored and you dont like it)

Copy link
Member

@derrabus derrabus Dec 10, 2025

Choose a reason for hiding this comment

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

So I think that the problem is that the method should be abtract in src/Types/Type.php and no contain body

No. A subclass may narrow the return type of a method. This has nothing to do with the parent class/method being abstract or not.

Other options that I can try:

  1. Make BaseDateTypeTestCase generic. ✌🏻

Copy link
Member

@morozov morozov Dec 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Make BaseDateTypeTestCase generic. ✌🏻

The problem is not with the tests. The problem is with the types.

Here, one test covers one type, so we have a luxury of parameterizing the tests by guessing what PHP types the given data type operates. But users interact with multiple types at a time – what are they going to do? The test should reflect the intended experience of the consumer, and that's not the experience we intend for.

Instead of tests, types should be parameterized, but that's likely a Pandora's box (our types are a mix of database types, user-defined types, mapping from PHP types to database data types, etc.).


self::assertEquals('00:00:00', $date->format('H:i:s'));
Expand All @@ -42,10 +43,12 @@ public function testDateRestsSummerTimeAffection(): void
{
date_default_timezone_set('Europe/Berlin');

/** @var DateTime $date */
$date = $this->type->convertToPHPValue('2009-08-01', $this->platform);
self::assertEquals('00:00:00', $date->format('H:i:s'));
self::assertEquals('2009-08-01', $date->format('Y-m-d'));

/** @var DateTime $date */
$date = $this->type->convertToPHPValue('2009-11-01', $this->platform);
self::assertEquals('00:00:00', $date->format('H:i:s'));
self::assertEquals('2009-11-01', $date->format('Y-m-d'));
Expand Down
1 change: 1 addition & 0 deletions tests/Types/DateTimeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ public function testConvertsNonMatchingFormatToPhpValueWithParser(): void
{
$date = '1985/09/01 10:10:10.12345';

/** @var DateTime $actual */
$actual = $this->type->convertToPHPValue($date, $this->platform);

self::assertEquals('1985-09-01 10:10:10', $actual->format('Y-m-d H:i:s'));
Expand Down
1 change: 1 addition & 0 deletions tests/Types/TimeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public function testTimeConvertsToPHPValue(): void

public function testDateFieldResetInPHPValue(): void
{
/** @var DateTime $time */
$time = $this->type->convertToPHPValue('01:23:34', $this->platform);

self::assertEquals('01:23:34', $time->format('H:i:s'));
Expand Down
5 changes: 5 additions & 0 deletions tests/Types/TypeWithConstructor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,9 @@ public function getSQLDeclaration(array $column, AbstractPlatform $platform): st
{
return '';
}

public function convertToPHPValue(mixed $value, AbstractPlatform $platform): mixed
{
return $value;
}
}
Loading