findOneBy: interface and implementation do not match #6443

Open
opened 2026-01-22 15:33:23 +01:00 by admin · 8 comments
Owner

Originally created by @LucHamers on GitHub (Apr 12, 2020).

Q A
Version 2.8.0

Support Question

The method findOneBy ist implemented in Doctrine\ORM\EntityRepository as follows:

public function findOneBy(array $criteria, array $orderBy = null)
{
    $persister = $this->_em->getUnitOfWork()->getEntityPersister($this->_entityName);

    return $persister->load($criteria, null, null, [], null, 1, $orderBy);
}

It implements the interface Doctrine\Persistence\ObjectRepository, in which the method is defined as follows:

public function findOneBy(array $criteria);

In the interface the parameter $orderBy is missing. Is this a bug?

Originally created by @LucHamers on GitHub (Apr 12, 2020). | Q | A |------------ | ----- | Version | 2.8.0 ### Support Question The method findOneBy ist implemented in Doctrine\ORM\EntityRepository as follows: ```php public function findOneBy(array $criteria, array $orderBy = null) { $persister = $this->_em->getUnitOfWork()->getEntityPersister($this->_entityName); return $persister->load($criteria, null, null, [], null, 1, $orderBy); } ``` It implements the interface Doctrine\Persistence\ObjectRepository, in which the method is defined as follows: ```php public function findOneBy(array $criteria); ``` In the interface the parameter $orderBy is missing. Is this a bug?
Author
Owner

@SenseException commented on GitHub (Apr 12, 2020):

Persistence is a different project than ORM and others that are depending on Persistence don't need a second argument. I agree that this could be solved for a new major release if the interface would be in ORM.

PHP allows to have additional arguments compared to an interface if those have a default value.

@SenseException commented on GitHub (Apr 12, 2020): Persistence is a different project than ORM and others that are depending on Persistence don't need a second argument. I agree that this could be solved for a new major release if the interface would be in ORM. PHP allows to have additional arguments compared to an interface if those have a default value.
Author
Owner

@LucHamers commented on GitHub (Apr 12, 2020):

Technically this is no issue, using the parameter works fine. Problem is, that the phpstorm shows a warning that there is a parameter where there shouldn't be one. And in my opinion the parameter is important, and as user I cannot find it when my IDE only shows the interface without this parameter.

@LucHamers commented on GitHub (Apr 12, 2020): Technically this is no issue, using the parameter works fine. Problem is, that the phpstorm shows a warning that there is a parameter where there shouldn't be one. And in my opinion the parameter is important, and as user I cannot find it when my IDE only shows the interface without this parameter.
Author
Owner

@greg0ire commented on GitHub (Apr 12, 2020):

Technically this is no issue, using the parameter works fine.

You have no guarantee about that though, correct? If you do, then you have to let PHPStorm figure out that the object you are using is an EntityRepository somehow, be it with checks, assertions, @var annotations or type declarations.

@greg0ire commented on GitHub (Apr 12, 2020): > Technically this is no issue, using the parameter works fine. You have no guarantee about that though, correct? If you do, then you have to let PHPStorm figure out that the object you are using is an `EntityRepository` somehow, be it with checks, assertions, `@var` annotations or type declarations.
Author
Owner

@LucHamers commented on GitHub (Apr 13, 2020):

In my opinion, the interface should reflect the implementation and vice versa. If the EntityRepository needs a different interface (and it obviously does), then it also should have an interface WITH the $orderBy parameter in the findOneBy method. That this problem shows in the IDE, is just a symptom. There probably is some kind of workaround for the IDE via annotations or whatever, but I think that the root cause of this should be addressed.

@LucHamers commented on GitHub (Apr 13, 2020): In my opinion, the interface should reflect the implementation and vice versa. If the EntityRepository needs a different interface (and it obviously does), then it also should have an interface WITH the $orderBy parameter in the findOneBy method. That this problem shows in the IDE, is just a symptom. There probably is some kind of workaround for the IDE via annotations or whatever, but I think that the root cause of this should be addressed.
Author
Owner

@greg0ire commented on GitHub (Apr 13, 2020):

I think that the root cause of this should be addresses.

Me too, but before that we should try to understand why things were done like this.

Here is the commit that introduces the interface: 59e6b8c6ed

Here is the PR that introduces the implementation: https://github.com/doctrine/orm/pull/504. Sadly, the commit message does not help. Let it be a reminder that commit messages consisting only of links to Github, or worse, JIRA issues should be forbidden.
EDIT: found the correct URL: https://github.com/doctrine/orm/issues/1850

Apparently, adding this argument to the interface was considered

Also see: https://github.com/doctrine/common/pull/741

Are there other implementations that also have this parameter by the way? And can you show the code sample that causes your issue?

@greg0ire commented on GitHub (Apr 13, 2020): > I think that the root cause of this should be addresses. Me too, but before that we should try to understand why things were done like this. Here is the commit that introduces the interface: https://github.com/doctrine/common/commit/59e6b8c6edcb271622923035b687a063c2b47ce8 Here is the PR that introduces the implementation: https://github.com/doctrine/orm/pull/504. Sadly, the commit message does not help. Let it be a reminder that commit messages consisting only of links to Github, or worse, JIRA issues should be forbidden. EDIT: found the correct URL: https://github.com/doctrine/orm/issues/1850 [Apparently, adding this argument to the interface was considered](https://github.com/doctrine/common/issues/591#issuecomment-162376042) Also see: https://github.com/doctrine/common/pull/741 Are there other implementations that also have this parameter by the way? And can you show the code sample that causes your issue?
Author
Owner

@LucHamers commented on GitHub (Apr 13, 2020):

I don't know if there are other implementations of this interface. I am just a doctrine user. In my code, I use it to find the highest number in a table:

// MembershipNumber contains among other fields the actual number "membershipNumber"
// Find the entry with the highest membership number in the database
$lastMembershipNumberEntry = $this->entityManager->getRepository(MembershipNumber::class)
                                                 ->findOneBy([], ['membershipNumber' => 'DESC']);
$nextNumber = 1;
if ($lastMembershipNumberEntry) {
    $nextNumber = $lastMembershipNumberEntry->getMembershipNumber() + 1;
}
// now create a new entry with the next available number
@LucHamers commented on GitHub (Apr 13, 2020): I don't know if there are other implementations of this interface. I am just a doctrine user. In my code, I use it to find the highest number in a table: ````php // MembershipNumber contains among other fields the actual number "membershipNumber" // Find the entry with the highest membership number in the database $lastMembershipNumberEntry = $this->entityManager->getRepository(MembershipNumber::class) ->findOneBy([], ['membershipNumber' => 'DESC']); $nextNumber = 1; if ($lastMembershipNumberEntry) { $nextNumber = $lastMembershipNumberEntry->getMembershipNumber() + 1; } // now create a new entry with the next available number ````
Author
Owner

@greg0ire commented on GitHub (Apr 13, 2020):

I don't know if there are other implementations of this interface. I am just a doctrine user.

The good news is, you don't have to be a Doctrine maintainer to answer that question, being a Github user will be enough: https://github.com/search?q=org%3Adoctrine+%22implements+ObjectRepository%22&type=Code

When I read your code, I feel that the issue would go away by changing this piece of code: 8c259ea5cb/lib/Doctrine/ORM/EntityManager.php (L693)

Let's either replace | with &, or just drop ObjectManager| entirely, since it provides no additional information.

But then again, this line of code has a history: https://github.com/doctrine/orm/pull/6780

@greg0ire commented on GitHub (Apr 13, 2020): > I don't know if there are other implementations of this interface. I am just a doctrine user. The good news is, you don't have to be a Doctrine maintainer to answer that question, being a Github user will be enough: https://github.com/search?q=org%3Adoctrine+%22implements+ObjectRepository%22&type=Code When I read your code, I feel that the issue would go away by changing this piece of code: https://github.com/doctrine/orm/blob/8c259ea5cb632dbb57001b2262048ae7fa52b102/lib/Doctrine/ORM/EntityManager.php#L693 Let's either replace `|` with `&`, or just drop `ObjectManager|` entirely, since it provides no additional information. But then again, this line of code has a history: https://github.com/doctrine/orm/pull/6780
Author
Owner

@greg0ire commented on GitHub (Apr 13, 2020):

Maybe we should somehow discourage the use of getRepository, see this post of how you could change your code to completely avoid that kind of issues, and get some other advantages in the process: https://www.tomasvotruba.com/blog/2017/10/16/how-to-use-repository-with-doctrine-as-service-in-symfony/

EDIT:

Maybe we should somehow discourage the use of getRepository

yes we should. If you want to help with this, consider contributing documentation updates showing other, better ways of doing things.

@greg0ire commented on GitHub (Apr 13, 2020): Maybe we should somehow discourage the use of `getRepository`, see this post of how you could change your code to completely avoid that kind of issues, and get some other advantages in the process: https://www.tomasvotruba.com/blog/2017/10/16/how-to-use-repository-with-doctrine-as-service-in-symfony/ EDIT: > Maybe we should somehow discourage the use of `getRepository` [yes we should](https://github.com/doctrine/orm/pull/6780#discussion_r157355972). If you want to help with this, consider contributing documentation updates showing other, better ways of doing things.
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: doctrine/archived-orm#6443