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

Relax exception check for unique constraints #136

Closed
wants to merge 1 commit into from

Conversation

gbirke
Copy link
Member

@gbirke gbirke commented Aug 3, 2023

This is a temporary workaround for the changes made in
Doctrine ORM 2.16, discussed in
doctrine/orm#10785

When that discussion is resolved, we should remove the check for
RuntimeException, because it might be thrown by other errors that have
nothing to do with duplicate IDs, which would lead to our
PaymentOverrideException hinting at the wrong thing.

Also remove the PHPStan rule that had an exception for this check.
When the Exception comes back, the method should have an annotation that
it's thrown. Otherwise we'll have to put the PHPStan rule back or try
out phpstan-doctrine where this has been solved:
phpstan/phpstan-doctrine#315

This is a temporary workaround for the changes made in
Doctrine ORM 2.16, discussed in
doctrine/orm#10785

When that discussion is resolved, we should remove the check for
`RuntimeException`, because it might be thrown by other errors that have
nothing to do with duplicate IDs, which would lead to our
`PaymentOverrideException` hinting at the wrong thing.

Also remove the PHPStan rule that had an exception for this check.
When the Exception comes back, the method should have an annotation that
it's thrown. Otherwise we'll have to put the PHPStan rule back or try
out phpstan-doctrine where this has been solved:
phpstan/phpstan-doctrine#315
@gbirke
Copy link
Member Author

gbirke commented Aug 3, 2023

Wrong approach, right fix is in #137

@gbirke gbirke closed this Aug 3, 2023
@gbirke gbirke deleted the relax-exception-check branch August 3, 2023 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant