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 null comparisons in ManyToManyPersister #10587

Merged
merged 3 commits into from
Apr 12, 2023

Conversation

MatTheCat
Copy link
Contributor

@MatTheCat MatTheCat commented Mar 21, 2023

Fixes #5587, #5827 and #7717

The reason for

if (($operator === Comparison::EQ || $operator === Comparison::IS) && $value === null) {
return null;
} elseif ($operator === Comparison::NEQ && $value === null) {
return null;
}
was that BasicEntityPersister replaces null comparisons with IS [NOT] NULL so it does not need the corresponding parameter.

Problem is, ManyToManyPersister has then no longer access to the comparison.

My fix is to remove the condition in SqlValueVisitor and make each persister wary of null comparisons. It feels super weird but tests pass.

@MatTheCat MatTheCat changed the title Add test case for https://github.com/doctrine/orm/issues/7717 Handle null comparisons in ManyToManyPersister Mar 21, 2023
@derrabus derrabus added this to the 2.14.3 milestone Apr 12, 2023
@derrabus derrabus merged commit fca1ef7 into doctrine:2.14.x Apr 12, 2023
@MatTheCat MatTheCat deleted the 7717_testcase branch April 12, 2023 17:26
@MatTheCat
Copy link
Contributor Author

Thanks! #5827 and #7717 can be closed; I thought GitHub would do it automatically.

@greg0ire
Copy link
Member

I think you have to repeat Fixes every time for that to happen 🙂

@greg0ire greg0ire linked an issue Apr 12, 2023 that may be closed by this pull request
@MatTheCat
Copy link
Contributor Author

Indeed, just read https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword 😅

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.

isNull criteria doesn't work @ManyToMany relation matching criteria ignores isNull(column)
3 participants