Split `ORMException` into more fine-grained, better documented and typed exception classes #6743

greg0ire · 2017-10-02T17:53:34Z

This class is too big, and some exceptions could inherit from doctrine/common exceptions instead of this one.

TODO

Proposal

method	used in class	should implement
`notSupported`	`Doctrine\ORM\Tools\SchemaTool`	`SchemaToolException`
`namedNativeQueryNotFound`	`Doctrine\ORM\Configuration`	`ConfigurationException`
`namedQueryNotFound`	`Doctrine\ORM\Configuration`	`ConfigurationException`
`proxyClassesAlwaysRegenerating`	`Doctrine\ORM\Configuration`	`ConfigurationException`
`invalidEntityRepository`	`Doctrine\ORM\Configuration`	`ConfigurationException`
`unknownEntityNamespace`	`Doctrine\ORM\Configuration`	`ConfigurationException`
`entityManagerClosed`	`Doctrine\ORM\EntityManager`	`ManagerException`
`entityMissingAssignedIdForField`	no usages	/dev/null
`entityMissingForeignAssignedId`	no usages	/dev/null
`invalidHydrationMode`	`Doctrine\ORM\EntityManager`	`ManagerException`
`mismatchedEventManager`	`Doctrine\ORM\EntityManager`	`ManagerException`
`missingIdentifierField`	`Doctrine\ORM\EntityManager`	`ManagerException`
`missingMappingDriverImpl`	`Doctrine\ORM\EntityManager`	`ManagerException`
`unrecognizedIdentifierFields`	`Doctrine\ORM\EntityManager`	`ManagerException`
`cantUseInOperatorOnCompositeKeys`	`Doctrine\ORM\Persisters\Entity\BasicEntityPersister`	`PersisterException`
`invalidFlushMode`	no usages	/dev/null
`invalidOrientation`	`Doctrine\ORM\Persisters\Entity\BasicEntityPersister`	`PersisterException`
`unexpectedAssociationValue`	`Doctrine\ORM\UnitOfWork`	???
`unrecognizedField`	`Doctrine\ORM\Persisters\Entity\BasicEntityPersister`	`PersisterException`
`findByRequiresParameter`	`Doctrine\ORM\EntityRepository`	`RepositoryException`
`invalidFindByCall`	no usages	/dev/null
`invalidMagicCall`	`Doctrine\ORM\EntityRepository`	`RepositoryException`
`invalidFindByInverseAssociation`	`Doctrine\ORM\Persisters\Entity\BasicEntityPersister`	`RepositoryException`
`invalidResultCacheDriver`	`Doctrine\ORM\AbstractQuery`	`CacheException`
`queryCacheNotConfigured`	`Doctrine\ORM\Configuration`	`CacheException`
`metadataCacheNotConfigured`	`Doctrine\ORM\Configuration`	`CacheException`
`queryCacheUsesNonPersistentCache`	`Doctrine\ORM\Configuration`	`CacheException`
`metadataCacheUsesNonPersistentCache`	`Doctrine\ORM\Configuration`	`CacheException`

Ocramius · 2017-10-03T12:57:42Z

@greg0ire the split is indeed needed (see doctrine/common#681), but splitting just per component is a bit silly.

These are (almost) all separate exceptions, implementing a component-specific exception interface (for example, CacheException), and extend ORMException (need to do that for BC compliance).

For example:

class InvalidRepositoryMagicMethodCall extends ORMException implements RepositoryException
{
    public static function fromMethodCallAndArguments(string $method, array $arguments) : self {
        ...
    }
}

In an ideal world, this should be:

class InvalidRepositoryMagicMethodCall extends \InvalidArgumentException implements RepositoryException
{
    public static function fromMethodCallAndArguments(string $method, array $arguments) : self {
        ...
    }
}

Moving away from ORMException would be quite a BC break, so I don't think we can totally fix this.

greg0ire · 2017-10-03T13:14:06Z

Moving away from ORMException would be quite a BC break, so I don't think we can totally fix this.

@Ocramius My plan was to make ORMException an interface, and have all the split exceptions implement that interface. Is that a big BC-break? It's not for instanceof, or, more importantly, catch.

So to sum up, would that be ok with you:

class InvalidRepositoryMagicMethodCall extends \InvalidArgumentException implements RepositoryException, ORMException
{
    public static function fromMethodCallAndArguments(string $method, array $arguments) : self {
        ...
    }
}

Ocramius · 2017-10-03T13:17:03Z

@greg0ire given that the constructors would move around anyway, having an ORMException as an interface would be OK

greg0ire · 2017-10-03T13:20:50Z

Ok so if I understand correctly, I should move every method to its separate exception class, and use the split I proposed to create interfaces instead of classes. I'll try to push an example soon based on the "ideal world" snippet you gave + implements ORMException for BC

Ocramius · 2017-10-03T13:24:30Z

@greg0ire the implements ORMException can be done on the namespace-specific exception marker interface:

namespace Doctrine\ORM\Repository;

interface RepositoryException extends \Throwable, \Doctrine\ORM\ORMException
{
}

greg0ire · 2017-10-03T14:03:15Z

Even better!

Majkl578 · 2017-10-03T16:53:45Z

ORMException being an interface was exactly my idea.

namespace Doctrine\ORM;

interface ORMException extends \Throwable {}

Then specific exceptions would implement it (or have its own interface, as shown above):

namespace Doctrine\ORM\Mapping;

use  Doctrine\ORM\ORMException;

interface MappingException extends ORMException {}

class BlahException extends \RuntimeException implements MappingException {}

greg0ire · 2017-10-03T18:12:40Z

lib/Doctrine/ORM/NewORMException.php

+/**
+ * This interface should be implemented by all exceptions in this package.
+ */
+interface NewORMException extends \Throwable


Will of course be refactored to ORMException

greg0ire · 2017-10-03T18:13:09Z

lib/Doctrine/ORM/Repository/InvalidMagicMethodCall.php

+
+use Doctrine\ORM\RepositoryException;
+
+final class InvalidMagicMethodCall extends \InvalidArgumentException implements RepositoryException


I made the exception final

I removed the Repository bit from the name since it already is in the namespace.

Was InvalidArgumentException chosen instead of BadMethodCallException because of BC or because magic methods are getting the method name as an argument?

I don't know, I copy pasted a snippet from @Ocramius but BadMethodCallException might be better indeed: it's not about the arguments being invalid, it's about the field not existing.

greg0ire · 2017-10-03T18:13:42Z

lib/Doctrine/ORM/Repository/InvalidMagicMethodCall.php

+
+declare(strict_types=1);
+
+namespace Doctrine\ORM\Repository;


I put the exception in the namespace of the relevant component.

greg0ire · 2017-10-03T18:15:00Z

lib/Doctrine/ORM/RepositoryException.php

+
+declare(strict_types=1);
+
+namespace Doctrine\ORM;


I put that at the top level namespace to avoid the repetition. An alternative would be to name the class Doctrine\ORM\Repository\Exception, tell me what you prefer.

We have a PersisterException in Doctrine\ORM\Persisters namespace. Also a CacheException in Doctrine\ORM\Cache. If we keep that structure, it suggests that RepositoryException will be in Doctrine\ORM\Repository.

I would like to see a structure as you suggested with Doctrine\ORM\Repository\Exception, even with the already existing exception classes, but I don't think that this would be an anticipated change.

greg0ire · 2017-10-03T18:18:43Z

lib/Doctrine/ORM/Repository/InvalidMagicMethodCall.php

+
+final class InvalidMagicMethodCall extends \InvalidArgumentException implements RepositoryException
+{
+    public static function fromEntityNameFieldNameAndMethod(


This method name is a bit long, I'm open to proposals

fieldNameNotFound or fieldNotFound because it's the reason for not being able to use a magic method in an entity.

So no from?

The static name may lack fluent reading because of a missing method prefix like "from". Sounds funny with "because" InvalidMagicMethodCall::becauseFieldNotFoundIn, but I like "In" as suffix. Maybe the suffix can make it more readable without having needed arguments in the method name.

greg0ire · 2017-10-04T13:44:35Z

lib/Doctrine/ORM/Repository/InvalidMagicMethodCall.php

+    public static function fromEntityNameFieldNameAndMethod(
+        string $entityName,
+        string $fieldName,
+        $method


Woops I forgot the typehint here

greg0ire · 2017-10-06T18:15:05Z

lib/Doctrine/ORM/Repository/InvalidFindByInverseAssociation.php

+
+use Doctrine\ORM\RepositoryException;
+
+final class InvalidFindByInverseAssociation extends \BadMethodCallException implements RepositoryException


I'm not a huge fan of this class name. I know I created it myself, but it's derived from the former method name. What I don't like is that it kind of implies there could be valid find by inverse association method calls. Should I drop the Invalid part?

I guess the name can be discussed in combination with the exception methods. I suggest InvalidAssociation or InvalidAssociationSide. I've read the exception message in the tests and used the mentioned names for it.

InvalidAssociationSide sounds great

But the other one leaves more room for other named constructors.

greg0ire · 2017-10-06T18:16:00Z

lib/Doctrine/ORM/Repository/InvalidFindByInverseAssociation.php

+
+final class InvalidFindByInverseAssociation extends \BadMethodCallException implements RepositoryException
+{
+    public static function becauseIsInverseAssociation(


@Ocramius @SenseException can you please agree on a "because" vs "from" naming recommendation before I migrate all the exceptions?

Like I previously wrote, "because" sounds too funny, so I can agree on a different prefix. If we go with my exception name suggestions of a previous comment about this class name, we can create a method like InvalidAssociationSide::fromInverseSide() or InvalidAssociation::fromInverseSideUsage(). We can recombine method name and class name between those two method-suggestions, if you like class name or method name, but not the other one.

I don't think there is a point to using named constructors if we don't have several of them (or at least, leave room for more of them). I'll therefore go with your second proposition: classname = issue, and method name = cause of the issue.

Or rather, InvalidFindByCall::fromInverseSideUsage

I think I'm gonna cram findByRequiresParameter in there, Ocramius said those should be almost all separate exceptions, I think using the "almost" card here is right.

fromMissingParameter sounds weird though, maybe because is not that bad 🤔

How about onMissingParameter, onInverseSideUsage?

I think I'm gonna cram findByRequiresParameter in there, Ocramius said those should be almost all separate exceptions, I think using the "almost" card here is right.

as @ostrolucky pointed out, this one is not about findBy, but about a magic call, so I guess I'm gonna put it in InvalidMagicCall instead

EvilMagicCall 😆
It sounds right that it belongs into an exception for magic calls.
About just one needed named constructor: They become or already are a standard in Doctrine, so it would be weird if we have one case where an instance was created with new. Especially if that exception class might need a second exception case one day, we have to look that the one with new will get an own named constructor too.

SenseException · 2017-10-06T18:57:17Z

tests/Doctrine/Tests/ORM/Functional/EntityRepositoryTest.php

@@ -683,7 +684,7 @@ public function testCanRetrieveRepositoryFromClassNameWithLeadingBackslash()
    /**
     * @group DDC-1376
     *
-     * @expectedException Doctrine\ORM\ORMException
+     * @expectedException Doctrine\ORM\Repository\InvalidFindByInverseAssociation
     * @expectedExceptionMessage You cannot search for the association field 'Doctrine\Tests\Models\CMS\CmsUser#address', because it is the inverse side of an association.


I just realized it now, that one test is using $this->expectException() and $this->expectExceptionMessage() while we have annotations here.

I think annotations should be completely dropped in favor of those method calls :P

greg0ire · 2017-10-06T21:17:26Z

Wait why is invalidFindByInverseAssociation only called in BasicEntityPersister? Does it really have to do with findBy?

greg0ire · 2017-10-07T09:28:13Z

Wait why is invalidFindByInverseAssociation only called in BasicEntityPersister? Does it really have to do with findBy?

Looking at the unit tests, it seems this exception is indeed supposed to be thrown when using findBy on a repo, so I guess it is ok.

Majkl578 · 2017-12-08T02:41:25Z

Needs a rebase.
Also I think all exceptions should be suffixed by Exception, at least for consistency.

Ocramius

This goes in as-is 👍

The BC breaks to be documented are to be added to the UPGRADE.md, but I'm raising a separate issue for that.

Code review comments addressed.

Ocramius · 2018-04-13T15:21:02Z

Thanks @greg0ire! \o/ #7194

greg0ire · 2018-04-13T15:39:18Z

Thanks a lot for merging! Will list the BC-break this week-end.

Majkl578 · 2018-05-01T23:27:08Z

@greg0ire Any update on documentating these changes?

Majkl578 · 2018-05-01T23:29:37Z

Also what do we do with all the exceptions left in root namespace?

lib/Doctrine/ORM/EntityNotFoundException.php
lib/Doctrine/ORM/NonUniqueResultException.php
lib/Doctrine/ORM/NoResultException.php
lib/Doctrine/ORM/OptimisticLockException.php
lib/Doctrine/ORM/ORMInvalidArgumentException.php
lib/Doctrine/ORM/PessimisticLockException.php
lib/Doctrine/ORM/TransactionRequiredException.php
lib/Doctrine/ORM/UnexpectedResultException.php

Current state is highly inconsistent.

greg0ire force-pushed the split_orm_exception branch from 2169905 to 7898d3d Compare October 2, 2017 17:54

greg0ire commented Oct 3, 2017

View reviewed changes

greg0ire commented Oct 4, 2017

View reviewed changes

greg0ire force-pushed the split_orm_exception branch from 7b6c3eb to 897ca26 Compare October 6, 2017 18:13

greg0ire commented Oct 6, 2017

View reviewed changes

SenseException reviewed Oct 6, 2017

View reviewed changes

greg0ire force-pushed the split_orm_exception branch 2 times, most recently from 678811a to 1f70547 Compare November 18, 2017 11:50

greg0ire force-pushed the split_orm_exception branch 2 times, most recently from 50d23b5 to 94e566c Compare November 30, 2017 18:25

guilhermeblanco force-pushed the develop branch from 9f5fd37 to e7d61c2 Compare December 4, 2017 00:36

Majkl578 added BC Break Improvement WIP labels Dec 8, 2017

greg0ire force-pushed the split_orm_exception branch from efc918a to 9040e93 Compare December 9, 2017 18:23

Avoid extending \Exception directly

10aac31

greg0ire force-pushed the split_orm_exception branch from 9044cda to 10aac31 Compare April 12, 2018 17:38

Ocramius self-assigned this Apr 13, 2018

Ocramius removed the Requires merge/review label Apr 13, 2018

Ocramius approved these changes Apr 13, 2018

View reviewed changes

Ocramius merged commit 77e3e5c into doctrine:master Apr 13, 2018

Ocramius changed the title ~~Split orm exception~~ Split ORMException into more fine-grained, better documented and typed exception types Apr 13, 2018

Ocramius changed the title ~~Split ORMException into more fine-grained, better documented and typed exception types~~ Split ORMException into more fine-grained, better documented and typed exception classes Apr 13, 2018

Ocramius mentioned this pull request Apr 13, 2018

Document BC breaks introduced by #6743 (Split ORMException and clean up exception types) #7194

Closed

greg0ire deleted the split_orm_exception branch April 13, 2018 15:38

Ocramius mentioned this pull request Apr 13, 2018

Add throw Annotation #6885

Closed

Majkl578 mentioned this pull request May 1, 2018

Split DBALException into smaller specific exceptions, consider dropping "Exception" suffixes doctrine/dbal#3124

Closed

This was referenced May 2, 2018

Further exceptions tweaks #7210

Closed

Starting Doctrine Migrations 2.0 doctrine/migrations#617

Merged

jwage mentioned this pull request May 5, 2018

Split up monster MigrationException in to smaller exceptions doctrine/migrations#621

Closed

Majkl578 mentioned this pull request May 6, 2018

Split and move around exceptions, make MigrationException an interface doctrine/migrations#636

Merged

Ocramius mentioned this pull request Nov 10, 2018

Throw exception in case of calling flush inside flush #7448

Open

Ocramius mentioned this pull request Dec 28, 2018

Fix previous exception rethrow on transaction rollback #7546

Closed

dependabot-preview bot mentioned this pull request May 28, 2019

Bump doctrine/migrations from 1.8.1 to 2.0.2 unil-lettres/dilps-tiresias#12

Closed

dependabot-preview bot mentioned this pull request Jun 10, 2019

Bump doctrine/migrations from 1.8.1 to 2.1.0 unil-lettres/dilps-tiresias#36

Closed

Ocramius mentioned this pull request Jul 31, 2019

Interfaces mis @throw annotations #7786

Open

greg0ire mentioned this pull request May 15, 2021

Split the ORMException class #8692

Merged

3 tasks

greg0ire removed this from the 3.0.0 milestone Jun 27, 2021

greg0ire mentioned this pull request Jan 8, 2025

[RFC] Follow-up on doctrine/collections Order enum #11313

Open


		use Doctrine\ORM\RepositoryException;

		final class InvalidMagicMethodCall extends \InvalidArgumentException implements RepositoryException


		use Doctrine\ORM\RepositoryException;

		final class InvalidFindByInverseAssociation extends \BadMethodCallException implements RepositoryException

Split ORMException into more fine-grained, better documented and typed exception classes #6743

Split ORMException into more fine-grained, better documented and typed exception classes #6743

Conversation

greg0ire commented Oct 2, 2017 • edited Loading

TODO

Proposal

Ocramius commented Oct 3, 2017

greg0ire commented Oct 3, 2017 • edited Loading

Ocramius commented Oct 3, 2017

greg0ire commented Oct 3, 2017

Ocramius commented Oct 3, 2017

greg0ire commented Oct 3, 2017

Majkl578 commented Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SenseException Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

SenseException Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

greg0ire Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Oct 6, 2017

greg0ire commented Oct 7, 2017

Majkl578 commented Dec 8, 2017

Ocramius left a comment

Choose a reason for hiding this comment

Ocramius commented Apr 13, 2018

greg0ire commented Apr 13, 2018

Majkl578 commented May 1, 2018

Majkl578 commented May 1, 2018

Split `ORMException` into more fine-grained, better documented and typed exception classes #6743

Split `ORMException` into more fine-grained, better documented and typed exception classes #6743

greg0ire commented Oct 2, 2017 •

edited

Loading

greg0ire commented Oct 3, 2017 •

edited

Loading

Majkl578 commented Oct 3, 2017 •

edited

Loading

SenseException Oct 4, 2017 •

edited

Loading

greg0ire Oct 6, 2017 •

edited

Loading

SenseException Oct 6, 2017 •

edited

Loading

greg0ire Oct 6, 2017 •

edited

Loading

greg0ire Oct 6, 2017 •

edited

Loading