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

[11.x] Skip object construction if no rebound callbacks are set #53502

Merged
merged 2 commits into from
Nov 21, 2024

Conversation

axlon
Copy link
Contributor

@axlon axlon commented Nov 13, 2024

Something I noticed while debugging #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.

@axlon axlon marked this pull request as draft November 13, 2024 16:46
@axlon
Copy link
Contributor Author

axlon commented Nov 14, 2024

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

@axlon axlon marked this pull request as ready for review November 14, 2024 08:49
@taylorotwell
Copy link
Member

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.

@taylorotwell taylorotwell marked this pull request as draft November 14, 2024 15:37
@axlon
Copy link
Contributor Author

axlon commented Nov 14, 2024

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 17b04b5), 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.

@axlon axlon marked this pull request as ready for review November 15, 2024 21:39
@taylorotwell
Copy link
Member

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 taylorotwell marked this pull request as draft November 18, 2024 22:05
@axlon
Copy link
Contributor Author

axlon commented Nov 21, 2024

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

@axlon axlon marked this pull request as ready for review November 21, 2024 19:53
@taylorotwell taylorotwell merged commit 62eab15 into laravel:11.x Nov 21, 2024
31 checks passed
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.

2 participants