About "the mandatory NotFoundExceptionInterface on non-existing service problem", throwing an exception on missing service id is optional in the current implementation while mandatory in the PSR. How should we solve this problem? I was thinking we could deprecate the second argument ofget, but can we pretend this is a valid implementation if using get with a special argument makes it no longer compliant?

When the 2nd argument is used, we're not in PSR11 anymore

Makes sense. We can't hurt people that don't know which implementation they are using, because if they don't they will probably not risk callingget with two arguments. So it's indeed a non-issue.

@greg0ire

Makes sense. We can't hurt people that don't know which implementation they are using, because if they don't they will probably not risk calling get with two arguments. So it's indeed a non-issue.

👍 the interface was designed to be compatible with Symfony's default behavior when called with the PSR signature.

👍 the interface was designed to be compatible with Symfony's default behavior when called with the PSR signature.

I did not read the underlying discussions for this spec but there definitely was a "well this is convenient" feeling when implementing it in Symfony 😄

It looks only small changes are required, nice!

@dunglas yup, that was a breeze :)

RuntimeException also isn't it?

Why not make ExceptionInterface extend Psr\Container\ContainerExceptionInterface?

Why not make ExceptionInterface extend Psr\Container\ContainerExceptionInterface?

Only exceptions thrown directly by the container are supposed to implement this, so I only applied it to exceptions listedhere

This meansthis is an issue doesn't it ? Should I changethis line so that the exception thrown here is wrapped into another?

RuntimeException also isn't it?

Can't find it in Container.php, so no.

In fact, it can, seehttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php#L601

Andhttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/ContainerBuilder.php#L858

Andhttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/ContainerBuilder.php#L374

I suggest to play simpe here. We don't care to add the interface on all exceptions. This wouldn't hurt.

In fact, it can, seehttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php#L601

Yeah but :

Exceptions directly thrown by the container

What does "directly" mean here?

I don't know in fact, and that's a good question!
But throwing this exception in other cases is not going to make things not-PSR11 compliant. It just wouldn't be a normalized use case. Fine, nothing broken.
I showed you that in fact, via ContainerBuilder, there is a much greater variety of exceptions that can be thrown out there.

Very true, plus, less code, so… I'll change that.@mnapoli , can you advise about the meaning of "directly" here though? You know, for science?

There. The diff is tiny now :)

This means this is an issue doesn't it ? Should I change this line so that the exception thrown here is wrapped into another?

@nicolas-grekas what about this? Any thoughts?

👍

Thank you@greg0ire.

Thanks for merging!

Can you please consider BC breaks not only with respect to "implementation" but also to "declaration"?
This is how it was before:

interface I1 {} interface I2 {}class C1 implements I1, I2 {}

And that's what happened:

interface I1 extends I2 {};Class C1 implements I1, I2 -> Fatal Error

Your interfaces are your burden as much as your implementation.

@allflame can't reproduce :https://3v4l.org/gNuOY

@greg0ire you have to change the order of the implemented interfaces :https://3v4l.org/032Sk

It's a know PHP bug.

EDIT: here is the bugs.php.net reference :https://bugs.php.net/bug.php?id=63816

Oh ok... well sorry, but I didn't know that bug.

Which class should be affected by that? This PR did not change any class to implement two interfaces, did it?

@xabbuh classes outside Symfony that happen to implementContainerInterfaceandPsrContainerInterface (not sure what order is buggy but one of them is)

Hm yeah, nothing we can account for here. This needs to be changed in those projects or fixed in PHP.

@xabbuh this "bug" is like 5 years old and since it just a consequence of OOP implementation in PHP it won't likely be "fixed" for another 5 years. My message here is the following: changes in "contracts/definitions" (that is interfaces, inheritance, function declaration etc) have way more potential to break things than changes in the "implementation" (e.g. actual code)
And one thing that you didn't know about PHP behavior and made that change - that's fine, but if you are aiming for BC in minor release - you cannot make statements like that, knowing this will break things (for whatever reasons)

Well, this means that we would have to avoid providing a PSR-11 compliant container because someone might have already built an external project implementing both PSR-11 and our interface ? This means we would never be able to implement PSR on existing code outside major versions.

Btw, our BC promise explicitly mentions that wedon't ensure BC if you add new methods in a child class (as this would then forbid us to add any method in our existing classes due to potential signature mismatches for the child class using the name already). Adding extra interfaces on the class generally involve adding such method (PSR-11 actually may not need it though)

@stof Yes, this is what software versioning, BC and PHP imply.
The code snippet I've provided has no child classes and I don't understand why you mentioned it here. The real cause was adding extra interfaces to interface which introduced incompatibility. I'll let you figure out why this differs so much with your classes example.

Well, are you actually implementing the Symfony ContainerInterface from scratch without using our base classes ?

No, of course not.
My personal belief is that most Symfony code is very good to use as is, that's why I'm using it in other frameworks trying to substitute their classes.
Original piece of code was like this:
https://github.com/allflame/vain-phalcon/blob/v0.4.104/src/Di/Symfony/SymfonyContainerAdapter.php#L26
This has to be done, because part of the framework depends on PhalconInterface, Symfony on its own depends on SymfonyInterface and the whole application is container-agnostic (well, up to PSR-11), hence PsrInterface.
And that's when this commit backfired.