The exception is not implementing Throwable

[Nyholm]

Here is the history:

Version 1.0.0

The extension interface did not extend \Throwable

Version 1.1.0

The extension interface did extend \Throwable

Version 1.1.1

The extension interface did not extend \Throwable.

[..] as it leads to inheritance issues when child classes implement the Throwable interface in addition to ContainerInterface under PHP versions prior to 7.4.

Version 2.0.0

The extension interface did extend \Throwable as it should since it is a new major release and we don't need to consider edge cases with the BC layer.

Version 2.0.1

The extension interface does not extend \Throwable.

Removes extension of Throwable in the ContainerExceptionInterface to prevent issues under PHP versions < 7.4, where having a concrete instance extend Throwable as well as ContainerExceptionInterface can lead to E_FATAL.

---

Except for adding 3 type hints, these are the only changes in this repository. How can I, as an open source maintainer, reliably depend on this package? Or how can I, as a user that like static analyses, know what exception to catch? Catching NotFoundExceptionInterface or ContainerExceptionInterface is technically not valid.

I understand that there was things nobody could expect when releasing 1.1.0. But in my opinion it was a misstake releasing 1.1.1 and 2.0.1. It is way better being predictable and slightly wrong.

Im not seeking to put blame nor do I require an explanation

I just want us to be aware of this and focus on 2 things moving forward:

1. Better and longer review processes. It was just a few hours between 1.1.0 and 1.1.1. I doubt there was a chance for a public review and to consider the consequences.
2. Version 3 must extend \Throwable

[Jean85]

@Nyholm thanks for opening this discussion. We already discussed this internally, and extending \Throwable is what we want to do in any way.

As you pointed out, dropping the \Throwable extension was done because it was wreaking havoc on end users thanks to a PHP bug that is solved only on 7.4; particularly, all Symfony CI & apps on PHP < 7.4 where broken due to that.

Hence, we had to rush the patch on both major releases, so that no one could install something that could break their code not to their fault; we could've said "your problem, you just need to change the implements order", but that would put too much burdening on the users IMO.

To be fair, the concrete classes that implement those interfaces must implement \Throwable too or they are not exceptions, so and a maintainer you shouldn't be bothered by this issue. In any case, since I care about static analysis too, we're going to release 1.2.0 and 2.1.0 with a PHP requirement bump to 7.4, and with implements \Throwable back in its proper place.

I hope you'll find this answer sufficient.

[mnapoli]

Yes, @Nyholm if the previous discussions you will find that:

• 1.1.1 and 2.0.1 were done as a quick fix to avoid breaking the whole ecosystem on PHP 7.4
• the next step is to re-release with PHP > 8.0 & extending Throwable

That way projects on 8.0 will get the last release (with good static analysis), the rest will have 1.1.1/2.0.1 installed.

Better and longer review processes. It was just a few hours between 1.1.0 and 1.1.1. I doubt there was a chance for a public review and to consider the consequences.

The pull request for 1.1/2.0 was opened for years, but that didn't allow us to catch the PHP 7.4 bug. What was done in 1.1.1 and 2.0.1 were emergency lifesavers, waiting a year for review on that wasn't possible. What consequences do you have in mind exactly related to those pull requests?

Version 3 must extend \Throwable

2.1 should, not sure if we decided whether 1.2 was worth it 🤔

[Nyholm]

I understand that it was an unexpected issue. I don't think it is weird that we made a mistake here. It is the way the mistakes were fixed that I'm more hesitant about. I opened this issue to make sure this package is the best it can be.

Im happy there is a plan to drop PHP <7.4 in the next minor release and that we add back \Throwable. I do think it make sense do to 1.2 as well. Mostly because the effort is so small and because it may help someone to do ^1.2 || ^2.1.

[Jean85]

Im happy there is a plan to drop PHP <7.4 in the next minor release and that we add back \Throwable. I do think it make sense do to 1.2 as well. Mostly because the effort is so small and because it may help someone to do ^1.2 || ^2.1.

Exactly. It's in the spirit of our bylaw to smooth the upgrade path as much as possible, so doing in the 1.x branch is good 👍

PS: for adding \Throwable back ^7.4||^8.0 is enough IIRC... Or am I missing something?

[mnavarrocarter]

Maybe totally out of scope of this, but why do error types need to be interfaces?

A ContainerException class that extends the base Exception or the RuntimeException classes with no extra custom logic is perfectly possible. Having interfaces for exceptions is one of those practices that have little to no reason to exist and yet are extremely common.

Grateful if someone can point if am missing something about why people use interfaces for this.

In my opinion, FIG should consider moving away from error types as interfaces in future majors.

[martinssipenko]

@mnavarrocarter Purpose of this package is to provide interfaces that containers could implement thus guaranteeing that if the user only uses methods and classes provided by this package the actual container implementation can be any as long as it follows the contract established by this package.

If, as you suggested, a ContainerException class in implementation would extend Exception or RuntimeException then those exceptions would not be distinguishable from other exceptions. For example, when a user would use a container he would want to catch exceptions that are related to the container, like if entry is missing from the container, but if RuntimeException would be thrown the user would not be able to know whether it was indeed an exception coming from the container, as it could have been something other throwing that exception, and would not be able to distinguish various kinds of container exception, i.e. not found vs generic container exception.

[mnavarrocarter]

Hi @martinssipenko , thanks for taking the time to answer.

I think this will explain my point better.

If you have:

<?php

class ContainerException extends Exception
{
}

class NotFoundException extends ContainerException
{
}

You can then do:

<?php

try {
    $container->get('some-service');
} catch (ContainerException $e) {
   // Handle this general container error
}

try {
    $container->get('some-service');
} catch (NotFoundException $e) {
   // Handle this more specific error, which means a service was not found
}

// You can even catch and handle both differently
try {
    $container->get('some-service');
} catch (NotFoundException $e) {
   
} catch (ContainerException $e) {

}

This is absolutely possible and reasonable. In fact, is idiomatic PHP, as the standard library exceptions (Runtime, InvalidArgument, etc) all extend the base Exception class.

[Nyholm]

The user code needs to be implementation independent. And this psr package should only include interfaces.

Im happy with the initial responses. Im closing this issue because it is way off topic.

[Jean85]

This is still possible with interfaces though, and on the contrary, since we don't have multiple inheritance on classes, you'll be binding the end user to expand Exception and nothing else.

With an interface, you can still extend your custom exceptions AND follow the spec. This is pretty important for interoperability, where preexisting implementations could have their exception hierarchy, and we can't ask them to break BC because of this.

[dragoonis]

Maybe totally out of scope of this, but why do error types need to be interfaces?

Dependency Inversion. We build inside-out.

Our PSR packages aren't implementations, they're contracts.

Hope that answers your question @mnavarrocarter ?

[mnavarrocarter]

This is still possible with interfaces though, and on the contrary, since we don't have multiple inheritance on classes, you'll be binding the end user to expand Exception and nothing else.

I would argue that in practice, implementors end up doing that. And also argue that there is no problem with extending Exception. Can you point a possible issue with extending the base Exception class?

The user code needs to be implementation independent. And this psr package should only include interfaces.

There are others fig packages that contain implementation logic, like psr/logger. I understand the benefits of interfaces for services. I do not think they bring any benefit as error marker types.

Dependency Inversion. We build inside-out.

Our PSR packages aren't implementations, they're contracts.

I get that. The main interface here is well designed and extremely useful. This is not the case with the error ones. The only possible implementation of an interface that serves as error marker is Exception anyway. No matter what the package that implements it is, it will always be a sub-type of Exception. Instead of escaping that fact it will be good to embrace it. That's what I'm trying to point out.

[Jean85]

Can you point a possible issue with extending the base Exception class?

Let's say you have your own exception classes, but they already extends other exceptions, and they do not have a common ancestor (except at the root with \Exception):

class ContainerException extends \RuntimeException { ... }
class ServiceNotFound extends \LogicException { ... }

Now you want to support/implement PSR-11. With an exception class, you can't; with an interface, you can.

[mnavarrocarter]

That's is easily solvable by rolling a new major of your library that adds support for the PSR. It is very likely you have to modify your ContainerInterface implementation to adjust to the interface anyways. Even in some popular projects whose example has inspired many of the FIG standards a non-breaking adoption hasn't been always possible. Thinking of Http Plug and PSR-18 in concrete here.

https://github.com/php-http/httplug/releases/tag/v2.0.0

[mnapoli]

@mnavarrocarter this could have been an option years ago. And I do see your point, it makes sense.

Now the reality is that it's no longer an option, we can't break something that big.