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

Split the ORMException class #8692

Merged
merged 1 commit into from
Jun 15, 2021
Merged

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented May 15, 2021

This is a "backport" of #6743

I'm using quotes here because it's different in that it should be backwards-compatible, and a subsequent backport of #6473 should happen on 3.0.x if this gets merged.

Todo

Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

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

Should the old ORMException be tagged as @internal or @deprecated (not sure which one is better) so that static analysis tools report its usage in catch clauses ? Otherwise, it will have to stay forever.
Also, existing factory methods in ORMException should be deprecated

namespace Doctrine\ORM\Exception;

/**
* This interface should be implemented by all exceptions in the Repository
Copy link
Member

Choose a reason for hiding this comment

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

this is not an interface.

And if you intend to turn it into an interface in 3.0, I suggets making it an abstract class so that users cannot instantiate it.

Copy link
Member

Choose a reason for hiding this comment

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

btw, this could be introduced as an interface directly, with the actual extensions extending from the old class (for BC) and implementing the new extension.

@greg0ire
Copy link
Member Author

I'd say let's mark it as @deprecated, marking it as @internal would make BC-breaks in it OK, when those BC-breaks shouldn't happen really.

@greg0ire greg0ire closed this May 21, 2021
@greg0ire greg0ire reopened this May 21, 2021
@greg0ire greg0ire force-pushed the split_orm_exception branch 3 times, most recently from dc3db86 to 4efcd2e Compare May 31, 2021 20:18
@greg0ire greg0ire marked this pull request as ready for review May 31, 2021 20:26
@greg0ire greg0ire changed the base branch from 2.9.x to 2.10.x June 5, 2021 21:59
ostrolucky
ostrolucky previously approved these changes Jun 14, 2021
SenseException
SenseException previously approved these changes Jun 15, 2021
@greg0ire
Copy link
Member Author

Rebased as I don't understand why there were 14 commits…

@greg0ire greg0ire merged commit 08149ea into doctrine:2.10.x Jun 15, 2021
@greg0ire greg0ire deleted the split_orm_exception branch June 15, 2021 21:43
@greg0ire greg0ire added this to the 2.10.0 milestone Jun 15, 2021
@BenoitDuffez
Copy link
Contributor

Hi, prob not the best place to ask this, but I'm wondering why the old exception was deprecated when the entity manager still has it in its annotation? For example EntityManager::persist has @throws ORMException which is not covered by a use statement, so the Doctrine\ORM\ORMException is used.

Now I have two options (or both at the same time)

  • have my IDE warn me I'm trying to catch a deprecated exception (if I catch the old one)
  • have my IDE warn me about an uncaught exception (if I catch the new one)

Either way I don't even know which exception is thrown, so I'm not very confident in all my try/catches around Doctrine.

Is this covered by a document that I would have missed? I checked the upgrade document at the root of this repo but didn't find the info I was looking for.

I'm currently using 2.11.2.

@greg0ire
Copy link
Member Author

greg0ire commented Apr 1, 2022

@BenoitDuffez indeed not the best place.

I'm wondering why the old exception was deprecated when the entity manager still has it in its annotation?

As you can see, this was a big PR I made ages ago, and I've probably made some mistakes. Please send a PR replacing that annotation with what seems correct from exploring all the possibilities, a quick look shows that there aren't that many.

@BenoitDuffez
Copy link
Contributor

@greg0ire sure! Sorry about the way I asked. Thanks for replying so rapidly.

Here's the PR: #9623. I did limited testing and now my IDE is happy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants