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 #404

Merged
merged 1 commit into from
Jan 8, 2023

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Dec 26, 2022

Closes #403

@VincentLanglet VincentLanglet force-pushed the queryResult branch 2 times, most recently from 681d38e to 04aafa6 Compare December 26, 2022 21:20
@VincentLanglet VincentLanglet changed the title Improve QueryResultDynamicReturnTypeExtension Handle all hydration mode in QueryResultDynamicReturnTypeExtension Dec 26, 2022
@VincentLanglet VincentLanglet force-pushed the queryResult branch 3 times, most recently from 2794644 to cdc7f30 Compare December 26, 2022 22:15
@VincentLanglet
Copy link
Contributor Author

Hi @arnaud-lb, I'm trying to improve #232 to supports all hydration modes.

Do you mind taking a look ?
I dunno if we can support everything but maybe some of my ideas are going in the right direction :)

@VincentLanglet VincentLanglet force-pushed the queryResult branch 2 times, most recently from eec9f2e to cefaeec Compare December 26, 2022 23:31
@VincentLanglet VincentLanglet force-pushed the queryResult branch 2 times, most recently from 8d9b685 to c96764c Compare January 7, 2023 21:57
@VincentLanglet
Copy link
Contributor Author

Hi @greg0ire, maybe you can help me on this.

I'm not familiar with all the hydratation mode.
Do you agree with the expected types I wrote in the tests ?

@greg0ire
Copy link

greg0ire commented Jan 7, 2023

Not super familiar either, but nothing strikes me as incorrect in what you wrote.

@VincentLanglet
Copy link
Contributor Author

Not super familiar either, but nothing strikes me as incorrect in what you wrote.

Thanks, if you have a suggestion about who I should/could ping ; I'm open to it.

@ondrejmirtes I would say, this is ready to be reviewed.

@VincentLanglet VincentLanglet marked this pull request as ready for review January 7, 2023 22:25
{
$query = $em->createQuery('
SELECT m
FROM QueryResult\Entities\Many m
');

assertType(
'mixed',
'list<array>',
Copy link
Member

Choose a reason for hiding this comment

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

We should be more specific here - based on the entity, we should know the array shape.

Copy link
Contributor Author

@VincentLanglet VincentLanglet Jan 8, 2023

Choose a reason for hiding this comment

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

I agree. But I thought this could have been a good first step.
Wouldn't be ok to merge this and improve the array shape in a second PR ?

list<array> is better than mixed,
then list<array{id: int, ...}> will be better than list<array>.

Also I had no idea how to improve the

if ($isObject->yes()) {
    return new ArrayType(new MixedType(), new MixedType());
}

part with the PHPStan kit.

@ondrejmirtes
Copy link
Member

Yes, that's fine. I'm gonna merge this and test on real-world projects before releasing it. Thanks.

@ondrejmirtes ondrejmirtes merged commit 5a3bbc1 into phpstan:1.3.x Jan 8, 2023
@ondrejmirtes
Copy link
Member

I think it's mostly fine, but it can lead to new errors. Previously where there was mixed now there's a more specific type. Which means that if you're on level 8, you can get a new error like:

should return array<array{x: string, y: string|null, z: string|null, aa: int|null}> but returns list<array>.

This is happening for HYDRATE_SCALAR when selecting a few specific columns in SELECT.

@ondrejmirtes
Copy link
Member

But on a different project it leads to 108 new errors.

So there are situations where it's wrong.

Some examples:

		$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());

leading to: Offset 'related' does not exist on array{1: string}.

Or:

$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'];
		}

leading to: Cannot call method toString() on array.

Or:

$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;

leading to: Comparison operation ">" between list<array{1: int<0, max>|numeric-string}> and 0 results in an error.

Because of these problems, I'm gonna revert it now. Please send a new PR with improvements. Thank you.

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Jan 8, 2023

But on a different project it leads to 108 new errors.

So there are situations where it's wrong.

Thanks for the report.
It should be better with #412

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.

Add support for other hydration mode in QueryResultDynamicReturnTypeExtension
3 participants