Closed axlon closed 1 week ago
I had to adjust a test that specifically tests this behavior, but I think the test was faulty to begin with. The test tested that the container would call a resolving callback when a rebind happened, the only reason the binding was being resolved was because the framework was preparing to call rebind callbacks (which weren't set in the test).
Now the tests properly reflect (what I believe to be) the correct behavior, if a binding is rebound and rebinding callbacks are set the new binding is resolved (triggering resolving callbacks) and the rebinding callbacks are called. If a binding is rebound and no rebinding callbacks are set, the binding won't be resolved and thus the container won't call any resolving callbacks
Could there be a situation where a deferred service provider registers a binding and rebound callback. The developer rebinds a binding that is defined in that deferred provider but that hasn't actually been "made" yet - this PR would make it so the rebound callback defined in that deferred provider would not fire.
~This is indeed a problem, I see the Application
class already has some logic in place to deal with this in Application::make()
and Application::resolve()
. This logic should probably be extended to methods that cause rebinds as well if we go this way. I will take a look at this, as well as some tests to cover this~
@taylorotwell I added Application::extend()
to load any relevant deferred service providers when extending (See https://github.com/laravel/framework/pull/53502/commits/17b04b5c40353b3456c3ed9baeba6bf7e7725e1a), but then noticed a test (FoundationApplicationTest::testDeferredServicesAreLazilyInitialized
) started failing.
I think it makes sense to keep extension logic as is. So that no deferred providers are loaded, this is because when an extension is registered the service in question either is, or isn't already loaded. If it is, then the deferrable provider should already have been loaded as well meaning the rebind callbacks must already be present. If it isn't, then there's no need to call any rebind callbacks, because any services that rely on the service being extended could not have been resolved yet either.
I'm not sure, I still think this is breaking. If I have a test and I bind an instance in the container of SomeInterface
, and the deferred provider registers a rebind callback that calls a method on things that implement that interface anytime they are bound, that will no longer be called after this PR, which is a behavior change on a patch release.
@taylorotwell not sure whether you meant Container::instance()
or Container::extend()
in your last comment so I went ahead and checked both, both seem to not call rebind callbacks (on 11.x) when the service was not previously bound, or resolved respectively.
E.g. these tests pass:
public function testBindingAnInstanceCallsRebindCallbacks(): void
{
$container = new Container();
$rebound = false;
$container->rebinding('foo', function () use (&$rebound) {
$rebound = true;
});
$container->instance('foo', new stdClass);
$this->assertFalse($rebound);
}
public function testExtendingAnInstanceCallsRebindCallbacks(): void
{
$container = new Container();
$rebound = false;
$container->rebinding('foo', function () use (&$rebound) {
$rebound = true;
});
$container->extend('foo', fn () => new stdClass);
$this->assertFalse($rebound);
}
I also tried this in an application (maybe something was happening in the test suite that I missed):
class MyServiceProvider extends ServiceProvider implements DeferrableProvider
{
public static $rebound = false;
public function provides(): array
{
return ['foo'];
}
public function register(): void
{
$this->app->rebinding('foo', static function () {
static::$rebound = true;
});
}
}
class MyTest extends TestCase
{
public function testItRebindsInstance()
{
$this->app->instance('foo', new \stdClass());
$this->assertFalse(MyServiceProvider::$rebound);
}
public function testItRebindsExtension(): void
{
$this->app->extend('foo', fn () => new \stdClass());
$this->assertFalse(MyServiceProvider::$rebound);
}
}
I also couldn't trigger a rebind callback this way.
So unless I am misunderstanding something here (which is entirely possible), there would be no breaking change, because the rebind callback won't be triggered right now either.
Something I noticed while debugging https://github.com/laravel/framework/issues/53501 is that Laravel's container currently always instantiates an extended/rebound class to pass to rebinding callbacks, even when no such callbacks are set. This PR fixes that.