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

[5.7] [Option 1] Improve PSR-11 implementation #25870

Merged
merged 3 commits into from Oct 4, 2018

Conversation

Projects
None yet
9 participants
@deleugpn
Contributor

deleugpn commented Oct 2, 2018

I've recently tried to implement a small lib to bridge between a Laravel app and a legacy app and I finally understood why so many people tried to change the PSR-11 implementation, which frankly I got it quite wrong. Ref #25682, #25678, #21335, #21327, #19822 and laravel/ideas#803.

The Method

Let's look at the possibilities of $container->get($identifier); call.

1- $identifier has been explicitly bound and the container can resolve it.
2- $identifier has been explicitly bound and the container cannot resolve it.
3- $identifier has not been explicitly bound, but the container can resolve it.
4- $identifier has not been explicitly bound and the container cannot resolve it.

The Problem

Right now, only option 1 and 2 works when using PSR-11 with Laravel container. Something has to explicitly bind into the Laravel container in order for it to work. If it was never bound before, it will not try to do anything. PSR-11 doesn't offer any way to bind things into the container, only resolve out of it.

The Proposal

By making has an alias of resolve / make, we cover all 4 scenarios with get.

1- It will resolve explicitly bound into their concrete.
2- It will throw ContainerExceptionInterface when trying to resolve explicitly bound into their concrete.
3- It will try to resolve non-explicitly bound (auto-wire).
4- It will fail to resolve anything non-resolvable.

The Usage

If this patch gets applied, package developers can opt to ignore the has method and use Laravel container as follow:

function (ContainerInterface $container) {
    // ...

    try {
        $class = $container->get($identifier);
    } catch (ContainerExceptionInterface $e) 
        // Cannot resolve $identifier
    } 
}

or even

function (\Psr\Container\ContainerInterface $container) {
    // ...

    try {
        $class = $container->get($identifier);
    } catch (\Psr\Container\NotFoundExceptionInterface $e) 
        // $identifier was not bound or cannot be resolved, so let's return a default value.

       return new MyDefaultImplementationOfIdentifier();
    } catch (\Psr\Container\ContainerExceptionInterface $e) 
        // Cannot resolve $identifier
    } 
}

deleugpn added some commits Oct 2, 2018

@driesvints

Can you please fix the tests and StyleCI checks?

Nvm, seems you just did :)

@HongjiangHuang

This comment has been minimized.

Show comment
Hide comment
@HongjiangHuang

HongjiangHuang Oct 2, 2018

Contributor

I think this get semantic discrepancy, get means object from the container, rather than make object,psr-11 interface does not define bind because interface does not need to pay attention to how to bind, only need to focus on how to obtain. Interface is universal and unless there's no way, not use try catch, the change will affect thousands of people

Contributor

HongjiangHuang commented Oct 2, 2018

I think this get semantic discrepancy, get means object from the container, rather than make object,psr-11 interface does not define bind because interface does not need to pay attention to how to bind, only need to focus on how to obtain. Interface is universal and unless there's no way, not use try catch, the change will affect thousands of people

@deleugpn

This comment has been minimized.

Show comment
Hide comment
@deleugpn

deleugpn Oct 2, 2018

Contributor

The try catch is a must-have anyway as PSR-11 specifies that get does not have to guarantee an object back.
Right now, get is throwing NotFoundException for anything not bound into the container.
Also, the attached link on laravel/ideas was created by one of the PSR-11 editors, where he mentions that it's up to the container implementation to judge what is considered part of the container.
Auto wire solutions can assume that something is part of the container if we think we can resolve it, even though we might not succeed.
Previous solutions tried to change how has works. This one focus solely on how get works.

Contributor

deleugpn commented Oct 2, 2018

The try catch is a must-have anyway as PSR-11 specifies that get does not have to guarantee an object back.
Right now, get is throwing NotFoundException for anything not bound into the container.
Also, the attached link on laravel/ideas was created by one of the PSR-11 editors, where he mentions that it's up to the container implementation to judge what is considered part of the container.
Auto wire solutions can assume that something is part of the container if we think we can resolve it, even though we might not succeed.
Previous solutions tried to change how has works. This one focus solely on how get works.

@HongjiangHuang

This comment has been minimized.

Show comment
Hide comment
@HongjiangHuang

HongjiangHuang Oct 2, 2018

Contributor

In general, if the container does not define the object, get should not be able to retrieve it.
This is make meaning behavior with get is defined as the conflict and laravel/ideas has been closed

Contributor

HongjiangHuang commented Oct 2, 2018

In general, if the container does not define the object, get should not be able to retrieve it.
This is make meaning behavior with get is defined as the conflict and laravel/ideas has been closed

@deleugpn

This comment has been minimized.

Show comment
Hide comment
@deleugpn

deleugpn Oct 2, 2018

Contributor

PSR-11 disagrees with you.

Contributor

deleugpn commented Oct 2, 2018

PSR-11 disagrees with you.

@X-Coder264

This comment has been minimized.

Show comment
Hide comment
@X-Coder264

X-Coder264 Oct 2, 2018

Contributor

In general, if the container does not define the object, get should not be able to retrieve it.

PSR-11 says this about the get method:

Finds an entry of the container by its identifier and returns it.

Since Laravel's container has such great autowiring abilities there is no reason why the get method should return only objects that are explicitly bound into the container (especially since PSR-11 doesn't impose such restrictions).

Contributor

X-Coder264 commented Oct 2, 2018

In general, if the container does not define the object, get should not be able to retrieve it.

PSR-11 says this about the get method:

Finds an entry of the container by its identifier and returns it.

Since Laravel's container has such great autowiring abilities there is no reason why the get method should return only objects that are explicitly bound into the container (especially since PSR-11 doesn't impose such restrictions).

@moderndevelopment

This comment has been minimized.

Show comment
Hide comment
@moderndevelopment

moderndevelopment Oct 2, 2018

IMO "get" is pretty straightforward. An item is fetched from the container. Autowiring "makes" an object. This seems outside the scope of psr-11. Which is why it is left up to the framework. Some containers handle autowiring differently, however, unless there is an "identifier" stored on the container explicitly, "get" doesn't seem to apply.

Seems like a nightmare of maintenance to make this work properly and address all of the edge cases

moderndevelopment commented Oct 2, 2018

IMO "get" is pretty straightforward. An item is fetched from the container. Autowiring "makes" an object. This seems outside the scope of psr-11. Which is why it is left up to the framework. Some containers handle autowiring differently, however, unless there is an "identifier" stored on the container explicitly, "get" doesn't seem to apply.

Seems like a nightmare of maintenance to make this work properly and address all of the edge cases

@deleugpn

This comment has been minimized.

Show comment
Hide comment
@deleugpn

deleugpn Oct 2, 2018

Contributor

This is great feedback for PSR editors, which is beyond the scope of this PR. According to their statement, it's the container implementation's job to decide whether to do auto-wire for get or not and there's no good reason not to do it.

@moufmouf said:

IMHO, it would make a great deal of sense to change the behaviour of the Laravel container so that the get method can also return auto-wired objects. The current implementation is not wrong, but it is definitely limiting the power of what can be done with the container and there is no good reason to do that.

Perhaps @weierophinney and @mnapoli might also be interested on the feedback about how people have a resistence of having get auto-wire objects.

Contributor

deleugpn commented Oct 2, 2018

This is great feedback for PSR editors, which is beyond the scope of this PR. According to their statement, it's the container implementation's job to decide whether to do auto-wire for get or not and there's no good reason not to do it.

@moufmouf said:

IMHO, it would make a great deal of sense to change the behaviour of the Laravel container so that the get method can also return auto-wired objects. The current implementation is not wrong, but it is definitely limiting the power of what can be done with the container and there is no good reason to do that.

Perhaps @weierophinney and @mnapoli might also be interested on the feedback about how people have a resistence of having get auto-wire objects.

@weierophinney

This comment has been minimized.

Show comment
Hide comment
@weierophinney

weierophinney Oct 2, 2018

@deleugpn I don't agree that PSR-11 only allows for answering the first two of the four distinctions you made.

Containers can autowire, but for those consuming the container, that is an implementation detail. Ideally, if autowiring is in place, has() will return true for anything that can be autowired.

In zend-servicemanager, we have the concept of "abstract factories". These are factories that can produce zero or more instances, and the interface for these includes a method for testing if it can produce the requested instance. Abstract factories therefor provide us with autowiring, and allow our container to answer the last two of the bullet points you noted. (In point of fact, since version 3, zend-di now provides an abstract factory that can be consumed by zend-servicemanager, allowing it to provide reflection-based autowiring for any service!)

So, that's my long-winded way of saying I agree with @moufmouf here.

It would be a good idea for you to look at other PSR-11 implementations as you do this sort of work, as it can allow you to see how others have tackled the problem. I think you would have found early that most of them provide some way of autowiring.

weierophinney commented Oct 2, 2018

@deleugpn I don't agree that PSR-11 only allows for answering the first two of the four distinctions you made.

Containers can autowire, but for those consuming the container, that is an implementation detail. Ideally, if autowiring is in place, has() will return true for anything that can be autowired.

In zend-servicemanager, we have the concept of "abstract factories". These are factories that can produce zero or more instances, and the interface for these includes a method for testing if it can produce the requested instance. Abstract factories therefor provide us with autowiring, and allow our container to answer the last two of the bullet points you noted. (In point of fact, since version 3, zend-di now provides an abstract factory that can be consumed by zend-servicemanager, allowing it to provide reflection-based autowiring for any service!)

So, that's my long-winded way of saying I agree with @moufmouf here.

It would be a good idea for you to look at other PSR-11 implementations as you do this sort of work, as it can allow you to see how others have tackled the problem. I think you would have found early that most of them provide some way of autowiring.

@deleugpn

This comment has been minimized.

Show comment
Hide comment
@deleugpn

deleugpn Oct 2, 2018

Contributor

@weierophinney I agree that PSR-11 can answer all 4. It's just current Laravel implementation (without this PR) that only answer the first 2. I'm trying to change that.

Contributor

deleugpn commented Oct 2, 2018

@weierophinney I agree that PSR-11 can answer all 4. It's just current Laravel implementation (without this PR) that only answer the first 2. I'm trying to change that.

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Oct 3, 2018

Member

Reading the PSR-11 spec:

has takes one unique parameter: an entry identifier, which MUST be a string.
has MUST return true if an entry identifier is known to the container and false if it is not.

There is no language there about if the container could hypothetically resolve some unknown class. It only states that that we should return a boolean value indicating if the identifier is known to us.

Something is not explicitly bound to the container is not known to us. We may be able to resolve it, we may not be able to. We don't KNOW if we can until we actually try it (or recursively reflect through every dependency down the chain) - and if we don't know until we try it, what is the point of the has method at all? Just use get and if you get an exception then you know it failed.

I don't want to get deep into knocking on PSR-11 here but with an auto-wiring container that can resolve things that aren't explicitly bound, the has method has no benefit. Of course, I'm assuming the original intent behind has was to be able to more efficiently check if something is bound before actually spending the processing time resolving it. However, in an auto-wiring container, it would incur the same cost as just calling get directly.

I guess I basically agree with the implementation you have here. has will be just as expensive as get but I don't really see any other solution to the problem in an auto-wiring container.

Member

taylorotwell commented Oct 3, 2018

Reading the PSR-11 spec:

has takes one unique parameter: an entry identifier, which MUST be a string.
has MUST return true if an entry identifier is known to the container and false if it is not.

There is no language there about if the container could hypothetically resolve some unknown class. It only states that that we should return a boolean value indicating if the identifier is known to us.

Something is not explicitly bound to the container is not known to us. We may be able to resolve it, we may not be able to. We don't KNOW if we can until we actually try it (or recursively reflect through every dependency down the chain) - and if we don't know until we try it, what is the point of the has method at all? Just use get and if you get an exception then you know it failed.

I don't want to get deep into knocking on PSR-11 here but with an auto-wiring container that can resolve things that aren't explicitly bound, the has method has no benefit. Of course, I'm assuming the original intent behind has was to be able to more efficiently check if something is bound before actually spending the processing time resolving it. However, in an auto-wiring container, it would incur the same cost as just calling get directly.

I guess I basically agree with the implementation you have here. has will be just as expensive as get but I don't really see any other solution to the problem in an auto-wiring container.

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Oct 3, 2018

Member

@deleugpn were you planning to change the functionality of has to basically just use get?

Member

taylorotwell commented Oct 3, 2018

@deleugpn were you planning to change the functionality of has to basically just use get?

@deleugpn

This comment has been minimized.

Show comment
Hide comment
@deleugpn

deleugpn Oct 3, 2018

Contributor

@taylorotwell I agree with your reasoning here. Although the PSR text doesn't say it, the editors have been saying that has can return true if we think we can resolve it, even though we don't know yet. That's up to us to decide. Back on Laravel 5.5, I tried that approach on #21335 by simply trying to figure out if we can resolve it.

The approach on this PR is different, though, we can just ignore the existence of has for auto-wire functionality and go straight to get. If we get an object, great, if we don't, we can catch the exception and figure out what went wrong (NotFound vs ContainerException). I'm personally liking this approach more than the others, but we can revisit changing has instead if you prefer.

The problem with Laravel right now is that get won't even try to resolve an identifier if it was never bound. That's the killer problem imo. That's why get should not rely on has.

Contributor

deleugpn commented Oct 3, 2018

@taylorotwell I agree with your reasoning here. Although the PSR text doesn't say it, the editors have been saying that has can return true if we think we can resolve it, even though we don't know yet. That's up to us to decide. Back on Laravel 5.5, I tried that approach on #21335 by simply trying to figure out if we can resolve it.

The approach on this PR is different, though, we can just ignore the existence of has for auto-wire functionality and go straight to get. If we get an object, great, if we don't, we can catch the exception and figure out what went wrong (NotFound vs ContainerException). I'm personally liking this approach more than the others, but we can revisit changing has instead if you prefer.

The problem with Laravel right now is that get won't even try to resolve an identifier if it was never bound. That's the killer problem imo. That's why get should not rely on has.

@deleugpn deleugpn referenced this pull request Oct 3, 2018

Merged

[5.7] PSR-11 container #4579

@harikt

This comment has been minimized.

Show comment
Hide comment
@harikt

harikt Oct 4, 2018

Contributor

I agree with @deleugpn here. Thank you for pushing this forward. And thanks for your patience on it 👍 .

Contributor

harikt commented Oct 4, 2018

I agree with @deleugpn here. Thank you for pushing this forward. And thanks for your patience on it 👍 .

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Oct 4, 2018

Member

So, is this a breaking change?

Member

taylorotwell commented Oct 4, 2018

So, is this a breaking change?

@taylorotwell taylorotwell merged commit 35b8219 into laravel:5.7 Oct 4, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Oct 4, 2018

Member

AFAIK it's not since you are throwing the same exception if the object is bound?

Member

taylorotwell commented Oct 4, 2018

AFAIK it's not since you are throwing the same exception if the object is bound?

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Oct 4, 2018

Member

Note that the container now does not follow the PSR-11 spec since the spec (IMO a poor idea) states that get must throw an exception if has is false.

Member

taylorotwell commented Oct 4, 2018

Note that the container now does not follow the PSR-11 spec since the spec (IMO a poor idea) states that get must throw an exception if has is false.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Oct 4, 2018

since the spec (IMO a poor idea) states that get must throw an exception if has is false.

Mmh I have to agree with you considering your use case, it would have been better not to include that sentence I guess.

mnapoli commented Oct 4, 2018

since the spec (IMO a poor idea) states that get must throw an exception if has is false.

Mmh I have to agree with you considering your use case, it would have been better not to include that sentence I guess.

@weierophinney

This comment has been minimized.

Show comment
Hide comment
@weierophinney

weierophinney Oct 5, 2018

Just for reference: has() primarily exists in the specification for the purpose of factory-driven containers. Factories receive the container as an argument, and then can vary behavior based on presence or absence of services. As an example, absence of a configuration service may not be considered problematic, as the service being generated could use defaults; some services may be allowed as nullable; and so on.

Another reason for the has() method is for use with composite containers. In this scenario, the parent container may query each composed container to find one capable of returning the service.

In an auto-wiring, non-composite container, has() likely will not be used, and could even be written such that it always returns true, with the assumption that it will always attempt to build a given service if asked. get() would simply raise an exception if it cannot.

weierophinney commented Oct 5, 2018

Just for reference: has() primarily exists in the specification for the purpose of factory-driven containers. Factories receive the container as an argument, and then can vary behavior based on presence or absence of services. As an example, absence of a configuration service may not be considered problematic, as the service being generated could use defaults; some services may be allowed as nullable; and so on.

Another reason for the has() method is for use with composite containers. In this scenario, the parent container may query each composed container to find one capable of returning the service.

In an auto-wiring, non-composite container, has() likely will not be used, and could even be written such that it always returns true, with the assumption that it will always attempt to build a given service if asked. get() would simply raise an exception if it cannot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment