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

Handle exceptions on entity flush #36

Merged
merged 2 commits into from
Jul 12, 2024
Merged

Conversation

dlubitz
Copy link
Contributor

@dlubitz dlubitz commented Jul 11, 2024

Fixes #35

The entity states are changing during the single flushes of entities. And if we have to remove more than one entity this crashes now, due to the change in doctrine/orm.

In general I think we should get rid of the manually handling of entity persistance, as we can see in this case.

For now we check this state, before flushing to ensure, we do not try to flush a detatched entity.

@dlubitz dlubitz self-assigned this Jul 11, 2024
@dlubitz dlubitz added the bug label Jul 11, 2024
Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

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

Yes, but… see question on the issue.

@dlubitz
Copy link
Contributor Author

dlubitz commented Jul 12, 2024

I've changed it as suggested to try/catch

@kdambekalns
Copy link
Member

Gnah… InvalidArgumentException is all we get here? That is sad… just swallowing these unconditionally might lead to hard-to-find issues, if not now, then later. 🤔

@dlubitz
Copy link
Contributor Author

dlubitz commented Jul 12, 2024

Yep, that's all we get. 🤷‍♂️

@kdambekalns kdambekalns requested a review from kitsunet July 12, 2024 08:14
@kdambekalns
Copy link
Member

@kitsunet What do you think?

@kdambekalns
Copy link
Member

And the PHP version used for the tests needs to be raised… 🙈

@kitsunet
Copy link
Member

I think we really shouldn't deal with te UOW at all, and need to refactor htis, but it's fine for me as a quick fix.

@kitsunet
Copy link
Member

by reading, I read the issues in doctrine and see how this works, not sure if there might be any side effects.

@dlubitz dlubitz changed the base branch from main to 5.0 July 12, 2024 11:17
@dlubitz dlubitz changed the title Check state of entity before flushing Handle exceptions on entity flush Jul 12, 2024
@kdambekalns kdambekalns reopened this Jul 12, 2024
@kdambekalns kdambekalns force-pushed the bugfix/35-entity-flush branch from 67e138d to c7c8db4 Compare July 12, 2024 11:38
@dlubitz dlubitz force-pushed the bugfix/35-entity-flush branch from c7c8db4 to 87783d7 Compare July 12, 2024 12:43
@dlubitz dlubitz merged commit cbcf6c5 into neos:5.0 Jul 12, 2024
2 checks passed
@dlubitz dlubitz deleted the bugfix/35-entity-flush branch July 12, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Entity has to be managed or scheduled for removal for single computation" on publish moved nodes
3 participants