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

Fix return type of getSingleScalarResult #10870

Merged
merged 1 commit into from
Aug 9, 2023
Merged

Fix return type of getSingleScalarResult #10870

merged 1 commit into from
Aug 9, 2023

Conversation

whatUwant
Copy link
Contributor

Doctrine\ORM\AbstractQuery::getSingleScalarResult() does not have null as a possible type of returned data while NoResultException is listed as one of the possible exceptions thrown.

However NoResultException is thrown only when hydration mode is not HYDRATE_SINGLE_SCALAR.

    public function getSingleResult($hydrationMode = null)
    {
        $result = $this->execute(null, $hydrationMode);

        if ($this->_hydrationMode !== self::HYDRATE_SINGLE_SCALAR && ! $result) {
            throw new NoResultException();
        }

In getSingleScalarResult hydration mode was set to HYDRATE_SINGLE_SCALAR via Doctrine\ORM\AbstractQuery::execute() which makes it impossible to throw that particular type of exception.

@derrabus derrabus merged commit 6b220e3 into doctrine:2.16.x Aug 9, 2023
@whatUwant whatUwant deleted the fix-getSingleScalarResult-return-type branch August 9, 2023 09:42
@darenas31415
Copy link
Contributor

darenas31415 commented Aug 10, 2023

However NoResultException is thrown only when hydration mode is not HYDRATE_SINGLE_SCALAR.

That's not quite correct as $result = $this->execute(null, $hydrationMode); can still throw NoResultException exception.

Please see screenshot:

image

How to reproduce:

$qb = $this->em->createQueryBuilder();

$qb->select('p.code')
   ->from('App:Product', 'p')
   ->where('p.name = :name')
   ->setMaxResults(1)
   ->setParameter('name', 'missing');

$code = $qb->getQuery()->getSingleScalarResult();

@derrabus
Copy link
Member

You're right. Please send a PR to add back the @throws annotation.

@darenas31415
Copy link
Contributor

darenas31415 commented Aug 10, 2023

Shall I add back only @throws annotation or revert this PR completely?

I can't make getSingleScalarResult return null under any condition, I think the original code was correct.

I'd suggest to revert this PR altogether unless @whatUwant can provide a test case for returning null.

@whatUwant
Copy link
Contributor Author

It's correct that NoResultException can still be thrown.
null can be returned when aggregate functions such as MIN MAX are used.

$qb = $this->em->createQueryBuilder();

$qb->select('MAX(p.id)')
   ->from('App:Product', 'p')
   ->where('p.name = :name')
   ->setParameter('name', 'missing');

$code = $qb->getQuery()->getSingleScalarResult();

@derrabus
Copy link
Member

getSingleScalarResult() with a SELECT on a nullable field should yield null, shouldn't it?

@darenas31415
Copy link
Contributor

getSingleScalarResult() with a SELECT on a nullable field should yield null, shouldn't it?

You are totally right 👍

derrabus pushed a commit that referenced this pull request Aug 13, 2023
Fix regression introduced in #10870

`$result = $this->execute(null, $hydrationMode);` in `getSingleResult` can still throw NoResultException exception.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants