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

[DI] Implement PSR-11 #21265

Merged
merged 1 commit into from Feb 8, 2017

Conversation

@greg0ire
Copy link
Contributor

commented Jan 12, 2017

TODO:

  • wait for a stable version of the psr/container package;
  • deprecate instanciating ServiceNotFoundException directly, or using any of its methods directly; not relevant anymore
  • act on the outcome of php-fig/container#8 (solved in php-fig/container#9) ;
  • solve the mandatory NotFoundExceptionInterface on non-existing service
    problem (with a flag ?);
    non-issue, see comments below
  • provide meta-package psr/container-implementation if all problems can
    be solved.
Q A
Branch? master
New feature? yes
BC breaks? not at the moment
Tests pass? didn't pass before pushing, or even starting to code, will see Travis
License MIT
Doc PR TODO

This PR is a first attempt at implementing PSR-11, or at least trying to get closer to it.
Delegate lookup is optional, and thus not implemented for now.

@@ -21,7 +22,7 @@
* @author Fabien Potencier <fabien@symfony.com>
* @author Johannes M. Schmitt <schmittjoh@gmail.com>
*/
interface ContainerInterface
interface ContainerInterface extends PsrContainerInterface

This comment has been minimized.

Copy link
@greg0ire

greg0ire Jan 12, 2017

Author Contributor

I chose to have ContainerInterface extend PsrContainerInterface, so that any interface is marked as psr-compliant. Not sure it is the right thing to do though, because php cannot enforce all the contracts defined in the interface docs (like what exception to throw when). Tell me if you feel I should remove this and add implements ContainerInterface in the main implementation.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 12, 2017

Member

If it works for PHP, fine.

This comment has been minimized.

Copy link
@keradus

keradus Jan 13, 2017

Member

that basically means in whole framework you will not use Psr version, but Symfony version.
Then, one will be able to use concrete class that implements Symfony version as parameter for method expecting Psr version, but not the other way around. So, if I will have some package that follows Psr, I won't be able to use it directly in Symfony project, because everywhere the framework will require Symfony version instead

This comment has been minimized.

Copy link
@greg0ire

greg0ire Jan 13, 2017

Author Contributor

everywhere the framework will require Symfony

Not sure I get what you mean, but IMO once this is merged, we should hunt for the Symfony version everywhere, see what calls are made there, and if we can replace use Psr version as type hinting there.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 13, 2017

Member

@greg0ire without breaking BC, with the benefit of reducing the feature-set for the shake of PSR11 compliance? I doubt this is ever going to happen...

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 13, 2017

Member

@jvasseur yes it does
the issue is that $foo instanceof A implies that calling $foo->test($anotherInstanceOfA) is allowed.
Since $anInstanceofB instanceof A, calling $anInstanceofB->test($anotherInstanceOfAButNotB) must thus be allowed.

This comment has been minimized.

Copy link
@greg0ire

greg0ire Jan 13, 2017

Author Contributor

So… I found a bug in php?

This comment has been minimized.

Copy link
@jvasseur

jvasseur Jan 13, 2017

Contributor

It's a know limitation of the PHP engine : https://bugs.php.net/bug.php?id=69612#1431343525

This comment has been minimized.

Copy link
@jvasseur

jvasseur Jan 13, 2017

Contributor

But your example is wrong anyway, functions parameters could be contravariant (they are invariant in PHP) meaning they can accept a wider type. In your example they are covariant instead.

This comment has been minimized.

Copy link
@greg0ire

greg0ire Jan 13, 2017

Author Contributor

Oh you're right, my logic was wrong indeed! Which means this would indeed be a BC-break, sorry I got confused.

/**
* Base InvalidArgumentException for Dependency Injection component.
*
* @author Bulat Shakirzyanov <bulat@theopenskyproject.com>
*/
class InvalidArgumentException extends \InvalidArgumentException implements ExceptionInterface
class InvalidArgumentException extends \InvalidArgumentException implements ExceptionInterface, ContainerException

This comment has been minimized.

Copy link
@greg0ire

greg0ire Jan 12, 2017

Author Contributor

EnvNotFound extends this exception, so no need to add implementation declaration in it.

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2017

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 of get, but can we pretend this is a valid implementation if using get with a special argument makes it no longer compliant?

@greg0ire greg0ire force-pushed the greg0ire:implement_psr_11 branch from 9adc7ca to 0d62fdc Jan 12, 2017

@@ -16,7 +16,8 @@
}
],
"require": {
"php": ">=5.5.9"
"php": ">=5.5.9",
"psr/container": "^1.0@dev"

This comment has been minimized.

This comment has been minimized.

Copy link
@greg0ire

greg0ire Jan 12, 2017

Author Contributor

See TODO list ;) , I'm not sure to be able to implement it perfectly, so first, let's get as close as we can without a BC-break.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 12, 2017

Member

Still please add it.

This comment has been minimized.

Copy link
@greg0ire

greg0ire Jan 12, 2017

Author Contributor

Fine 👍

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Jan 14, 2017

Contributor

Looks like it should also be added to the root composer.json.

This comment has been minimized.

Copy link
@greg0ire

greg0ire Jan 14, 2017

Author Contributor

Uuuuh… it's already there, isn't it?

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Jan 14, 2017

Contributor

Indeed... I missed it 😳

This comment has been minimized.

Copy link
@greg0ire

greg0ire Jan 14, 2017

Author Contributor

Thanks for reviewing anyway :)

@nicolas-grekas
Copy link
Member

left a comment

There is no issue with throwing : the PSR standardize only one argument, and says to throw. Fortunately that's the default behavior so anyone using the container in the PSR11 way will have it work as expected.
When the 2nd argument is used, we're not in PSR11 anymore, thus we can behave as we want.

@@ -21,7 +22,7 @@
* @author Fabien Potencier <fabien@symfony.com>
* @author Johannes M. Schmitt <schmittjoh@gmail.com>
*/
interface ContainerInterface
interface ContainerInterface extends PsrContainerInterface

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 12, 2017

Member

If it works for PHP, fine.

@@ -16,7 +16,8 @@
}
],
"require": {
"php": ">=5.5.9"
"php": ">=5.5.9",
"psr/container": "^1.0@dev"

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 12, 2017

Member

Still please add it.

@greg0ire greg0ire force-pushed the greg0ire:implement_psr_11 branch from 0d62fdc to f8bf6ea Jan 12, 2017

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2017

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 calling get with two arguments. So it's indeed a non-issue.

@greg0ire greg0ire force-pushed the greg0ire:implement_psr_11 branch from f8bf6ea to 00a8126 Jan 12, 2017

@@ -20,6 +20,7 @@
"doctrine/common": "~2.4",
"twig/twig": "~1.28|~2.0",
"psr/cache": "~1.0",
"psr/container": "^1.0@dev",

This comment has been minimized.

Copy link
@soullivaneuh

soullivaneuh Jan 12, 2017

Contributor

Just reminder comment: Remove @dev annotation when the package will be stable.

This comment has been minimized.

Copy link
@fabpot

fabpot Feb 1, 2017

Member

Can we remove it now so that this PR is ready to be merged?

This comment has been minimized.

Copy link
@greg0ire

greg0ire Feb 1, 2017

Author Contributor

fixed

@mnapoli

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2017

@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.

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2017

👍 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 😄

@dunglas

This comment has been minimized.

Copy link
Member

commented Jan 13, 2017

It looks only small changes are required, nice!

@greg0ire greg0ire force-pushed the greg0ire:implement_psr_11 branch from 00a8126 to 62c61ef Jan 13, 2017

@greg0ire greg0ire changed the title Implement PSR-11 [DI] Implement PSR-11 Jan 13, 2017

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2017

@dunglas yup, that was a breeze :)

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jan 13, 2017

RuntimeException also isn't it?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jan 13, 2017

Why not make ExceptionInterface extend Psr\Container\ContainerExceptionInterface?

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2017

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 listed here

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

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2017

RuntimeException also isn't it?

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

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jan 13, 2017

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

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2017

In fact, it can, see https://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?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jan 13, 2017

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.

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2017

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?

@greg0ire greg0ire force-pushed the greg0ire:implement_psr_11 branch from 62c61ef to ff1b247 Jan 13, 2017

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2017

There. The diff is tiny now :)

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2017

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?

@dunglas

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

👍

@dunglas

dunglas approved these changes Feb 8, 2017

@javiereguiluz
Copy link
Member

left a comment

👍

@fabpot

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

Thank you @greg0ire.

@fabpot fabpot merged commit bde0efd into symfony:master Feb 8, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Feb 8, 2017

feature #21265 [DI] Implement PSR-11 (greg0ire)
This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Implement PSR-11

TODO:

- [x] wait for a stable version of the psr/container package;
- [x] ~~deprecate instanciating ServiceNotFoundException directly, or using any of its methods directly;~~ not relevant anymore
- [x] act on the outcome of php-fig/container#8 (solved in php-fig/container#9) ;
- [x] ~~solve the mandatory NotFoundExceptionInterface on non-existing service
problem (with a flag ?);~~ non-issue, see comments below
- [x] provide meta-package psr/container-implementation if all problems can
be solved.

| Q             | A
| ------------- | ---
| Branch?       | master
| New feature?  | yes
| BC breaks?    | not at the moment
| Tests pass?   | didn't pass before pushing, or even starting to code, will see Travis
| License       | MIT
| Doc PR        | TODO

This PR is a first attempt at implementing PSR-11, or at least trying to get closer to it.
Delegate lookup is optional, and thus not implemented for now.

Commits
-------

bde0efd Implement PSR-11

@greg0ire greg0ire deleted the greg0ire:implement_psr_11 branch Feb 8, 2017

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2017

Thanks for merging!

@allflame

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2017

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.

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2017

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

@jvasseur

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2017

@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

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2017

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

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jun 8, 2017

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

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2017

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

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jun 8, 2017

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

@allflame

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2017

@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)

@stof

This comment has been minimized.

Copy link
Member

commented Jun 8, 2017

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 we don'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)

@allflame

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2017

@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.

@stof

This comment has been minimized.

Copy link
Member

commented Jun 8, 2017

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

@allflame

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.