It seems that the identity map in v2.2 and 2.3 of doctrine was missing the not dirty entities.

It seems that the identity map in v2.2 and 2.3 of doctrine was missing the not dirty entities.

Could you explain this a bit more i'm not sure why the tests are failing know.

I added a new test to cover the new feature to collect events of "not dirty" entities.
But this test is failing in odler versions 2.2 & 2.3.
After checking out the tests in these version, it's because Doctrine only triggers the PreFlush event when nothing needs to be committed to the db. In later versions this has been fixed and the OnFlush and PostFlush are triggered in any case.

I moved all recording calls in the PreFlush event to fix this.

Coverage remained the same at 100.0% when pulling 70f098a on GeoffroyR:master into 3e93d5d on SimpleBus:master.

@GeoffroyR So if i'm correct this is ready to be merged. I only think we should create a major release for this to not break old implementations. What do you think?

why not listening "postFlush" ? (just curious)

@cmodijk should it be considered a major release? It might break existing implementations, but only because they didn't work in the first place. I think this is more a bugfix than a major, but heh, that's only my opinion :)
Anyway, I'm looking forward seeing this merged, as I'm currently facing this exact same issue.
Thanks for this!

Sorry but this still needs some more testing.

• I had an issue with some entities not being in the UOW in the preflush but only in the postflush so we should also subscribe to the postflush event and collect the event form the UOW entities.
• I think collections should also be checked and see if we collect the events of these correctly.

I have no time right now but will try to find some time next month to add a series of tests.

@docteurklein "postFlush" is not called if nothing is committed in previous versions of Doctrine 2.2 & 2.3

I have another issue related to this PR:
it will trigger Lazy Loading to check if an Entity contains recorded messages - or require all Entities in an object graph to be hydrated beforehand.

Scenario:
Entity1 (E1) and Entity2 (E2) are both instances of ContainsRecordedMessages - E1 has a ManyToOne relation to E2.

I fetch the E1 from the repository (E2 is not hydrated for the current task), change something on the E1 and record an Event. E2 is still just a Doctrine Proxy object and can per se not contain any messages.

The check at collectEventsFromEntity will no iterate over the managed entities, see E2 and try to fetch the messages from the Entity. This will trigger Doctrine Lazy Loading - which is not required since a uninitialised proxy object will not be able to contain any messages.

Suggestion:

• check if the object is a proxy or not
• verify that it is initialised if it is a proxy object

private function collectEventsFromEntity($entity)
{
   if ($entity instanceof ContainsRecordedMessages
        && ( !$entity instanceof Proxy 
             || ($entity instanceof Proxy && $entity->__isInitialized__)
       )
   ) {
      foreach ($entity->recordedMessages() as $event) {
          $this->collectedEvents[] = $event;
      }
      $entity->eraseMessages();
   }
}

@cmodijk should it be considered a major release? It might break existing implementations, but only because they didn't work in the first place. I think this is more a bugfix than a major, but heh, that's only my opinion :)

@docteurklein You are right but that also means that if someone already implemented a workaround this (like i did myself) is going to be surprised if it is fixed without a major bump i think. And i'm working on the next major versions of al packages to drop support for php 5.4 and 5.5 and I hope i can use this in that.

I had an issue with some entities not being in the UOW in the preflush but only in the postflush so we should also subscribe to the postflush event and collect the event form the UOW entities.

Do you know why these entities where not in the preflush ?

I think collections should also be checked and see if we collect the events of these correctly.

I'm not sure but about this do you have a test case for this?

check if the object is a proxy or not verify that it is initialised if it is a proxy object

@patrickjahns tnx for the suggestion.

@GeoffroyR Are you able to finish this or should i try to make the final changes in the coming weeks?

Codecov Report

❗ No coverage uploaded for pull request base (master@3e93d5d). Click here to learn what that means.
The diff coverage is 95.45%.

@@            Coverage Diff            @@
##             master       #4   +/-   ##
=========================================
  Coverage          ?   97.56%           
=========================================
  Files             ?        2           
  Lines             ?       41           
  Branches          ?        0           
=========================================
  Hits              ?       40           
  Misses            ?        1           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e93d5d...96fc781. Read the comment docs.

@cmodijk

Do you know why these entities where not in the preflush ?

I checked back why I had to add the PostFlush and it is related to the lifecycle events. I recorderded some events based on lifecycle events and these were not collected on preFlush.

About the collections, I was a bit concerned about cascade events but I did some tests and could not find any caveat.

Hey everyone, great that this issue receives attention! I do think it requires a major version bump, since otherwise the behavior will be too subtle for people simply upgrading. It's something that needs to be documented well too.
With regard to collections: I assumed that entities in collections are themselves registered in the identity map by their ID.
Lazy loading is an interesting case, I don't have anything to add really.

I agree with @matthiasnoback regarding collections: I don't think you have anything to check manually.

I checked back why I had to add the PostFlush and it is related to the lifecycle events. I recorderded some events based on lifecycle events and these were not collected on preFlush.

Oke, but I think the pull request is fine then right? I should be able to merge this now?

Sorry, wrong button.

I might be wrong (I didn't make the test but I looked at a generated doctrine proxy) and I don't think checking on initialized proxy has any effect because proxies only initialize if you call methods that touch persisted properties.

Can you confirm this behavior?
Anyway, I would personnally be OK with merging this as it provides a correct and useful behavior.

I might be wrong (I didn't make the test but I looket at a generated doctrine proxy) and I don't think checking on initialized proxy has any effect because proxies only initialize if you call methods that touch persisted properties.

From my testing, any method invocation on a unitiliazed proxy object will trigger initialization. (Anything otherwise would cause problems / inconsistencies)

@docteurklein When you retrieve a Entity with a reference to a other Entity but only fetch the date of the first one the second one still gets a instance with a proxy implementation. When we then in our listener call the recordedMessages() on the method these proxy objects they get loaded but we did not do any work on them so that is unnecessary and could resolve into other problems.

ha ok thanks for the explanations!

@docteurklein No problem :-)

Ping @GeoffroyR i think this can get merged right?

I tested the basic concept today i'm merging this now and releasing in a bit later today.