Doctrine 2 - ORM
  1. Doctrine 2 - ORM
  2. DDC-203

Detached entities are recognized as new ones

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0-ALPHA3
    • Fix Version/s: 2.0-BETA3
    • Component/s: ORM
    • Security Level: All
    • Labels:
      None
    • Environment:
      Cli tests with PHPUnit
      PHP 5.3.1 (cli) (built: Nov 27 2009 10:08:49)
      PHPUnit 3.4.1

      Description

      From
      http://www.doctrine-project.org/documentation/manual/2_0/en/working-with-objects
      on using persist() on an entity X:

      1. If X is a detached entity, an InvalidArgumentException will be thrown.

      So I do this:
      $picard = $this->_getUser('Picard');
      $this->_em->persist($picard);
      $this->_em->flush();
      $this->_em->detach($picard);
      $this->_em->persist($picard);
      $this->_howMany("Picard", 1);
      Result:
      1) NakedPhp\Storage\DoctrineTest::testSavesUpdatedEntities
      There are 2 instances of Picard saved instead of 1.
      Failed asserting that <string:2> matches expected <integer:1>.
      The entity is recognized as new instead of detached, so no error is thrown and it is duplicated in the database.

      1. ddc-203.patch
        3 kB
        Giorgio Sironi
      2. ddc203-beberlei.patch
        1 kB
        Benjamin Eberlei
      3. ddc203-beberlei-update.patch
        3 kB
        Benjamin Eberlei

        Issue Links

          Activity

          Hide
          Roman S. Borschel added a comment -

          The problem I have with this is that by doing this, every single entity passed to persist() gets its identifier read out using reflection just to determine whether its really new.

          I would rather change the contract and the documentation to clearly indicate that detached instances must not be passed to persist().

          Show
          Roman S. Borschel added a comment - The problem I have with this is that by doing this, every single entity passed to persist() gets its identifier read out using reflection just to determine whether its really new. I would rather change the contract and the documentation to clearly indicate that detached instances must not be passed to persist().
          Hide
          Giorgio Sironi added a comment -

          There are no problems in a stricter contract for persist() if detached instances are kept separate from new ones in applications, which is usually true since they come from a cache or a store. I would go with the change as it's the simplest reliable solution.

          Show
          Giorgio Sironi added a comment - There are no problems in a stricter contract for persist() if detached instances are kept separate from new ones in applications, which is usually true since they come from a cache or a store. I would go with the change as it's the simplest reliable solution.
          Hide
          Roman S. Borschel added a comment - - edited

          I updated the documentation to be more clear about this issue: http://www.doctrine-project.org/documentation/manual/2_0/en/working-with-objects:persisting-entities

          What do you think?

          The problem is, we can not do it reliably for all cases. Yes, we could look into the identifier property if the identifier is generated and conclude from that whether it is detached or not but that does not work for manually assigned identifiers. For these we would need to hit the database and I just don't think we can do that.

          So right now changing the persist contract as I did in the documentation, explicitly stating that detached entities should not be passed to persist() and that the behavior is undefined if done so, is the "best" thing.

          Tell me what you think. The alternative would be to do the above (peek into identifier field of generated identifiers or look the identifier up in the db if it is assigned manually). I'm pretty sure the JPA implementations do just that but I dont know for sure and I dont think we want that.

          I am still undecided though Something inside me prefers to go the other route, even if it means some additional db hits for manually assigned identifiers. Please make your vote.

          Show
          Roman S. Borschel added a comment - - edited I updated the documentation to be more clear about this issue: http://www.doctrine-project.org/documentation/manual/2_0/en/working-with-objects:persisting-entities What do you think? The problem is, we can not do it reliably for all cases. Yes, we could look into the identifier property if the identifier is generated and conclude from that whether it is detached or not but that does not work for manually assigned identifiers. For these we would need to hit the database and I just don't think we can do that. So right now changing the persist contract as I did in the documentation, explicitly stating that detached entities should not be passed to persist() and that the behavior is undefined if done so, is the "best" thing. Tell me what you think. The alternative would be to do the above (peek into identifier field of generated identifiers or look the identifier up in the db if it is assigned manually). I'm pretty sure the JPA implementations do just that but I dont know for sure and I dont think we want that. I am still undecided though Something inside me prefers to go the other route, even if it means some additional db hits for manually assigned identifiers. Please make your vote.
          Hide
          Giorgio Sironi added a comment -

          If an identifier is a natural key, and not generated, a detached object of that class and a new one are really indistinguishable if they have the same values on the other columns.
          I am for simplicity: the detached/merge entities are related to use cases like caches, sessions and serialization of every kind. So typically you in an application you always know when they come from such a source and can call merge() appropriately.
          If calling persist() erroneously will result in throwing an exception on flush() (primary key value already present), the application developer will notice the problem.

          Show
          Giorgio Sironi added a comment - If an identifier is a natural key, and not generated, a detached object of that class and a new one are really indistinguishable if they have the same values on the other columns. I am for simplicity: the detached/merge entities are related to use cases like caches, sessions and serialization of every kind. So typically you in an application you always know when they come from such a source and can call merge() appropriately. If calling persist() erroneously will result in throwing an exception on flush() (primary key value already present), the application developer will notice the problem.
          Hide
          Roman S. Borschel added a comment -

          Pushing final resolution back to beta3. The problem is that this missing detection of detached objects can lead to weird errors in other cases as well. Need to think about it some more.

          Show
          Roman S. Borschel added a comment - Pushing final resolution back to beta3. The problem is that this missing detection of detached objects can lead to weird errors in other cases as well. Need to think about it some more.

            People

            • Assignee:
              Roman S. Borschel
              Reporter:
              Giorgio Sironi
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: