Add rule about exception namespace

[greg0ire]

Continuing the series of rules PRs, this is a more controversial one. Yay or Nay?

This will result in use statements, and will make spotting badly named
exceptions easier: if you are confused about a class being an exception
or not, then the naming is bad. Also, shallower trees. Things should be
grouped by feature/component, not by kind, no exceptions (pun intended).

Changed to Exception subnamespaces

[Ocramius]

I disagree with this one: I like to have a nice catalog of "things that can go wrong" under MyNamespace\Exception, with every namespace having its own little catalog of things that can break.

Mixing them all together with the actual non-exceptional code is not really helpful

[greg0ire]

Another note that might make you reconsider @Ocramius , in my ORM PR, the exceptions are the only things that are namespaced in a component. For instance, EntityManager is at the root, and its exceptions are in the EntityManager (so you can have that catalog of things that go bad for a specific class). Same goes for Repository or Configuration.

[greg0ire]

For Cache, it's a lot messier, everything is in the Cache namespace (41 classes)

[Ocramius]

@greg0ire I wouldn't put them in the EntityManager namespace - that just leads me to look for stuff in there, and then I'd only find exceptions. That doesn't help the reader, and you should always optimise for the reader.

[greg0ire]

Here is a view of how it looks right now:

PessimisticLockException.php
ORMInvalidArgumentException.php
Internal/Hydration/HydrationException.php
TransactionRequiredException.php
NoResultException.php
Configuration/InvalidEntityRepository.php
Configuration/UnknownEntityNamespace.php
Configuration/ProxyClassesAlwaysRegenerating.php
Configuration/NamedNativeQueryNotFound.php
NonUniqueResultException.php
Mapping/MappingException.php
Mapping/UnknownGeneratorType.php
Mapping/InvalidCustomGenerator.php
Mapping/TableGeneratorNotImplementedYet.php
Persisters/UnrecognizedField.php
Persisters/InvalidOrientation.php
Persisters/MatchingAssociationFieldRequiresObject.php
Persisters/CantUseInOperatorOnCompositeKeys.php
UnexpectedAssociationValue.php
Repository/InvalidMagicMethodCall.php
Repository/InvalidFindByCall.php
Query/QueryException.php
Query/AST/ASTException.php
UnexpectedResultException.php
OptimisticLockException.php
Cache/MetadataCacheUsesNonPersistentCache.php
Cache/MetadataCacheNotConfigured.php
Cache/NonCacheableEntityAssociation.php
Cache/FeatureNotImplemented.php
Cache/LockException.php
Cache/QueryCacheNotConfigured.php
Cache/CannotUpdateReadOnlyEntity.php
Cache/CannotUpdateReadOnlyCollection.php
Cache/InvalidResultCacheDriver.php
Cache/NonCacheableEntity.php
Cache/QueryCacheUsesNonPersistentCache.php
EntityManager/UnrecognizedIdentifierFields.php
EntityManager/InvalidHydrationMode.php
EntityManager/MissingMappingDriverImplementation.php
EntityManager/MismatchedEventManager.php
EntityManager/MissingIdentifierField.php
EntityManager/EntityManagerClosed.php
Tools/MissingColumnException.php
Tools/ToolsException.php
Tools/Pagination/RowNumberOverFunctionNotEnabled.php
Tools/NotSupported.php
EntityNotFoundException.php
NotSupported.php

How do we reorganize this and keep it manageable?

[Ocramius]

Yep, I really think that's making the locating of exceptions and implementations quite confusing

[greg0ire]

An option could be to move Cache exceptions in Cache/Exception, another might be to move it in Exception/Cache, and a third would be to move them all in Exception. Which has your preference?

[Ocramius]

My current preference is to just have Exception there. Exception/Cache could also work, but it leads to problems if there is a Cache namespace with implementations. Creating a Cache namespace just for the Cache/Exception below it is also just adding noise.

[greg0ire]

Creating a Cache namespace just for the Cache/Exception below it is also just adding noise.

The Cache directory is already filled with many other classes.

[greg0ire]

I think this is going towards cache exception in Cache\Exception, and em exception in just Exception.
So the rule should probably be: if a class throws something, it should be in an Exception subnamespace of the namespace where this class resides.

[Ocramius]

The Cache directory is already filled with many other classes.

Yeah, I'm talking about examples where the Cache directory would be created just for the sake of Cache/Exception to exist :-)

Let's call them Potato, and let's assume MyLibrary/Potato.php is a value object:

1. MyLibrary/NotAPotato.php - confusing - is there a concept like Potato near NotAPotato?
2. MyLibrary/Potato/NotAPotato.php - confusing - same as above, plus we created a namespace just for failures, without making it clear that only failures are to be located in here
3. MyLibrary/Exception/Potato/NotAPotato.php possibly valid, but problematic if MyLibrary/Potato is created
4. MyLibrary/Exception/NotAPotato.php - currently used by most libs - kinda works OK and doesn't mean that exceptions will have a brittle name

[greg0ire]

Yeah, not grouping by kind is not realistic when you have lots and lots of classes of that kind (way more than other kinds), So let's go with that. Just to be sure we agree, in my ORM PR, that will result in:

• EntityManager/MissingIdentifierField.php -> Exception/MissingIdentifierField.php
• EntityManager/EntityManagerClosed.php -> Exception/EntityManagerClosed.php
• Configuration/InvalidEntityRepository.php -> Exception/InvalidEntityRepository.php
• Cache/Exception/LockException.php -> Cache/Exception/LockException.php

[greg0ire]

Pushed a new version to reflect the desired policy

[greg0ire]

Another edge-case: there is a class named Configuration, and it throws various exceptions. I'd but them under Exception, but the Configuration directory already exists and holds Configuration\MetadataConfiguration, which must feel pretty lonely in its directory. Should I place those exceptions inside that directory, and amend the rule accordingly? Please advise.

[Ocramius]

@greg0ire given Foo\Configuration, IMO just put it under Foo\Exception\SomethingWentWrong.

[Majkl578]

I like to have a nice catalog of "things that can go wrong" under MyNamespace\Exception,

In a larger code base, it's not realistic and will be chaotical. The root exception interface declares the catalogue.

Given the changes in ORM and Migrations, this is imho good to go.

[alcaeus]

Given the changes in ORM and Migrations, this is imho good to go.

I concur. Do we have a sniff readily available or do we need to create it?

[Majkl578]

Well, this PR is not a sniff, but a not fixable rule in README only.

[Majkl578]

@SenseException Do you think we can cherry-pick this into your #63?

[SenseException]

Yes, I can do that.

[Majkl578]

closing here - consumed by #63