AbstractQuery::getSingleScalarResult() throws exception when no result

[BenMorel]

Please don't merge this PR yet, I think there's a lot of confusion around the current behaviour, that must be cleared first.

---

The current doc for AbstractQuery::getSingleScalarResult() says:

@return mixed The scalar result, or NULL if the query returned no result.

Even though it currently throws an exception in this case. I tested this myself, and it is confirmed by this test:

https://github.com/doctrine/orm/blob/v2.6.4/tests/Doctrine/Tests/ORM/Functional/QueryTest.php#L339

    /**
     * @expectedException Doctrine\ORM\NoResultException
     */
    public function testGetSingleScalarResultThrowsExceptionOnNoResult()
    {
        $this->_em->createQuery("select a from Doctrine\Tests\Models\CMS\CmsArticle a")
             ->getSingleScalarResult();
    }

This PR fixes the docblock.

---

That being said, I'm really confused by this code:

https://github.com/doctrine/orm/blob/v2.6.4/lib/Doctrine/ORM/AbstractQuery.php#L805

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

This looks like the method wants to behave as the doc says, by not throwing when the hydration mode is HYDRATE_SINGLE_SCALAR, but because it does not test against the $hydrationMode parameter, but against $this->_hydrationMode, the behaviour is not as expected.

From what I can see in the code, it should, deeply nested in some method calls, setHydrationMode() to the value provided as a parameter at some point, but it looks like it doesn't.

So in summary:

• getSingleScalarResult() throws an exception by default,
• but it does return null instead, if setHydrationMode(HYDRATE_SINGLE_SCALAR) has been called on the Query before that

So I'm not sure whether fixing the docblock is the right thing to do, we should probably fix the code so that it behaves consistently instead, by always returning null for HYDRATE_SINGLE_SCALAR; but this could break existing code that, despite the doc, may now rely on this "feature", which is even backed by a test.

Thoughts?

[greg0ire]

An option could be to deprecate it in favor of a new method with a clearly defined contract, backed up with tests, but that would imply finding a new name for that method, and inconsistency with other methods, unless we mass deprecate the methods in AbstractQuery

[BenMorel]

I'm not sure what the plans are for 3.0, but still, it could be nice to fix the inconsistency on this method in the 2.x releases. An plan could be:

• document the inconsistency in the method's docblock, and mention that it's a known bug that will not be fixed in 2.6
• fix the bug in 2.7, by always returning null when it should? And document the change explicitly in the changelog, of course.

[greg0ire]

document the inconsistency in the method's docblock, and mention that it's a known bug that will not be fixed in 2.6

👍

fix the bug in 2.7, by always returning null when it should? And document the change explicitly in the changelog, of course.

I'm afraid this method has a quite big usage, such a change could have a really big impact, that's why I was leaning towards a deprecation.

Regarding the fix: what if there is a record that matches the WHERE clause, but the value that is retrieved is NULL? We would have no way to differentiate that from no record matching the WHERE clause. Is returning null really the best option?

[BenMorel]

Regarding the fix: what if there is a record that matches the WHERE clause, but the value that is retrieved is NULL? We would have no way to differentiate that from no record matching the WHERE clause. Is returning null really the best option?

IDK, I was just suggesting to make the behaviour right by respecting the documented contract (I'd personally only use it for non-nullable columns), but I don't really mind if it throws instead.

[lcobucci]

but it does return null instead, if setHydrationMode(HYDRATE_SINGLE_SCALAR) has been called on the Query before that

I know this is a tad confusing but getSingleResult() sends the hydration mode to execute(), which forwards it to the right method (depending on the L2C configuration). If we do send the query to the DB, executeIgnoreQueryCache() is called and that sets the hydration mode.

This means that it's completely fine for getSingleResult() to rely on _hydrationMode.

With that said, getSingleScalarResult() should never throw a NoResultException.

[BenMorel]

I know this is a tad confusing but getSingleResult() sends the hydration mode to execute(), which forwards it to the right method (depending on the L2C configuration). If we do send the query to the DB, executeIgnoreQueryCache() is called and that sets the hydration mode.

But that means that if I call getSingleResult(HYDRATE_SINGLE_SCALAR), I'm not guaranteed that the HYDRATE_SINGLE_SCALAR mode will be used, as this depends on the configuration?

How weird.

[lcobucci]

You're guaranteed since it does change the configuration of the hydration mode.

I quickly debugged things and saw that the exception is actually thrown by SingleScalarHydrator#hydrateAllData().
Historically speaking, AbstractQuery#getSingleResult() was created about 2 years before of SingleScalarHydrator#hydrateAllData().

What I think happened here is that, when introducing the SingleScalarHydrator#hydrateAllData(), we didn't update the documentation/behaviour of AbstractQuery#getSingleResult() - which lead us to this very confusing thing.

Since the code in SingleScalarHydrator#hydrateAllData() that throws the exception hasn't been changed since its creation I'd say that this PR is the correct way to address this issue without messing up with BC. Maybe @Ocramius or @guilhermeblanco can confirm it before we merge things.

[guilhermeblanco]

When executing a "must-be exactly one" operation, any result that is not exactly 1 should throw an exception. This is a good standard practice that most parts of Doctrine have borrowed from Hibernate. So the code is indeed correct and the comments were wrong.

Merging the patch! Thanks!!! =D