Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

Commit

Permalink
Move flush to fire after event triggering.
Browse files Browse the repository at this point in the history
  • Loading branch information
@TomHAnderson
Tom Anderson authored and TomHAnderson committed Jun 6, 2015
1 parent ae528a6 commit f7965f8
Showing 1 changed file with 14 additions and 9 deletions.
23 changes: 14 additions & 9 deletions src/Server/Resource/DoctrineResource.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,12 +312,14 @@ public function create($data)
}

$this->getObjectManager()->persist($entity);
$this->getObjectManager()->flush();

$results = $this->triggerDoctrineEvent(DoctrineResourceEvent::EVENT_CREATE_POST, $entity);
if ($results->last() instanceof ApiProblem) {
return $results->last();
}

$this->getObjectManager()->flush();

return $entity;
}

Expand All @@ -343,13 +345,14 @@ public function delete($id)
}

$this->getObjectManager()->remove($entity);
$this->getObjectManager()->flush();

$results = $this->triggerDoctrineEvent(DoctrineResourceEvent::EVENT_DELETE_POST, $entity);
if ($results->last() instanceof ApiProblem) {
return $results->last();
}

$this->getObjectManager()->flush();

return true;
}

Expand Down Expand Up @@ -545,20 +548,21 @@ public function patch($id, $data)
}
// @codeCoverageIgnoreEnd

// Hydrate entity with patched data
$this->getHydrator()->hydrate((array) $data, $entity);

$results = $this->triggerDoctrineEvent(DoctrineResourceEvent::EVENT_PATCH_PRE, $entity);
if ($results->last() instanceof ApiProblem) {
return $results->last();
}

$this->getObjectManager()->flush();
// Hydrate entity with patched data
$this->getHydrator()->hydrate((array) $data, $entity);

$results = $this->triggerDoctrineEvent(DoctrineResourceEvent::EVENT_PATCH_POST, $entity);
if ($results->last() instanceof ApiProblem) {
return $results->last();
}

$this->getObjectManager()->flush();

return $entity;
}

Expand Down Expand Up @@ -591,19 +595,20 @@ public function update($id, $data)
}
// @codeCoverageIgnoreEnd

$this->getHydrator()->hydrate((array) $data, $entity);

$results = $this->triggerDoctrineEvent(DoctrineResourceEvent::EVENT_UPDATE_PRE, $entity);
if ($results->last() instanceof ApiProblem) {
return $results->last();
}

$this->getObjectManager()->flush();
$this->getHydrator()->hydrate((array) $data, $entity);

$results = $this->triggerDoctrineEvent(DoctrineResourceEvent::EVENT_UPDATE_POST, $entity);
if ($results->last() instanceof ApiProblem) {
return $results->last();
}

$this->getObjectManager()->flush();

return $entity;
}

Expand Down

7 comments on commit f7965f8

@kusmierz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomHAnderson why have you moved flush() in f7965f8? Now pre- and post- events are fired before they are actually in db... Maybe better idea would be to add prepersist/preflush events?

@TomHAnderson
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entity is passed by reference to the triggered events. They may change the entity so flush is after events.

@kusmierz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, in my mind, the entity could change in pre- events, so it will be saved changed. But post- events should be after flush - why you'd like to change it twice? Is there any difference between pre-persist and post-persist (in this particular case)?

another thing. Check EVENT_UPDATE_* events. The EVENT_UPDATE_PRE event will get clear entity (just from DB) before update... And next the entity will be hydrated. So it will be overwritten by hydrator (correct me if I'm wrong)...

Nevertheless - if you decide to leave it as is, could you add EVENT_*_FLUSH events? Thanks in advance!

@TomHAnderson
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy where the PRE and POST are now. For your suggestion of _FLUSH events, which I think would be fine, where do you want them, post flush? That seems like a fine place for a new event

Do you mind submitting a PR?

@kusmierz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about "post" flush ("pre-flush" is in fact update/create/etc _POST). Sure, I'm going to do PR in a moment.

BTW: I think it would be a good idea to add this change to changelog as BC or at least note it in readme. In fact it broke my code...

@kusmierz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for the record here: see #200.

@stefliekens
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this.

Please sign in to comment.