Adding exception interfaces

[moufmouf]

While working on definition-interop implementation, I found 2 places where exceptions could potentially be triggered:

• DefinitionProviderInterface might send back objects that are not one of the valid definitions (for instance, it might return a stdClass. In this case, we might want to throw a InvalidDefinition exception.
• while passing arguments to a definition, arguments passed could also be invalid (for instance, we could pass a stdClass to the MethodCall::addArgument method, which is invalid too. In this case, we might want to throw a InvalidArgument exception.

These could be simple empty interfaces:

interface DefinitionException {
}

interface InvalidDefinition extends DefinitionException {
}

interface InvalidArgument extends DefinitionException {
}

Shall I write a PR for this?
If yes, I'll also have to modify the unit test to test for these interfaces.

Also, what about the naming? Sall I suffix those with "Exception"? Or even worse, with ExceptionInterface? :) ... mmmm.... suffixes :)

[Anahkiasen]

I'm +1 on the Definition Exception but for the invalid arguments can't we just use a core InvalidArgumentException?

[prisis]

Im too for core InvalidArgumentException don't add to much interfaces :D

[mnapoli]

Custom exceptions are only useful to be caught (so that we can catch those exceptions and not others). Why would we want to catch those exceptions?

[moufmouf]

Mmmmm... good point. I should maybe review my exceptions best practices :)
So basically, an InvalidArgumentException could be enough?

We should then maybe just write in the spec (when we will write it) that compilers/containers MUST throw an InvalidArgumentException if provided with an invalid definition or an invalid argument.

We might still keep the base interface DefinitionException? For instance, if we want to write a command line utility that validates all definitions, it could be useful to have the possibility to catch exceptions that are related to definitions only?

Please note I have no strong opinion on this, I was simply trying to write the unit tests and noticed this was something we did not discuss before.

[Anahkiasen]

I think basing the exceptions on actual use cases as @mnapoli said would be best, I think currently in Assembly there's only one, the InvalidDefinitionException, no?

[mnapoli]

Let's put it another way: before adding an exception we should think how it would be justified in a meta document.

Interop\Container\NotFoundException was justified because some containers would need to catch it to do some behavior when a container entry was not found, for example catch it and return null (e.g. in Symfony that's a valid behavior for the container), fallback to another entry (e.g. composite containers), etc.

• why would a class implementing an interface defined in this package throw a InvalidDefinitionException?
• why would a consumer of these class want to catch a InvalidDefinitionException specifically?

I just want to point out that we don't care what exceptions containers will throw when reading standard definitions, because it's already out of the scope of this standard (it's happening in a container when resolving definitions, and here we don't define container parts that do the resolving).

[moufmouf]

Hey Matthieu,

I think I understand your point.

At least, I can tell you why a container consuming definition-interfaces should throw an exception.
The DefinitionProviderInterface::getDefinitions method should return objects implementing the DefinitionInterface (and more specifically objects implementing one of the allowed subclasses of the DefinitionInterface). I think we should specify that if something else is passed in parameter, the container/compiler SHOULD (or MUST) throw an exception. Because otherwise, this could lead to containers taking in parameter definitions that are not part of the allowed set of interfaces (and therefore containers not being 100% capable of creating every definition passed to them).
An alternative is to say that container/compiler SHOULD (or MUST) simply discard any object they don't know without failing. This would allow some containers to handle "advanced" definitions that other containers don't. Anyway, we should probably decide if it is ok or not to pass something that is not a xxxDefinitionInterface in DefinitionProviderInterface::getDefinitions.

Now, I also understand your point.

Why would a consumer of these class want to catch a InvalidDefinitionException specifically?

I honestly don't know. I've been trying to figure out a valid use case, but the truth is I can't. So you are probably right that defining a InvalidDefinitionException might be useless.

I'm ok to close this issue now, but first, I'd like to know what I should do with the unit test here:
https://github.com/container-interop/definition-interop-compiler-test-suite/blob/master/src/AbstractDefinitionCompatibilityTest.php#L123-L134

Are we ok that the container/compiler should throw an exception (and we don't care about the exception type). Or should we simply remove the test?

[Anahkiasen]

Anyway, we should probably decide if it is ok or not to pass something that is not a xxxDefinitionInterface in DefinitionProviderInterface::getDefinitions.

To me it's not. The point of this PSR is to allow packages to create universal definitions, if we start having definitions that have additional features only supported by container X or Y, we're back to square one really.

[mnapoli]

The DefinitionProviderInterface::getDefinitions method should return objects implementing the DefinitionInterface (and more specifically objects implementing one of the allowed subclasses of the DefinitionInterface).

I've opened #23 for that. The interface was lacking such precision, now it should be good. The fact that we can't strictly enforce that at the compilation level in PHP is just because PHP doesn't support this. But the interface specifies the return type in phpdoc. Implementation that do not respect that are just not compliant.

I think we should specify that if something else is passed in parameter, the container/compiler SHOULD (or MUST) throw an exception.

This is out of scope. We are standardizing definitions, not containers consuming these definitions.

I'd like to know what I should do with the unit test here

This test should be removed IMO.