Uploaded image for project: 'Doctrine 2 - ORM'
  1. Doctrine 2 - ORM
  2. DDC-544

Extract Interface on Doctrine\ORM\Repository

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Won't Fix
    • Affects Version/s: 2.0-ALPHA4
    • Fix Version/s: 2.0-BETA3
    • Component/s: ORM
    • Security Level: All
    • Labels:
      None

      Description

      There should be an interface composed of all the methods on the Repository, so that in userland you can do something like:

      interface IMyRepository extends Doctrine\ORM\IEntityRepository
      {
      
      }
      
      class MyRepository extends Doctrine\ORM\EntityRepository implements IMyRepository
      {
      
      }
      

      That way in your code you could type-hint for IMyRepository, or for the EntityRepository Interface even and it would be indefinately easier to mock or replace the implementation.

        Activity

        beberlei Benjamin Eberlei created issue -
        beberlei Benjamin Eberlei made changes -
        Field Original Value New Value
        Affects Version/s 2.0-ALPHA4 [ 10036 ]
        Component/s ORM [ 10012 ]
        Hide
        romanb Roman S. Borschel added a comment -

        To match our naming conventions that should rather be: interface EntityRepository and the old EntityRepository => BasicEntityRepository or similar.

        Show
        romanb Roman S. Borschel added a comment - To match our naming conventions that should rather be: interface EntityRepository and the old EntityRepository => BasicEntityRepository or similar.
        romanb Roman S. Borschel made changes -
        Fix Version/s 2.0-BETA2 [ 10050 ]
        Hide
        shurakai Christian Heinrich added a comment -

        Roman, do you mean that we should rename the file (and class) EntityRepository to BasicEntityRepository and create a new EntityRepository that contains an interface EntityRepository?

        If you want me to, I could do this.

        Regards
        Christian

        Show
        shurakai Christian Heinrich added a comment - Roman, do you mean that we should rename the file (and class) EntityRepository to BasicEntityRepository and create a new EntityRepository that contains an interface EntityRepository? If you want me to, I could do this. Regards Christian
        Hide
        romanb Roman S. Borschel added a comment -

        @Christian: Yes, thats what I meant. I will schedule this for BETA3 though. I want BETA2 to be a smooth upgrade without any BC issues, if possible.

        Show
        romanb Roman S. Borschel added a comment - @Christian: Yes, thats what I meant. I will schedule this for BETA3 though. I want BETA2 to be a smooth upgrade without any BC issues, if possible.
        romanb Roman S. Borschel made changes -
        Fix Version/s 2.0-BETA3 [ 10060 ]
        Fix Version/s 2.0-BETA2 [ 10050 ]
        Hide
        romanb Roman S. Borschel added a comment -

        I am wondering whether this is really necessary? There are not many methods on EntityRepository and its easy to create your own interface(s) for your repositories that include these few methods. To be consistent with our naming standards there would have to be a bc break which seems unnecessary to me, unless we invent some other name for the interface, i.e. "ObjectRepository".

        Show
        romanb Roman S. Borschel added a comment - I am wondering whether this is really necessary? There are not many methods on EntityRepository and its easy to create your own interface(s) for your repositories that include these few methods. To be consistent with our naming standards there would have to be a bc break which seems unnecessary to me, unless we invent some other name for the interface, i.e. "ObjectRepository".
        romanb Roman S. Borschel made changes -
        Priority Major [ 3 ] Minor [ 4 ]
        Hide
        beberlei Benjamin Eberlei added a comment -

        Wont fix

        Show
        beberlei Benjamin Eberlei added a comment - Wont fix
        beberlei Benjamin Eberlei made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Won't Fix [ 2 ]
        beberlei Benjamin Eberlei made changes -
        Workflow jira [ 11274 ] jira-feedback [ 14401 ]
        beberlei Benjamin Eberlei made changes -
        Workflow jira-feedback [ 14401 ] jira-feedback2 [ 16265 ]
        beberlei Benjamin Eberlei made changes -
        Workflow jira-feedback2 [ 16265 ] jira-feedback3 [ 18518 ]
        Hide
        saem Saem Ghani added a comment -

        It's unfortunate this wasn't implemented, it's actually a significant issue for us. I have to say I completely disagree with Roman's reasoning in his last comment.

        We have a very large database and commensurately many repositories. We're now at a point that we need to fire our own application level events (as an example, we end up having to compose in services a lot), some of them originating within repositories (doctrine events are insufficient, and we need application specific ones). We're using Symfony2 for DIC and build repositories not through doctrine but via services. We consider using setter injection to be a significant anti-pattern, an object should be valid/fully-initialized post construction so we'd rather not do that – if you were wondering. Now for all our repositories that we would like to build in extra functionality we create our own repository class, compose in the doctrine repository and implement the interface by convention (we could write our own, but the mileage sucks). Now where we could have passed in an instance of anything obeying that interface we're stuck (there goes type hinting). We can avoid some drudgery through traits but it's still an unfortunate solution.

        If this is at all possible (even if it has a less than ideal name), we'd very much appreciate this, thank you.

        PS. A BC break in beta would have been very easy and now it sucks even more.

        Show
        saem Saem Ghani added a comment - It's unfortunate this wasn't implemented, it's actually a significant issue for us. I have to say I completely disagree with Roman's reasoning in his last comment. We have a very large database and commensurately many repositories. We're now at a point that we need to fire our own application level events (as an example, we end up having to compose in services a lot), some of them originating within repositories (doctrine events are insufficient, and we need application specific ones). We're using Symfony2 for DIC and build repositories not through doctrine but via services. We consider using setter injection to be a significant anti-pattern, an object should be valid/fully-initialized post construction so we'd rather not do that – if you were wondering. Now for all our repositories that we would like to build in extra functionality we create our own repository class, compose in the doctrine repository and implement the interface by convention (we could write our own, but the mileage sucks). Now where we could have passed in an instance of anything obeying that interface we're stuck (there goes type hinting). We can avoid some drudgery through traits but it's still an unfortunate solution. If this is at all possible (even if it has a less than ideal name), we'd very much appreciate this, thank you. PS. A BC break in beta would have been very easy and now it sucks even more.
        Hide
        ocramius Marco Pivetta added a comment -

        Saem Ghani there is such an interface in doctrine common. Check Doctrine\Common\Persistence\ObjectRepository at https://github.com/doctrine/common/blob/2.3.2/lib/Doctrine/Common/Persistence/ObjectRepository.php

        Show
        ocramius Marco Pivetta added a comment - Saem Ghani there is such an interface in doctrine common. Check Doctrine\Common\Persistence\ObjectRepository at https://github.com/doctrine/common/blob/2.3.2/lib/Doctrine/Common/Persistence/ObjectRepository.php
        Hide
        saem Saem Ghani added a comment -

        Marco: I'm afraid that is incomplete. EntityRepository has a number of methods that fall outside of the scope of those prototyped in ObjectRepository and Selectable interfaces that it implements.

        Show
        saem Saem Ghani added a comment - Marco: I'm afraid that is incomplete. EntityRepository has a number of methods that fall outside of the scope of those prototyped in ObjectRepository and Selectable interfaces that it implements.
        Hide
        beberlei Benjamin Eberlei added a comment -

        Why don't you ship your own layer of repositories and disregard the Doctrine ones completly? I do that all the time, and then constructor injection is very simple as well.

        Show
        beberlei Benjamin Eberlei added a comment - Why don't you ship your own layer of repositories and disregard the Doctrine ones completly? I do that all the time, and then constructor injection is very simple as well.
        Hide
        saem Saem Ghani added a comment -

        Because the repositories provide features we use. Reusing/staying close to Symfony/Doctrine makes it easier to learn/train, and gives us herd immunity benefits around shared knowledge and most importantly testing. We're trying to reduce maintenance burden, this interface would not only help us do that but anyone else in a similar situation. Additionally, we have many projects most of them large, going across them we've got something like 400+ tables.

        Show
        saem Saem Ghani added a comment - Because the repositories provide features we use. Reusing/staying close to Symfony/Doctrine makes it easier to learn/train, and gives us herd immunity benefits around shared knowledge and most importantly testing. We're trying to reduce maintenance burden, this interface would not only help us do that but anyone else in a similar situation. Additionally, we have many projects most of them large, going across them we've got something like 400+ tables.
        Hide
        ocramius Marco Pivetta added a comment -

        Saem Ghani your last comment does not really provide any rationale behind extraction of the remaining methods into an interface. There's no advantage in extracting those methods to an own interface. createQueryBuilder, createResultSetMappingBuilder, createNamedQuery, createNativeNamedQuery, clear, __call, getEntityManager, getClassMetadata are all utility methods that are not good candidates for an interface. You can easily re-implement those on an existing entity manager.

        Just use the Doctrine\Common\Persistence\ObjectRepository API: you should actually stick to that to increase portability across the various object managers in doctrine project.

        If you need all those methods too, you are looking for inheritance, not composition (and probably are delegating too much responsibility to the repository).

        Show
        ocramius Marco Pivetta added a comment - Saem Ghani your last comment does not really provide any rationale behind extraction of the remaining methods into an interface. There's no advantage in extracting those methods to an own interface. createQueryBuilder, createResultSetMappingBuilder, createNamedQuery, createNativeNamedQuery, clear, __call, getEntityManager, getClassMetadata are all utility methods that are not good candidates for an interface. You can easily re-implement those on an existing entity manager. Just use the Doctrine\Common\Persistence\ObjectRepository API: you should actually stick to that to increase portability across the various object managers in doctrine project. If you need all those methods too, you are looking for inheritance, not composition (and probably are delegating too much responsibility to the repository).
        Hide
        saem Saem Ghani added a comment -

        Marco, rather than looking at each comment in isolation (which is what seems to be happening), please take things in aggregate. I've provided the rationale in previous comments. If you require more information I can clarify further.

        Extracting as an interface would:

        • Make it easier to make mocks during testing (right now a mock EntityManager and ClassMetadata instance must also be mocked out), this is significant friction/noise during testing
        • Allow people to compose as opposed to inherit, especially important when you have features that are specific to relational stores
        • We have Entities that are represented by data in the database AND on the file system. All of a sudden inheriting from a doctrine EntityRepository makes just as much sense as inheriting from a FileSystemRepository (of our own making). All the while the responsibility is the same, load/store of Entities, the finders/query methods still make sense.
        • At the same time the finder API is very useful (duplicating the code would suck, as was suggested earlier), but having to manually maintain that contract with each doctrine release is an unnecessary burden, not just for us, but anyone else in our predicament
        • Creating our own interface means we can't abstract over 3rd party repositories provided by other bundles.
        Show
        saem Saem Ghani added a comment - Marco, rather than looking at each comment in isolation (which is what seems to be happening), please take things in aggregate. I've provided the rationale in previous comments. If you require more information I can clarify further. Extracting as an interface would: Make it easier to make mocks during testing (right now a mock EntityManager and ClassMetadata instance must also be mocked out), this is significant friction/noise during testing Allow people to compose as opposed to inherit, especially important when you have features that are specific to relational stores We have Entities that are represented by data in the database AND on the file system. All of a sudden inheriting from a doctrine EntityRepository makes just as much sense as inheriting from a FileSystemRepository (of our own making). All the while the responsibility is the same, load/store of Entities, the finders/query methods still make sense. At the same time the finder API is very useful (duplicating the code would suck, as was suggested earlier), but having to manually maintain that contract with each doctrine release is an unnecessary burden, not just for us, but anyone else in our predicament Creating our own interface means we can't abstract over 3rd party repositories provided by other bundles.

        This list may be incomplete, as errors occurred whilst retrieving source from linked applications:

        • Request to http://www.doctrine-project.org/fisheye/ failed: Error in remote call to 'FishEye 0 (http://www.doctrine-project.org/fisheye/)' (http://www.doctrine-project.org/fisheye) [AbstractRestCommand{path='/rest-service-fe/search-v1/crossRepositoryQuery', params={expand=changesets[0:20].revisions[0:29],reviews, query=DDC-544}, methodType=GET}] : Received status code 503 (Service Temporarily Unavailable)

          People

          • Assignee:
            romanb Roman S. Borschel
            Reporter:
            beberlei Benjamin Eberlei
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: