Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PSR-11 friendly kernels #21

Closed
wants to merge 4 commits into from
Closed

Conversation

jjanvier
Copy link

@jjanvier jjanvier commented Oct 3, 2018

Hello Matthieu,

I'm not sure you'll be interested by this PR, but let's try. I was looking for something able to make me use a PSR-11 container with Symfony's kernel. And I found your bridge.

But I'd like to be able to use any kind of PSR-11 container. Hence this PR where you now have "PSR 11 compatible kernel" and specific "PHP DI compatible kernel".

If you're not interested just let me know. I'll create another bridge and refer to yours for the paternity.

What's missing:

  • test the catch of ContainerExceptionInterface (but I suck in PHPUnit, I'm too used to PhpSpec sadly...)
  • test the KernelWithPsr11Container?

@mnapoli
Copy link
Member

mnapoli commented Oct 4, 2018

Hi, thanks for the PR and trying to make it work with this repository!

I think those are good changes and a PSR-11 compatibility layer is great (instead of targeting PHP-DI only). But unfortunately I think this wouldn't be a good fit here:

  • as far as I understand there are BC breaks here (which I'd rather avoid if there is no huge gain for users)
  • this repository is very PHP-DI specific, so I think it would be more beneficial to have a separate (neutral) repository

So I encourage you to create a separate repository. In the future I could even refactor this one to use yours (or maybe get rid of this one if that's enough).

By the way I have a question that I will open in the diff.

if ($invalidBehavior === self::EXCEPTION_ON_INVALID_REFERENCE) {
throw new ServiceNotFoundException($id, null, $e);
}
} catch (ContainerExceptionInterface $e) {
throw new \Exception('Error while retrieving the entry.', $e->getCode(), $e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why catch a more specific exception to rethrow it with a less precise one? You could just let the original exception bubble (by not catching it)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because here you don't really respect Symfony's contract (which itself doesn't respect Psr/Container's contract).
Symfony's contract expects ServiceCircularReferenceException, ServiceNotFoundException and Exception to be thrown.

ContainerExceptionInterface can be thrown by $this->fallbackContainer->get($id).
But ContainerExceptionInterface does not extend or implement anything expected by Symfony.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand the problem: ContainerExceptionInterface doesn't extend/implement \Exception.

That's correct, but it's impossible to throw anything that doesn't extend exception so I think catching + rethrowing isn't necessary 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But honestly, I don't if it would work in more "structured" language. For instance, I'm not sure it would compile in Java

@jjanvier
Copy link
Author

jjanvier commented Oct 5, 2018

Hello,

No problem, I totally understand your concerns :)

@jjanvier jjanvier closed this Oct 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants