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

Restore document proxy state to uninitialized on load exception #10645

Merged
merged 1 commit into from
Jun 9, 2023

Conversation

notrix
Copy link
Contributor

@notrix notrix commented Apr 21, 2023

Q A
Type bug
BC Break no
Fixed issues

Summary

I had an issue in mongodb-odm bundle with proxy left in initialized state on load exception and thought if same could be applied in orm bundle. Test shows same issue is here.

Related

doctrine/mongodb-odm#2521

@notrix notrix force-pushed the fix-proxy-state-on-load-exception branch from 6c049ce to d472e9a Compare June 8, 2023 12:48
@notrix notrix changed the base branch from 2.13.x to 2.14.x June 8, 2023 12:49
@derrabus derrabus changed the base branch from 2.14.x to 2.15.x June 8, 2023 15:28
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Sorry that this PR hasn't been picked up yet. You change looks reasonable.

Can you give a real-world example of what kind of failure could happen here? I wonder if we could build a functional test for this scenario. All this mocking in your test makes the scenario look artificial/hypothetical.

Comment on lines 160 to 163
$exception = $this
->getMockBuilder(ConnectionException::class)
->disableOriginalConstructor()
->getMock();
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to mock an exception class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well ConnectionException was the closest exception to the real-world scenario. And as it has a nasty constructor I have decided to mock it. It could be less complicated 👍
And for the example: in my experience we have never encountered this with ORM, but with ODM and native Client-Side Field Level Encryption we had this issue. As our supervisor consumers catches all exceptions and keep working on messages. On em->flush() proxies that were not initialized has been flushed to db removing all fields that had no default values.
After I made PR to ODM lib decided to check if the same is possible with ORM also. And theoretically it is possible.

@notrix notrix force-pushed the fix-proxy-state-on-load-exception branch from d472e9a to 58398f5 Compare June 9, 2023 05:34
@derrabus derrabus added the Bug label Jun 9, 2023
@derrabus derrabus added this to the 2.15.3 milestone Jun 9, 2023
@derrabus derrabus merged commit c3106f9 into doctrine:2.15.x Jun 9, 2023
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.

4 participants