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] Fix extensions of contextual bindings #53514

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

axlon
Copy link
Contributor

@axlon axlon commented Nov 14, 2024

Fixes #53501

Right now, Laravel always marks a binding as resolved when its done resolving them, even if the binding in question was a contextual binding. When extending bindings Laravel will check if a binding was resolved, and if it was it will retrieve it and extend it.

The problem here is that if a binding is only contextual, it cannot be resolved, but the framework will still attempt to. This PR aims to fix this by only marking bindings as resolved when they aren't contextual. This allows contextual bindings and extensions to work together in the event where the extension happens after the contextual binding is resolved (it already worked without the binding being resolved before the extension)

@taylorotwell taylorotwell merged commit 377ba1d into laravel:11.x Nov 14, 2024
33 checks passed
@andrey-helldar
Copy link
Contributor

andrey-helldar commented Nov 15, 2024

Interesting point after upgrading from 11.31 to 11.32:

Before:

app()->resolved(Some::class); // false
app()->bound(Some::class); // false

app(Some::class);

app()->resolved(Some::class); // true
app()->bound(Some::class); // false

After:

app()->resolved(Some::class); // false
app()->bound(Some::class); // false

app(Some::class);

app()->resolved(Some::class); // false
app()->bound(Some::class); // false

I am using Laravel Octane in production and I need to load a lot of classes via bindings so that they are not stored in the application memory and are unloaded from it when processing is completed.

I use this method in tests to make sure that a certain class was or was not loaded.

For example:

Example 1
abstract class Service
{
    abstract public function handle(): void;

    public function __construct(
        protected Bot $bot,
        protected Chat $chat,
        protected array $data,
    ) {}
}

class FooService extends Service
{
    public function handle(): void
    {
        // something
    }
}

class BarService extends Service
{
    public function handle(): void
    {
        // something
    }
}

class WebhookController
{
    public function handle(MessageRequest $request, Bot $bot)
    {
        $chat = Chat::findOrFail(
            $request->get('chat')
        );

        $service = match ($request->get('type')) {
            'foo'   => FooService::class,
            'bar'   => BarService::class,
            default => null
        };

        if ($service) {
            app($service, compact('bot', 'chat'))->handle();
        }

        return response()->noContent();
    }
}

// Tests

expect()->extend('toBeResolvedInstance', fn () => expect(
    app()->resolved($this->value)
)->toBeTrue());

it('foo', function () {
    postJson('/webhook', ['chat' => 123, 'type' => 'foo', 'value' => 'qwerty1']);

    expect(FooService::class)->toBeResolvedInstance();
    expect(BarService::class)->not->toBeResolvedInstance();
});

it('bar', function () {
    postJson('/webhook', ['chat' => 123, 'type' => 'bar', 'value' => 'qwerty2']);

    expect(FooService::class)->not->toBeResolvedInstance();
    expect(BarService::class)->toBeResolvedInstance();
});

it('custom', function () {
    postJson('/webhook', ['chat' => 123, 'type' => 'custom', 'value' => 'qwerty2']);
    
    expect(FooService::class)->not->toBeResolvedInstance();
    expect(BarService::class)->not->toBeResolvedInstance();
});

I wrote the logic in “controller” for one purpose - to show the context of the application.

Example 2
abstract class Service
{
    abstract public function handle(): bool;

    public function __construct(
        protected array $data
    ) {}
}

class FooService extends Service
{
    public function handle(): bool
    {
        // something

        return true;
    }
}

class BarService extends Service
{
    public function handle(): bool
    {
        // something

        return false;
    }
}

class WebhookController
{
    public function handle(MessageRequest $request)
    {
        $services = [
            FooService::class,
            BarService::class,
        ];

        foreach ($services as $service) {
            $result = app($service, ['data' => $request->validated()])->handle();

            if ($result) {
                break;
            }
        }

        return response()->noContent();
    }
}

// Tests
expect()->extend('toBeResolvedInstance', fn () => expect(
    app()->resolved($this->value)
)->toBeTrue());

it('foo', function () {
    postJson('/webhook', ['chat' => 123, 'type' => 'foo', 'value' => 'qwerty1']);

    expect(FooService::class)->toBeResolvedInstance();
    expect(BarService::class)->not->toBeResolvedInstance();
});

To fix this bug:

Option 1
/*
 * Laravel 11.31 and below
 */
abstract class Module
{
    public function __construct(
        protected Bot $bot,
        protected Chat $chat,
        protected ?Member $member,
        protected array $source,
    ) {}

    // ...
}

return app($module, $data)->handle();

/*
 * Laravel 11.32
 */
abstract class Module
{
    protected Bot $bot;
    protected Chat $chat;
    protected ?Member $member;
    protected array $source;

    public function init(
        Bot $bot,
        Chat $chat,
        ?Member $member,
        array $source,
    ): static {
        $this->bot    = $bot;
        $this->chat   = $chat;
        $this->member = $member;
        $this->source = $source;

        return $this;
    }

    // ...
}

return app()->make($module)->init($bot, $chat, $member, $source)->handle();
Option 2
abstract class Module
{
    public function __construct(
        protected Bot $bot,
        protected Chat $chat,
        protected ?Member $member,
        protected array $source,
    ) {}

    // ...
}

if (config('app.debug')) {
    Log::info($module);
}

return (new $module($bot, $chat, $member, $source))->handle();

// Tests

use Illuminate\Log\Events\MessageLogged;
use Illuminate\Support\Facades\Event;

it('foo', function () {
    Event::fake(MessageLogged::class);

    postJson('/webhook', ['chat' => 123, 'type' => 'foo', 'value' => 'qwerty1']);

    Event::assertDispatched(fn (MessageLogged $e) => $e->message === FooService::class);
    Event::assertNotDispatched(fn (MessageLogged $e) => $e->message === BarService::class);
});
Option 3 (recommended)
abstract class Module
{
    public function __construct(
        protected Bot $bot,
        protected Chat $chat,
        protected ?Member $member,
        protected array $source,
    ) {}

    // ...
}

if (config('app.debug')) {
    Log::info($module);
}

return (new $module($bot, $chat, $member, $source))->handle();

// Tests

use Illuminate\Log\Events\MessageLogged;
use Illuminate\Support\Facades\Event;
use Pest\Expectations\OppositeExpectation;

expect()->extend('toBeCalled', function () {
    $items = Event::dispatched(MessageLogged::class, function (MessageLogged $e) {
        return $e->message === $this->value;
    });

    $message = $this instanceof OppositeExpectation
        ? sprintf('The "%s" class should not be called.', $this->value)
        : sprintf('The "%s" class must be called.', $this->value);

    expect($items->count())->toBeGreaterThan(0, $message);
});

it('foo', function () {
    Event::fake(MessageLogged::class);

    postJson('/webhook', ['chat' => 123, 'type' => 'foo', 'value' => 'qwerty1']);

    expect(FooService::class)->toBeCalled();
    expect(BarService::class)->not->toBeCalled();
});

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.

Container::extend() breaks when $abstract was previously contextually bound
3 participants