mirror of
https://github.com/doctrine/orm.git
synced 2026-03-23 22:42:18 +01:00
New Doctrine\ORM\QueryInterface to combat "final class Query"; mocking in unit tests #7014
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @kafoso on GitHub (Jul 21, 2022).
New
Doctrine\ORM\QueryInterfaceto combatfinal class QuerySummary
As discussed in https://github.com/doctrine/orm/issues/2830, the class
Doctrine\ORM\Queryis final and it continues to be in 3.0.x:d41c4c6cc6/lib/Doctrine/ORM/Query.phpDoctrine\ORM\QueryextendsDoctrine\ORM\AbstractQuery, but that is the end of the road. No other classes, interfaces, or traits are involved.This is a hassle in the following scenarios:
Doctrine\ORM\Query.Doctrine\ORM\Queryclass.Proposal
Doctrine\ORM\QueryInterface, whichDoctrine\ORM\Query(orDoctrine\ORM\AbstractQuery) then implements.Doctrine\ORM\Query(parameters and return types) to instead type hintDoctrine\ORM\QueryInterface.Doctrine\ORM\Query(and potentiallyDoctrine\ORM\AbstractQuery) toDoctrine\ORM\QueryInterface.Regarding point 2 above: This may cause backwards compatibility issues in cases where userland code type hints
Doctrine\ORM\Query. This should be a fairly limited corner case, again becauseDoctrine\ORM\Queryis final, and the very absence of an interface prevents decorator implementations.@kafoso commented on GitHub (Jul 23, 2022):
I've made the code changes necessary to achieve the above and said changes are fairly manageable. Both phpstan and phpunit are still happy.
I moved the
Doctrine\ORM\AbstractQuery::HYDRATE_*constants to the newDoctrine\ORM\QueryInterface, and because of this, I:tests/Doctrine/Tests/ORM/Functional/Ticket/GH9926Test.php), which specifically tests, thatDoctrine\ORM\AbstractQuery::HYDRATE_*references continue to function. This is mainly to ensure userland implementations targeting these constants won't break.If desired, I'll make a pull request with said changes.
I did, however, notice that a good number of classes in this library are final. Arguments for having all these classes be final probably exist, but the above would be completely obsolete, if the "final" keyword was removed from
Doctrine\ORM\Queryand potentially other classes.It is still a good idea to cover central classes with an interface, though. This leads to another question: Should we look at other classes, which suffer from the same "final class" restriction, and also provide an interface for these classes?
@beberlei commented on GitHub (Jul 23, 2022):
If you feel that you want to mock Query, then you sre doing testing wrong imho.
First, you should never mock apis that you dont own.
Second, returning mocks from mocks is usually sign of a code smell. Since you need to mock either EM or Repository to get to a query.
You should wrap the query in a repository and use an integration test.
@kafoso commented on GitHub (Jul 23, 2022):
At https://www.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/query-builder.html (latest currently being 3.3.7), the authors have entirely omitted an explanation of how results are retrieved using the QueryBuilder. Perhaps an oversight?
If we turn back the clock and look at https://www.doctrine-project.org/projects/doctrine-orm/en/2.8/reference/query-builder.html instead, we see the following:
And further down (Executing a Query), we see:
The code example below on the website specifically mentions
Doctrine\ORM\QueryBuilder->getQuery().Looking at the 3.0.x code for the QueryBuilder, it does not appear this API has changed. I.e. we will still have to do
Doctrine\ORM\QueryBuilder->getQuery().Surely, this means
Doctrine\ORM\Queryis part of the public API when using query builders?To be clear, I am speaking mostly of decorator and testing scenarios in userland code.
If tests are written for e.g. repositories, which has access to the Entity Manager (
Doctrine\ORM\EntityManagerInterface), andDoctrine\ORM\EntityManagerInterface->createQueryBuilder()is called in the repository class, the test code should be able to control the resulting Query instance. For unit testing purposes, this would be a mock (e.g.PHPUnit\Framework\MockObject\MockObjectwhen using phpunit, which – despite the name – is an interface).Mocking classes or interfaces is how most unit tests are performed, providing safeguards against unwanted and unintended code execution. It is not code smell. It literally is a major feature of phpunit and other xUnit implementations.
Unit tests over Integration tests. Otherwise, you'll be forcing userland implementations to deal with even more of the internals of Doctrine and potentially interface it with a database, which goes even further counter to the simple fix I am proposing here.
Don't get me wrong: Integration tests most definitely should be performed as part of the Doctrine test suite. Rarely is this necessary for the userland implementations.
The
finalkeyword isn't inherently bad (as Jon Skeet would probably attest to), which is why I am not suggesting removing it. I am instead suggesting the interface alternative, which (A) keeps theDoctrine\ORM\Queryclass "under lock and key", but (B) allows for decorator patterns, mocking, and ultimately entire substitutions in userland (if people want to go crazy, they will).@VincentLanglet commented on GitHub (Nov 20, 2023):
In my case @beberlei we would like to decorate the Query::execute method which is currently not possible because the class is final without any interface.