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

[5.6] Fix double firing of resolving callback #23290

Closed
wants to merge 3 commits into from
Closed

[5.6] Fix double firing of resolving callback #23290

wants to merge 3 commits into from

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