Skip to content

Conversation

@slavarazum
Copy link
Contributor

@slavarazum slavarazum commented Feb 25, 2018

When we bind something with IoC container like this:

app()->bind(Some::class, SomeAwesome::class);

And register resolving callback

app()->resolving(Some::class, function ($some) {
    dump('Resolving Some');
});

After resolve this class by resolve(Some::class) resolving callback will fire twice. Because 1st step will be - resolve Some::class and 2nd step will be resolve SomeAwesome::class as condition in getCallbacksForType method is:

if ($type === $abstract || $object instanceof $type) {
    $results = array_merge($results, $callbacks);
}

$object instanceof $type matches Some interface and also SomeAwesome class, because SomeAwesome is instanceof Some.
I think we should exclude cases when resolving callback registered to interface fires for class.

 if ($type === $abstract ||
    $object instanceof $type &&
    (class_exists($abstract) || ! interface_exists($abstract)) &&
    ! interface_exists($type)) {
    $results = array_merge($results, $callbacks);
}

@sisve
Copy link
Contributor

sisve commented Feb 25, 2018

The suggested if-statement looks like a mess and there's no tests in this PR.

There's still two resolving events trigger if Some is an class.

@slavarazum
Copy link
Contributor Author

@sisve Do you have any other suggestions how to resolve this issue?

@tillkruss
Copy link
Contributor

Extract parts of the condition into a method, or at least a temporary variable.

@GrahamCampbell GrahamCampbell changed the title Fix double firing of resolving callback [5.6] Fix double firing of resolving callback Feb 26, 2018
@slavarazum
Copy link
Contributor Author

@tillkruss It's clear, but I think maybe there is a better solution for this issue. I have investigated how IoC Container works and I haven't found any solution without making large-scale changes yet.

@taylorotwell
Copy link
Member

I am closing this pull request because it lacks sufficient explanation, tests, or both. It is difficult for us to merge pull requests without these things because the change may introduce breaking changes to the framework.

Feel free to re-submit your change with a thorough explanation of the feature and tests - integration tests are preferred over unit tests. Please include it's benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc.

Thanks!

@tomlankhorst
Copy link
Contributor

Btw, a 'hack' is to make the concrete class in a closure without invoking the service container:

app()->bind(Some::class, function(){
  return new SomeAwesome;
});

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.

5 participants