Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update events.rst to reflect behaviour of preUpdate #1429

Merged
merged 1 commit into from Jun 17, 2015
Merged

Update events.rst to reflect behaviour of preUpdate #1429

merged 1 commit into from Jun 17, 2015

Conversation

ghost
Copy link

@ghost ghost commented Jun 17, 2015

preUpdate is only triggered when there any changes made on the target entity. The documentation does not mention that as far as I can see.

@ghost
Copy link
Author

ghost commented Jun 17, 2015

Please correct me if I am wrong but I think this line of code is the reason it is working in this way.

@@ -164,7 +164,8 @@ the life-time of their registered entities.
database insert operations. Generated primary key values are
available in the postPersist event.
- preUpdate - The preUpdate event occurs before the database
update operations to entity data. It is not called for a DQL UPDATE statement.
update operations to entity data. It is not called for a DQL UPDATE statement
nor when the computed changeset is empty.
Copy link
Member

Choose a reason for hiding this comment

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

empty changeset === no update

Maybe some other precondition was unclear to you?

Copy link
Author

Choose a reason for hiding this comment

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

empty changeset === no update

Maybe I misunderstand you but that is what I mean. This behaviour is not documented as far as I can tell.

This is the only precondition I can think of right now that seem to be undocumented but maybe there are others.

Copy link
Member

Choose a reason for hiding this comment

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

Agree. I'll merge anyway, but consider hunting these down if you can :-)

I think we're missing a chapter on "ORM assuptions" or such...

Copy link
Author

Choose a reason for hiding this comment

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

To clarify: when reading the documentation as it is now it is not obvious that one of the preconditions is that one more or fields need to be modified in order for this to trigger.

Copy link
Author

Choose a reason for hiding this comment

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

I agree. I will open up new pull requests if I find more of these. Thanks for the feedback.

@Ocramius Ocramius self-assigned this Jun 17, 2015
Ocramius added a commit that referenced this pull request Jun 17, 2015
Update events.rst to reflect behaviour of preUpdate
@Ocramius Ocramius merged commit 37a409a into doctrine:master Jun 17, 2015
@Ocramius
Copy link
Member

👍

1 similar comment
@TomasVotruba
Copy link
Contributor

👍

@lcobucci lcobucci added this to the 2.6.0 milestone Dec 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants