Skip to content
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.

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

Handle all hydration mode in QueryResultDynamicReturnTypeExtension #412

Merged
merged 12 commits into from
Apr 5, 2023

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Jan 8, 2023

Follow up of #404

Fix SingleScalarResult: to fix

$eventsCount = $this->entityManager->createQueryBuilder()
			->select('COUNT(cle.id)')
			->from(CountryLocalityEvent::class, 'cle')
			->andWhere('cle.user = ?', $user)
			->andWhere('cle.expirationTime > ?', DateTimeProvider::now())
			->getQuery()
			->getSingleScalarResult();

		return $eventsCount > 0;

Fix ArrayResult: to fix

$result = $this->entityManager->createQueryBuilder()
			->select('d.name, d.id')
			->from(Destination::class, 'd')
			->addOrderBy('d.name')
			->getQuery()
			->getArrayResult();

		$destinations = [];
		foreach ($result as $item) {
			$destinations[(string) $item['id']->toString()] = (string) $item['name'];
		}

For the last issue @ondrejmirtes,

$query = $this->entityManager->createQueryBuilder()
			->select('IDENTITY(n.related)')
			->from(DistrictNeighbour::class, 'n')
			->andWhere('n.center = ?', $districtId)
			->getQuery();

		return array_map(static fn (array $row): UuidInterface => $row['related'], $query->getArrayResult());

I wonder if it couldn't be a bug in your codebase.

@VincentLanglet VincentLanglet marked this pull request as ready for review January 8, 2023 22:07
@VincentLanglet VincentLanglet force-pushed the queryResult branch 4 times, most recently from 01000d4 to d92cfc3 Compare February 25, 2023 15:24
@VincentLanglet
Copy link
Contributor Author

I understand it's not an easy PR to review/try, but if you find time to take a new look it would be great @ondrejmirtes.

Specifically because you reported in #404 (comment)
that

$query = $this->entityManager->createQueryBuilder()
			->select('IDENTITY(n.related)')
			->from(DistrictNeighbour::class, 'n')
			->andWhere('n.center = ?', $districtId)
			->getQuery();

		return array_map(static fn (array $row): UuidInterface => $row['related'], $query->getArrayResult());

was reported as an error ; and I believe there might be an error in your code base rather than in my PR
since my PR is consistent with the existing test
https://github.com/phpstan/phpstan-doctrine/blob/1.3.x/tests/Type/Doctrine/Query/QueryResultTypeWalkerTest.php#L1394-L1419
https://github.com/phpstan/phpstan-doctrine/blob/1.3.x/tests/Type/Doctrine/Query/QueryResultTypeWalkerTest.php#L215-L233

So I would need a confirmation from you
(and also need to know if the 108 new errors are fixed or if there is others I need to fix).

Thanks.

@ondrejmirtes
Copy link
Member

Alright, I'm gonna merge this, try it on a few real-world projects, and either release it, or report back the errors. Thank you!

@ondrejmirtes ondrejmirtes merged commit c110e57 into phpstan:1.3.x Apr 5, 2023
@VincentLanglet
Copy link
Contributor Author

Alright, I'm gonna merge this, try it on a few real-world projects, and either release it, or report back the errors. Thank you!

Thanks ! :)
I tried this PR on my work project, so far I got no false positive.
I'll be available to fix as quickly as possible any false error reported after this PR.

@ondrejmirtes
Copy link
Member

One more failure:

    /**
     * @return array{id: int, x: mixed, y: string, z: int, zz: string}|null
     */
    private function b(): ?array
    {
        return $this->getEntityManager()->createQueryBuilder()
            ->from(xyz::class, 'abc')
            ->select('abc')
            ->getQuery()
            ->getOneOrNullResult(Query::HYDRATE_ARRAY);
    }

Here PHPStan complains:

  157    Method
         a::b()
         should return array{id: int, x: mixed, y: string, z: int,
         zz: string}|null but returns array|null.

PHPStan should be able to look at entity fields and figure out that array shape is returned.

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Apr 5, 2023

One more failure:

    /**
     * @return array{id: int, x: mixed, y: string, z: int, zz: string}|null
     */
    private function b(): ?array
    {
        return $this->getEntityManager()->createQueryBuilder()
            ->from(xyz::class, 'abc')
            ->select('abc')
            ->getQuery()
            ->getOneOrNullResult(Query::HYDRATE_ARRAY);
    }

Here PHPStan complains:

  157    Method
         a::b()
         should return array{id: int, x: mixed, y: string, z: int,
         zz: string}|null but returns array|null.

PHPStan should be able to look at entity fields and figure out that array shape is returned.

This is kinda "tested" with 03741b4#diff-28394df5dba3e85fe86f9c8d9b05d6087fe897d0efa1d0c0ad3533a1f0252b99R158
and is a consequence of
03741b4#diff-d465b53a864937dbadc4795d83fc507747a0d603b3f517132f05c7098ffea8b7R204

I don't master enough this lib to look at entity fields in order to transform the object to an array shape (thanks to doctrine and object manager). Does something like this already exists in this plugin ?

I understand Array<Mixed, Mixed> can create issues...
One option, as a first step, could be to revert Array<Mixed, Mixed> to Mixed in case of object.
I don't think there is something like a BenevolentArray which would:

  • Return no error for @return array, @return array{foo: string}, ...
  • Return an error for @return Foo

Is there ? This could be useful here...

@ondrejmirtes
Copy link
Member

I don't master enough this lib to look at entity fields in order to transform the object to an array shape (thanks to doctrine and object manager). Does something like this already exists in this plugin ?

I don't know what the logic is in Doctrine to transform entity into an array with array hydration.

But you can get all there is to know about an entity with ObjectMetadataResolver, especially getClassMetadata. So you should look at the logic in Doctrine itself and replicate it here.

@VincentLanglet
Copy link
Contributor Author

But you can get all there is to know about an entity with ObjectMetadataResolver, especially getClassMetadata. So you should look at the logic in Doctrine itself and replicate it here.

Thanks, I find a solution with #443
But I currently have an issue with multi-selects.

Until I find a solution, if you prefer, I provided a general fix #444 which just revert this PR in the special case of getArrayResult with entities in the select.

I'll prioritize fixing all the false-positive before improving the QueryResult analysis.
I'll work on phpstan/phpstan-src#2209 (comment) now, but it's not easy since I still didn't reproduce the error...

@VincentLanglet
Copy link
Contributor Author

On one real world project, it shows 101 new errors.

One example:

	/**
	 * @return array<int, array<string, int>>
	 */
	public function getIdsByStatus(BankTransactionStatus $status): array
	{
		return $this->entityManager->createQueryBuilder()
			->select('bt.id')
			->from(BankTransaction::class, 'bt')
			->andWhere('bt.status = :status')->setParameter('status', $status)
			->addOrderBy('bt.id', 'ASC')
			->getQuery()
			->getArrayResult();
	}

PHPStan reports:

 ------ -------------------------------------------------------------------------------------------
  Line   app/services/Bank/BankTransactionRepository.php
 ------ -------------------------------------------------------------------------------------------
  473    Method Slevomat\Bank\BankTransactionRepository::getIdsByStatus() should return array<int,
         array<string, int>> but returns array<int, array<string, int|null>>.
 ------ -------------------------------------------------------------------------------------------

The primary key really can't be null. The definition:

	#[ORM\Id]
	#[ORM\GeneratedValue]
	#[ORM\Column(type: 'big_integer', options: ['unsigned' => true])]
	#[Get]
	private ?int $id = null;

I tried multiple times on my project similar queries and:
I correctly get

Dumped type: array<int, array{id: numeric-string}>

With bigint witch is the typed used by ORM

#[ORM\Id]
#[ORM\GeneratedValue]
#[ORM\Column(type: 'bigint', options: ['unsigned' => true])]
private ?int $id = null;

(Notice a bigint is returned as a string by Doctrine and not an int).

With big_integer:

#[ORM\Id]
#[ORM\GeneratedValue]
#[ORM\Column(type: 'big_integer', options: ['unsigned' => true])]
private ?int $id = null;

I get

Dumped type: array<int, array{id: mixed}>

Is it a custom type @ondrejmirtes ?

@ondrejmirtes
Copy link
Member

Good news - the problem with nullable primary key is project-specific problem that I was able to fix.

I'm gonna try to reproduce another problem I'm seeing.

@ondrejmirtes
Copy link
Member

See #446

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants