Doctrine 2 - ORM
  1. Doctrine 2 - ORM
  2. DDC-764

Discriminator maps interfere with OO design.

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 2.0-BETA3
    • Fix Version/s: None
    • Component/s: ORM
    • Security Level: All
    • Labels:
      None
    • Environment:
      Ubuntu, MySQL

      Description

      I have an abstract superclass which is set for class table inheritance. Currently the discriminator map is used to map strings to class names (with or without namespaces).

      This seems somewhat limiting and also breaks good OO design principles because subclass information must exist in the superclass.

      This means that if I publish a framework with classes designed to be extended, implementers are required to edit the classes they are extending (my framework!), rather than specifying modifications in the subclasses - as per normal OO convention.

      The solutions would either be:

      1) To not use a discriminator map, but to actually store the "Namespace\Class" in the database.
      2) To continue using a discriminator map, but allow for additional "discriminations" to be added by subclasses.

      #1 is the more preferable option as it doesn't require all subclasses to be "found and loaded" by Doctrine prior to using the superclass.

        Activity

        Hide
        Benjamin Eberlei added a comment -

        This is a valid request, however: From the ORM design this data HAS to be on the superclass. This is a problem of the internal datastructure.

        You can however use the "loadMetadata" event to populate the Discriminator Map in a somewhat decoupled way.

        Show
        Benjamin Eberlei added a comment - This is a valid request, however: From the ORM design this data HAS to be on the superclass. This is a problem of the internal datastructure. You can however use the "loadMetadata" event to populate the Discriminator Map in a somewhat decoupled way.
        Hide
        Michael Ridgway added a comment -

        I just had a lengthy discussion about this with the author of the ticket in IRC. A solution to this would be to store the discriminator information in the database. When you persist a new child class, it is registered in the database as a new discriminator for its parent class. Maybe I'm wrong, but the only reason the parent class needs ALL of the subclass discriminators is for hydration purposes. So, it's not a big deal if you don't have the discriminators for a couple of classes if they don't exist yet in the database, just wait until they're actually persisted to add them to the discriminator map.

        Obviously this is more performant if the tool could batch register discriminators to the database as well.

        Now this is probably doable by developers using the existing events/hooks, but I think this is going to be a pretty common request of the ORM (and I've seen it asked many times). I also don't necessarily think this should be the default, but this would be a great option.

        Show
        Michael Ridgway added a comment - I just had a lengthy discussion about this with the author of the ticket in IRC. A solution to this would be to store the discriminator information in the database. When you persist a new child class, it is registered in the database as a new discriminator for its parent class. Maybe I'm wrong, but the only reason the parent class needs ALL of the subclass discriminators is for hydration purposes. So, it's not a big deal if you don't have the discriminators for a couple of classes if they don't exist yet in the database, just wait until they're actually persisted to add them to the discriminator map. Obviously this is more performant if the tool could batch register discriminators to the database as well. Now this is probably doable by developers using the existing events/hooks, but I think this is going to be a pretty common request of the ORM (and I've seen it asked many times). I also don't necessarily think this should be the default, but this would be a great option.
        Hide
        Benjamin Eberlei added a comment -

        You can write a cookbook entry and we will add it to the manual. I guess this would make for a nice addition of the manual

        The subclass information is needed in several places in the ORM, i think roman evaluated a feature request like that before and rejected it because of technical non-feasibility.

        Show
        Benjamin Eberlei added a comment - You can write a cookbook entry and we will add it to the manual. I guess this would make for a nice addition of the manual The subclass information is needed in several places in the ORM, i think roman evaluated a feature request like that before and rejected it because of technical non-feasibility.
        Hide
        Alexander Trauzzi added a comment -

        Where in the ORM would the subclass information be needed in such a way that it couldn't be obtained simply by getting the class namespace and name from the exact same field used as a discriminator column in the abstract class's DB entry? That information is practically already in the database right now. You're just dereferencing it through a table of string to string mappings.

        As Michael indicated, this is a very highly sought after feature. You'll find the loadMetadata approach will get poor (if not zero) adoption because it's imposing a design consideration. Polymorphic relationships in other ORMs are a complete failure because they came to the same conclusions as here.

        To be honest, I'd probably just explain to someone extending my framework that due to a limitation in Doctrine2, they have to open up my superclass and put the name of their subclass in it. It's a lot simpler than telling them to write a method, do a whole bunch of boilerplate, figure out how to get it invoked and keep it all lined up.

        Doctrine2 claims to want to "stay out of the way", I can't think of any better a way to "get in the way" than to force people to maintain registries of subclasses. Again, this restriction does violate proper OO programming principles.

        A superclass should never be obligated to know about it's subclasses. It should be able to treat them as instances of itself. At the very least, Michael's suggestion to have it as another option is worth doing.

        You will hear more about this issue as D2 adoption picks up. Guaranteed.

        Show
        Alexander Trauzzi added a comment - Where in the ORM would the subclass information be needed in such a way that it couldn't be obtained simply by getting the class namespace and name from the exact same field used as a discriminator column in the abstract class's DB entry? That information is practically already in the database right now. You're just dereferencing it through a table of string to string mappings. As Michael indicated, this is a very highly sought after feature. You'll find the loadMetadata approach will get poor (if not zero) adoption because it's imposing a design consideration. Polymorphic relationships in other ORMs are a complete failure because they came to the same conclusions as here. To be honest, I'd probably just explain to someone extending my framework that due to a limitation in Doctrine2, they have to open up my superclass and put the name of their subclass in it. It's a lot simpler than telling them to write a method, do a whole bunch of boilerplate, figure out how to get it invoked and keep it all lined up. Doctrine2 claims to want to "stay out of the way", I can't think of any better a way to "get in the way" than to force people to maintain registries of subclasses. Again, this restriction does violate proper OO programming principles. A superclass should never be obligated to know about it's subclasses. It should be able to treat them as instances of itself. At the very least, Michael's suggestion to have it as another option is worth doing. You will hear more about this issue as D2 adoption picks up. Guaranteed.
        Hide
        Roman S. Borschel added a comment - - edited

        Please read

        http://www.doctrine-project.org/jira/browse/DDC-378
        http://www.doctrine-project.org/jira/browse/DDC-447

        If you have an idea how to patch/solve the problems mentioned in these tickets, we will happily review any patches.

        @"Again, this restriction does violate proper OO programming principles."

        Metadata mapping information has nothing to do with OO principles. You don't necessarily need to "open up" the parent class. If you're using xml/yaml mapping you need to "open up" the mapping file, which has hardly anything to do with OO. Speaking of OO principles I suggest using inheritance less often as it is frequently the poor mans approach to reusability Not implying you're doing that but there are usually much better ways to design a library such that users of the library don't need to create subclasses to use your functionality, which in turn results in strong coupling (inheritance = strong coupling aka "inheritance breaks encapsulation").

        @"A superclass should never be obligated to know about it's subclasses. "

        It doesn't. The mapping metadata needs to know, there is the problem. Its all about the mapping metadata, not about the classes themselves, see the two tickets I linked to above. The metadata is loaded incrementally for efficiency reasons, i.e. only the metadata is loaded on a request that is actually needed. That is the main cause of the problem. In Java I'd just load all the metadata of all classes and process it when the application is deployed, afterwards keeping it in memory while the application is running.

        Thanks for your suggestions.

        Show
        Roman S. Borschel added a comment - - edited Please read http://www.doctrine-project.org/jira/browse/DDC-378 http://www.doctrine-project.org/jira/browse/DDC-447 If you have an idea how to patch/solve the problems mentioned in these tickets, we will happily review any patches. @"Again, this restriction does violate proper OO programming principles." Metadata mapping information has nothing to do with OO principles. You don't necessarily need to "open up" the parent class. If you're using xml/yaml mapping you need to "open up" the mapping file, which has hardly anything to do with OO. Speaking of OO principles I suggest using inheritance less often as it is frequently the poor mans approach to reusability Not implying you're doing that but there are usually much better ways to design a library such that users of the library don't need to create subclasses to use your functionality, which in turn results in strong coupling (inheritance = strong coupling aka "inheritance breaks encapsulation"). @"A superclass should never be obligated to know about it's subclasses. " It doesn't. The mapping metadata needs to know, there is the problem. Its all about the mapping metadata, not about the classes themselves, see the two tickets I linked to above. The metadata is loaded incrementally for efficiency reasons, i.e. only the metadata is loaded on a request that is actually needed. That is the main cause of the problem. In Java I'd just load all the metadata of all classes and process it when the application is deployed, afterwards keeping it in memory while the application is running. Thanks for your suggestions.
        Hide
        Alexander Trauzzi added a comment - - edited

        @If you're using xml/yaml mapping you need to "open up" the mapping file, which has hardly anything to do with OO.
        It is an OO issue because the premise of the design is being violated. Whether it's XML, YAML or annotation metadata. If I write entities (no matter how) for my framework, I should under no circumstance expect someone using my framework to edit my entities. YAML, annotation or XML. Individually they aren't OO syntaxes, but it's all mapping to an OO concept in the end. The code separation is still the same.

        Anyway, I'm not sure if I have the skill or familiarity with Doctrine2 to even do a patch justice.
        But I can certainly offer the overall design. This is all to do with how D2 is marshaling instances of subclasses of a specific parent class. Not about mappings and metadata, or lists of subclasses.
        When dealing with this issue, we have to see the redundancy in discriminator maps. They're asking us for information that is already available!

        Let's just follow this example...


        Classes:

        namespace Fun;

        abstract class Fruit

        { private $colour; }

        class Apple extends Fruit

        { private $sliced; }

        class Banana extends Fruit

        { private $peeled; }

        Doctrine sees the tables:

        Fruit
        o id
        o type
        o colour

        Apple
        o id
        o sliced

        Banana
        o id
        o peeled

        Test case:

        $myApple = new Apple();
        $em->persist($myApple);

        Save Pseudo:

        Doctrine creates the row for the Fruit table, Doctrine calls get_class($myApple), stores "Fun\Apple" in type. Creates a row in Apple, stores the remaining subclass-specific data in it.

        Load Pseudo:

        When loading the class, Doctrine reads the "type" from the Fruit table to infer the class, and largely continues as normal.
        This is almost identical to how it's doing it right now! All we're doing differently is reflecting the class name and using it automatically, instead of using a discriminator map. Which at this point is clearly redundant.

        We don't need to know all the subclasses - ever. It's not needed. We only need to know the subclasses when they exist. If I never call $em->persist($newBanana), there's no entry in the DB with "type" set to "Fun\Banana".

        The net performance impact is actually a gain because we don't have to dereference the discriminator map. We're just creating a new instance of $fruitData['type']().

        Show
        Alexander Trauzzi added a comment - - edited @If you're using xml/yaml mapping you need to "open up" the mapping file, which has hardly anything to do with OO. It is an OO issue because the premise of the design is being violated. Whether it's XML, YAML or annotation metadata. If I write entities (no matter how) for my framework, I should under no circumstance expect someone using my framework to edit my entities. YAML, annotation or XML. Individually they aren't OO syntaxes, but it's all mapping to an OO concept in the end. The code separation is still the same. Anyway, I'm not sure if I have the skill or familiarity with Doctrine2 to even do a patch justice. But I can certainly offer the overall design. This is all to do with how D2 is marshaling instances of subclasses of a specific parent class. Not about mappings and metadata, or lists of subclasses. When dealing with this issue, we have to see the redundancy in discriminator maps. They're asking us for information that is already available! Let's just follow this example... — Classes: namespace Fun; abstract class Fruit { private $colour; } class Apple extends Fruit { private $sliced; } class Banana extends Fruit { private $peeled; } — Doctrine sees the tables: Fruit o id o type o colour Apple o id o sliced Banana o id o peeled — Test case: $myApple = new Apple(); $em->persist($myApple); — Save Pseudo: Doctrine creates the row for the Fruit table, Doctrine calls get_class($myApple), stores "Fun\Apple" in type. Creates a row in Apple, stores the remaining subclass-specific data in it. Load Pseudo: When loading the class, Doctrine reads the "type" from the Fruit table to infer the class, and largely continues as normal. This is almost identical to how it's doing it right now! All we're doing differently is reflecting the class name and using it automatically, instead of using a discriminator map. Which at this point is clearly redundant. We don't need to know all the subclasses - ever. It's not needed. We only need to know the subclasses when they exist. If I never call $em->persist($newBanana), there's no entry in the DB with "type" set to "Fun\Banana". The net performance impact is actually a gain because we don't have to dereference the discriminator map. We're just creating a new instance of $fruitData ['type'] ().
        Hide
        Alexander Trauzzi added a comment -

        An indivitual on IRC going by DanielF contacted me and said he will be looking into whether this can be done in Doctrine.

        It's a strong feature and definitely not unlike what's being done already, I'll help him in any way I can with testing/concept. If any others can help as he's doing this or offer some pointers, by all means!

        Show
        Alexander Trauzzi added a comment - An indivitual on IRC going by DanielF contacted me and said he will be looking into whether this can be done in Doctrine. It's a strong feature and definitely not unlike what's being done already, I'll help him in any way I can with testing/concept. If any others can help as he's doing this or offer some pointers, by all means!

          People

          • Assignee:
            Roman S. Borschel
            Reporter:
            Alexander Trauzzi
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: