Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Invalid
    • Affects Version/s: Git Master
    • Fix Version/s: None
    • Component/s: ORM
    • Security Level: All
    • Labels:
      None

      Description

      Hi. I am just now trying to convert a PHP project from Symfony 2.0 to Symfony 2.1. Then I also upgraded to the latest version of Doctrine, and I run into a severe problem.

      The application I am converting has two classes for users, one called "User" and the other called "UserName". They both inherit from "UserBase" and "UserName" is in fact an empty class that just inherit everything from "UserBase". Why do we have this setup? Because the "User" class is big and ugly with many dependencies form other tables in the database, and every time you load a "User" object it has to do a big join. And even then it has several collections that are lazy-loaded by Doctrine. In contrast "User Name" only contains name and e-mail, and is sufficient for most uses in the application.

      It was not possible to let "UserBase" be this lightweight user object, because it had to be declared as a mapped superclass, and a mapped superclass cannot be an entity. It functioned very well to let a lot of ManyToOne relationships in other classes be to the "UserName" entity, because then it only had to join one table, and it was fast. The full functionality of the "User" class we only need a few places.

      So, all was well and good, until I now got the task of porting the whole thing to the latest version of Symfony. What is wrong? I get the error message "Found entity of type User on association Object#Name, but expecting UserName". It is in the file UnitOfWork.php around line 740, and it makes my task impossible.

      In some cases I got a "User" object instead of "UserName" object that is declared in the "targetEntity". And that should be ok, because they come from the same table, and I only want to save the userId on the other object, and all "User" and "UserName" objects are directly form the database and not altered when I try to persist that other object.

      So, please, please, can you make the check a bit less restrictive? May be not just check if the entry is "instanceof" the "targetEntity", but allow it if the entity refers to the same table. Or only throw the exception if it is a new entity that needs to managed. Or make an annotation that makes it possible to switch this check off? I am a bit desperate here, because I really see no way around this. Thank you.

      1. Request.php
        10 kB
        Terje Bråten
      2. User.php
        18 kB
        Terje Bråten
      3. UserBase.php
        3 kB
        Terje Bråten
      4. UserName.php
        0.2 kB
        Terje Bråten

        Activity

        Terje Bråten created issue -
        Hide
        Benjamin Eberlei added a comment -

        Can you show your code that leads to this error? Maybe you can change your mapping slightly to make a refactoring possible. I understand the problem, but I don't think we can make this check less restrictive, because essentially what you are doing is a hack and this check protects users from a class of very very ugly and hard to debug bugs.

        Show
        Benjamin Eberlei added a comment - Can you show your code that leads to this error? Maybe you can change your mapping slightly to make a refactoring possible. I understand the problem, but I don't think we can make this check less restrictive, because essentially what you are doing is a hack and this check protects users from a class of very very ugly and hard to debug bugs.
        Hide
        Terje Bråten added a comment -

        Ok, here is some of my code files attached.
        Look in Request.php to see Performer, Initiator and Requestor. Many times they are filled with "UserName" objects, that come from a choice list. And the objects in the choice list comes from the database through Doctrine. But sometimes we also would like to fill those properties on the Request object with a fully fledged "User" object, that typically is the current logged in user.

        The only way around I see is to get another "UserName" object from the DB, that is the same as the "User", but that seems to be an unnecessary trip to the database that will slow things down.

        Terje

        Show
        Terje Bråten added a comment - Ok, here is some of my code files attached. Look in Request.php to see Performer, Initiator and Requestor. Many times they are filled with "UserName" objects, that come from a choice list. And the objects in the choice list comes from the database through Doctrine. But sometimes we also would like to fill those properties on the Request object with a fully fledged "User" object, that typically is the current logged in user. The only way around I see is to get another "UserName" object from the DB, that is the same as the "User", but that seems to be an unnecessary trip to the database that will slow things down. Terje
        Terje Bråten made changes -
        Field Original Value New Value
        Attachment User.php [ 11319 ]
        Attachment UserBase.php [ 11320 ]
        Attachment UserName.php [ 11321 ]
        Attachment Request.php [ 11322 ]
        Hide
        Terje Bråten added a comment - - edited

        By the way, is it not the setters on the object that is supposed to protect programmers from setting the wrong type of object on a property?

        I think it is a bad idea to double that kind of check in the ORM framework, even if it could lead to hard to detect errors. May be you could move the check down into the switch, to the case where you know it will persist the new object?

        Show
        Terje Bråten added a comment - - edited By the way, is it not the setters on the object that is supposed to protect programmers from setting the wrong type of object on a property? I think it is a bad idea to double that kind of check in the ORM framework, even if it could lead to hard to detect errors. May be you could move the check down into the switch, to the case where you know it will persist the new object?
        Hide
        Terje Bråten added a comment -

        Hi Benjamin,
        Have you thought more about this issue?

        Terje

        Show
        Terje Bråten added a comment - Hi Benjamin, Have you thought more about this issue? Terje
        Hide
        Marco Pivetta added a comment -

        Terje Bråten I don't think you should swap User and UserName objects within your entities, since they have different meaning. That will bring to weird issues and I think you are misusing a feature that otherwise is handled via partial objects.

        Having an object reference User or UserName is quite different in the ORM.

        If you really want to avoid lazy loading and use your own technique instead, use the referenced object's ID and a service or the EntityManager/EntityRepository API:

        $myObject = $someService->getSomeStuff();
        $id = $myObject->getUser()->getId(); // proxies have been optimized to avoid queries in this case
        $user = $someOtherService->getUser($id); // retrieve the lightweight or heavy version of the User based on your needs
        
        Show
        Marco Pivetta added a comment - Terje Bråten I don't think you should swap User and UserName objects within your entities, since they have different meaning. That will bring to weird issues and I think you are misusing a feature that otherwise is handled via partial objects. Having an object reference User or UserName is quite different in the ORM. If you really want to avoid lazy loading and use your own technique instead, use the referenced object's ID and a service or the EntityManager/EntityRepository API: $myObject = $someService->getSomeStuff(); $id = $myObject->getUser()->getId(); // proxies have been optimized to avoid queries in this case $user = $someOtherService->getUser($id); // retrieve the lightweight or heavy version of the User based on your needs
        Hide
        Terje Bråten added a comment -

        Hi Marco,

        Thank you for your input, but it does not solve my problem.

        The problem is when I want to save a $myObject in the database with a different or user object id attached to it in a specific property.
        I have a User object, but the property takes a UserName object.

        Show
        Terje Bråten added a comment - Hi Marco, Thank you for your input, but it does not solve my problem. The problem is when I want to save a $myObject in the database with a different or user object id attached to it in a specific property. I have a User object, but the property takes a UserName object.
        Hide
        Terje Bråten added a comment -

        Would it be acceptable to make this a configuration option?

        It could be named "targetEntityChecking" whith a default value of "strict", when target objects are strictly checked for being of the right class, but also with the possibility to set this option to "off", when you want to leave this check to the property setters where it really belongs.

        Because there really is use cases when it would be really useful to have this check be off, and then the framework would not be a such a straitjacket.

        (I can make a patch to this, if it would be accepted.)

        Terje.

        Show
        Terje Bråten added a comment - Would it be acceptable to make this a configuration option? It could be named "targetEntityChecking" whith a default value of "strict", when target objects are strictly checked for being of the right class, but also with the possibility to set this option to "off", when you want to leave this check to the property setters where it really belongs. Because there really is use cases when it would be really useful to have this check be off, and then the framework would not be a such a straitjacket. (I can make a patch to this, if it would be accepted.) Terje.
        Hide
        Marco Pivetta added a comment -

        Tarjei Huse I would be against it, since persisting an User or an UserName would be fundamentally different. I really think this is over-optimization work that you are encountering because you are not using a service layer to interact with lazy loading.

        Show
        Marco Pivetta added a comment - Tarjei Huse I would be against it, since persisting an User or an UserName would be fundamentally different. I really think this is over-optimization work that you are encountering because you are not using a service layer to interact with lazy loading.
        Hide
        Terje Bråten added a comment -

        The actual User or UserName object will always be in the database from before, and will be unchanged. It will not need persisting when the other object is persisted.

        What I want to persist is in my case the "Request" object, that has different properties with the value of users attached.

        So, if I have an User object, will you then force me to get a new UserName object from the DB whith the same ID, so set a UserName valued property on my Request object?

        As I stated before, it should not be the task of Doctrine to check the class of the value in the property, that task belongs to the setter. By duplicating that control, doctrine becomes too restrictive for me.

        Terje

        Show
        Terje Bråten added a comment - The actual User or UserName object will always be in the database from before, and will be unchanged. It will not need persisting when the other object is persisted. What I want to persist is in my case the "Request" object, that has different properties with the value of users attached. So, if I have an User object, will you then force me to get a new UserName object from the DB whith the same ID, so set a UserName valued property on my Request object? As I stated before, it should not be the task of Doctrine to check the class of the value in the property, that task belongs to the setter. By duplicating that control, doctrine becomes too restrictive for me. Terje
        Hide
        Marco Pivetta added a comment -

        Terje Bråten since you are using a read-only object, I'd then suggest you to mark it as read only with https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/UnitOfWork.php#L3007-3027 instead. That will also prevent the UnitOfWork from checking it.

        Anyway, I am still against having this different loading method, since the object expected on that relation may be requested by other classes/services that may want the full version of it while you have loaded the partial one, requiring you to have type checks all over the place.
        You are mixing up non homogeneous types, I don't think this should be supported nor suggested.

        Show
        Marco Pivetta added a comment - Terje Bråten since you are using a read-only object, I'd then suggest you to mark it as read only with https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/UnitOfWork.php#L3007-3027 instead. That will also prevent the UnitOfWork from checking it. Anyway, I am still against having this different loading method, since the object expected on that relation may be requested by other classes/services that may want the full version of it while you have loaded the partial one, requiring you to have type checks all over the place. You are mixing up non homogeneous types, I don't think this should be supported nor suggested.
        Hide
        Terje Bråten added a comment -

        Hi Marco,

        You are still showing a very poor understanding of the original problem.

        Marking the object as read-only would not help in any way, since that is not checked before this check that is causing trouble: https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/UnitOfWork.php#L752-762

        And I am not proposing any different loading method of any kind.

        Terje

        Show
        Terje Bråten added a comment - Hi Marco, You are still showing a very poor understanding of the original problem. Marking the object as read-only would not help in any way, since that is not checked before this check that is causing trouble: https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/UnitOfWork.php#L752-762 And I am not proposing any different loading method of any kind. Terje
        Hide
        Terje Bråten added a comment - - edited

        I guess my main beef with this check is that it is unconditional.
        The check is done, weather it is needed or not. If it only could be moved down a few lines to after the case self::STATE_NEW: statement, the only place it would make some sense to check it, all would be well and I could be a happy man and get along with what I was doing before.

        Terje.

        Show
        Terje Bråten added a comment - - edited I guess my main beef with this check is that it is unconditional. The check is done, weather it is needed or not. If it only could be moved down a few lines to after the case self::STATE_NEW: statement, the only place it would make some sense to check it, all would be well and I could be a happy man and get along with what I was doing before. Terje.
        Hide
        Benjamin Eberlei added a comment - - edited

        Hm, that would actually make sense to move the check into STATE_NEW. Good idea, can you open a Pull Request?

        Edit: wait no, thats wrong. You can still put a managed other entity there and its not allowed to be there.

        Show
        Benjamin Eberlei added a comment - - edited Hm, that would actually make sense to move the check into STATE_NEW. Good idea, can you open a Pull Request? Edit: wait no, thats wrong. You can still put a managed other entity there and its not allowed to be there.
        Hide
        Terje Bråten added a comment -

        But that is my whole point, if it is another managed entity there it should be the task of the setter on the object, and not doctrine, to check if it is allowed to be there or not.

        Could you then at at least allow the check to be conditioned on a doctrine configuration setting?
        (I proposed "targetEntityChecking", with values "strict" or "off".)

        Show
        Terje Bråten added a comment - But that is my whole point, if it is another managed entity there it should be the task of the setter on the object, and not doctrine, to check if it is allowed to be there or not. Could you then at at least allow the check to be conditioned on a doctrine configuration setting? (I proposed "targetEntityChecking", with values "strict" or "off".)
        Hide
        Terje Bråten added a comment -

        Hi Benjamin.
        Did you have a look to see if I could change the mapping and make a refactoring possible?
        Or do you have any thoughts on weahter the check could be made dependent on a configuration setting?

        Terje.

        Show
        Terje Bråten added a comment - Hi Benjamin. Did you have a look to see if I could change the mapping and make a refactoring possible? Or do you have any thoughts on weahter the check could be made dependent on a configuration setting? Terje.
        Hide
        Terje Bråten added a comment -

        Hi. I would appreciate some more feedback from the lead developer on this issue. Please?

        Should I go ahead and make a working patch for this issue, so that the execution of this test can be configured?

        Show
        Terje Bråten added a comment - Hi. I would appreciate some more feedback from the lead developer on this issue. Please? Should I go ahead and make a working patch for this issue, so that the execution of this test can be configured?
        Hide
        Terje Bråten added a comment -

        I do not like being ignored.

        Show
        Terje Bråten added a comment - I do not like being ignored.
        Hide
        Benjamin Eberlei added a comment -

        Terje Bråten I am very sorry that i cant promptly work on this, but I have not very much time lately. I don't get payed for Doctrine and do this in my free time, same for all the other contributors.

        To make another suggestion. Why are you not using Partial Objects? http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/partial-objects.html in combination with $em->refresh($partialuser);?

        I don't want to make this change more leniant again to be honest, and definately don't make it configurable. I had countless reports of people that got bitten by this very hard and lost hours of debugging just we didn't have this check. The way you used assocations was an undocumented hack and you can't expect this to work indefinately. You can either patch Doctrine yourself or try to fix the code to work with the latest version.

        Show
        Benjamin Eberlei added a comment - Terje Bråten I am very sorry that i cant promptly work on this, but I have not very much time lately. I don't get payed for Doctrine and do this in my free time, same for all the other contributors. To make another suggestion. Why are you not using Partial Objects? http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/partial-objects.html in combination with $em->refresh($partialuser);? I don't want to make this change more leniant again to be honest, and definately don't make it configurable. I had countless reports of people that got bitten by this very hard and lost hours of debugging just we didn't have this check. The way you used assocations was an undocumented hack and you can't expect this to work indefinately. You can either patch Doctrine yourself or try to fix the code to work with the latest version.
        Hide
        Terje Bråten added a comment - - edited

        Hi Benjamin.

        Thank you for your reply. I can take no for an answer, and I do appreciate that you develop Doctrine in your free time. (What I do not like is the feeling of being ignored when I do not get any response at all.)

        Thank you for pointing me in the direction of partial objects. I will have a look at it and see if that may solve my problem.

        So, just to clarify, do you object to making it configurable because it makes more work for you or do you have other reasons to object? If it is just the former, I would be happy to make the patch myself, if I can just be sure that it will be accepted when I am done.

        Thank you again for all the hard work you are putting into this project for the best of mankind. (Or at least that part of mankind that will be using Doctrine

        Terje

        Show
        Terje Bråten added a comment - - edited Hi Benjamin. Thank you for your reply. I can take no for an answer, and I do appreciate that you develop Doctrine in your free time. (What I do not like is the feeling of being ignored when I do not get any response at all.) Thank you for pointing me in the direction of partial objects. I will have a look at it and see if that may solve my problem. So, just to clarify, do you object to making it configurable because it makes more work for you or do you have other reasons to object? If it is just the former, I would be happy to make the patch myself, if I can just be sure that it will be accepted when I am done. Thank you again for all the hard work you are putting into this project for the best of mankind. (Or at least that part of mankind that will be using Doctrine Terje
        Hide
        Terje Bråten added a comment -

        I still do not like being ignored.

        Show
        Terje Bråten added a comment - I still do not like being ignored.
        Hide
        Marco Pivetta added a comment - - edited

        I personally think that double-checking at that location is a feature, and as Benjamin Eberlei exposed, it helps/helped a lot in avoiding hideous bugs (helped me too).

        Should not be removed. If your problem is mainly about performance and you know what you are doing, you can really go with partial objects. If you are sure that the objects already exist in your DB, you can even use things like

        $em->getReference($entity, $identifier)

        Does this fit your needs?

        Show
        Marco Pivetta added a comment - - edited I personally think that double-checking at that location is a feature, and as Benjamin Eberlei exposed, it helps/helped a lot in avoiding hideous bugs (helped me too). Should not be removed. If your problem is mainly about performance and you know what you are doing, you can really go with partial objects. If you are sure that the objects already exist in your DB, you can even use things like $em->getReference($entity, $identifier) Does this fit your needs?
        Hide
        Terje Bråten added a comment -

        Hi Marco,

        I guess my one unanswered question is the one about why it cannot be made configurable. Then both parties could be happy.

        So, what is the difference between $em->getReference($entity, $identifier)
        and $em->getPartialReference($entity, $identifier)?
        Is the latter one faster?

        Terje.

        Show
        Terje Bråten added a comment - Hi Marco, I guess my one unanswered question is the one about why it cannot be made configurable. Then both parties could be happy. So, what is the difference between $em->getReference($entity, $identifier) and $em->getPartialReference($entity, $identifier)? Is the latter one faster? Terje.
        Hide
        Marco Pivetta added a comment -

        Terje Bråten

        EntityManager#getReference

        returns proxies (when the entity is not already available)

        EntityManager#getPartialReference

        returns instances of your class with only the identifier set.

        Performance impact is negligible, since neither causes SQL to be thrown at your DB, but

        EntityManager#getPartialReference

        will produce instances incapable of lazy loading.

        Show
        Marco Pivetta added a comment - Terje Bråten EntityManager#getReference returns proxies (when the entity is not already available) EntityManager#getPartialReference returns instances of your class with only the identifier set. Performance impact is negligible, since neither causes SQL to be thrown at your DB, but EntityManager#getPartialReference will produce instances incapable of lazy loading.
        Hide
        Marco Pivetta added a comment -

        Unsupported usage of the ORM

        Show
        Marco Pivetta added a comment - Unsupported usage of the ORM
        Marco Pivetta made changes -
        Status Open [ 1 ] Closed [ 6 ]
        Resolution Invalid [ 6 ]

        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={query=DDC-2025, expand=changesets[0:20].revisions[0:29],reviews}, methodType=GET}] : Received status code 503 (Service Temporarily Unavailable)

          People

          • Assignee:
            Benjamin Eberlei
            Reporter:
            Terje Bråten
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: