Skip to content

Fix preFlush event documentation stating incorrectly that flush can be called safely#6858

Closed
egonolieux wants to merge 1 commit intodoctrine:masterfrom
egonolieux:master
Closed

Fix preFlush event documentation stating incorrectly that flush can be called safely#6858
egonolieux wants to merge 1 commit intodoctrine:masterfrom
egonolieux:master

Conversation

@egonolieux
Copy link
Copy Markdown

No description provided.

anything else. ``EntityManager#flush()`` can be called safely
inside its listeners.
anything else. ``EntityManager#flush()`` should never be called inside its listeners,
because it will cause an infinite recursive call.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we replace cause an infinite recursive call with result in an undefined behavior? That doesn't expose implementation and covers other potential issues as well.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, a more general explanation might be better indeed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also "should never be called inside its listeners" is misleading because it really depends on the type of listener you're using and how flush() is being called.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

What do you mean by "the type of the listener"? This is specifically about preFlush.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That comment was just wrong, sorry... too sleepy

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

No problem ;)

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