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

Infer types in OneToManyPersister::deleteEntityCollection #10075

Closed
wants to merge 1 commit into from

Conversation

norkunas
Copy link
Contributor

Similar to #8401

In our app we are using Symfony Uuid and when collection needs to be cleared the deletion query is executed without types therefore Uuid casts to string instead of to binary and no relations are deleted.

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.

Thank you. Sorry that we're picking up your PR that late.

$updates = $conn->getExecuteStatements();
$lastStatement = array_pop($updates);

self::assertSame('DELETE FROM onetomanypersister_child WHERE parent_id = ?', $lastStatement['sql']);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self::assertSame('DELETE FROM onetomanypersister_child WHERE parent_id = ?', $lastStatement['sql']);
self::assertSame('DELETE FROM onetomanypersister_child WHERE parent_id = ?', $lastStatement['sql']);

Running assertions on generated SQL is always brittle. We should run the test against a real database and assert that the given record has been deleted properly.

Copy link
Contributor Author

@norkunas norkunas Oct 26, 2022

Choose a reason for hiding this comment

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

There was no tests at all for this persister,so I've just copied from another one and adapted it.. will see If I could manage to add a better one.. Do you have any suggestions where I should add it?

@derrabus derrabus added the Bug label Oct 26, 2022
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.

2 participants