laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.62k stars 11.03k forks source link

Application booted callbacks called twice #53589

Open simonworkhouse opened 2 days ago

simonworkhouse commented 2 days ago

Laravel Version

11.33.2

PHP Version

8.2.23

Database Driver & Version

SQLite

Description

Booted callbacks are being called twice when they are registered within another booted callback.

Steps To Reproduce

Create a fresh laravel/laravel project and add the following to the register method of App\Providers\AppServiceProvider:

$this->app->booted(function () {
    dump("This only outputs once.");
    $this->app->booted(function () {
        dump("This outputs twice.");
    });
});

Execute php artisan about and it will show the following:

$ php artisan about   
"This only outputs once." // app/Providers/AppServiceProvider.php:15
"This outputs twice." // app/Providers/AppServiceProvider.php:17
"This outputs twice." // app/Providers/AppServiceProvider.php:17
...
simonworkhouse commented 2 days ago

A potential fix would be to update Illuminate\Foundation\Application::booted($callback) with the following:

public function booted($callback)
{
    if ($this->isBooted()) {
        $callback($this);
    } else {
        $this->bootedCallbacks[] = $callback;
    }
}

I just don't have the time available to write tests and submit a PR right now.

crynobone commented 2 days ago

What's your actual use case for this?

simonworkhouse commented 2 days ago

Do you require me to provide you with a detailed use-case in order for this bug to be fixed? Because I can do that, but I just don't see it as being a constructive use of one's time.

In short, I discovered this issue while integrating packages maintained by separate developers, where both packages need to ensure that actions occur after application boot, and one of those packages also provides a mechanism for triggering callbacks after it has booted (although not actual package boot, but it's own concept of "booting" as it provides support for extensions/addons/plugins). Now the first package would either have to depend on the second always executing it's booted callbacks after application boot (which is risky and not guaranteed), or it could simply add it's own application booted callbacks that are added during the second packages booted callbacks.

That being said, do please let me know if you require a detailed use-case.

crynobone commented 2 days ago

Do you require me to provide you with a detailed use-case in order for this bug to be fixed?

  1. This is the first time such issue is being reported, and the existing code (current behavior) exists as early as Laravel 5.1
  2. Laravel itself doesn't use such use case, instead it utilise ServiceProvider::booted() for similar use case (I would assume similar but would need additional information as requested to confirm) https://github.com/laravel/framework/blob/59bf3bfc0f9acce5b1d11c08ddba01f82201c678/src/Illuminate/Foundation/Support/Providers/RouteServiceProvider.php#L53-L66
simonworkhouse commented 2 days ago
  1. Certainly not as early as 5.1, I think that you might find that is the offending commit https://github.com/laravel/framework/commit/9eadb7f25db5b0a1aa78470c864a17eb815781f8
  2. Perhaps Laravel itself may not have this direct use-case, but there's this merged PR that provides support for functionality such as this https://github.com/laravel/framework/pull/39175 so clearly it's a desirable use-case.
rodrigopedra commented 1 day ago

I think we could either:

Option A: Swap lines 1105 and 1107 from the Illuminate\Foundation\Application@boot() method:

https://github.com/laravel/framework/blob/eb625fa6aa083dbae74b4f2cc7dc6dafcce5ff07/src/Illuminate/Foundation/Application.php#L1090-L1108

IMO, the internal booted variable should only become true as the very last thing of the Application@boot() method, until then, it is still "booting".

Option B: Change the Illuminate\Foundation\Application@boot() method from:

https://github.com/laravel/framework/blob/eb625fa6aa083dbae74b4f2cc7dc6dafcce5ff07/src/Illuminate/Foundation/Application.php#L1144-L1151

to:

public function booted($callback)
{
    if ($this->isBooted()) {
        $callback($this);
    } else {
        $this->bootedCallbacks[] = $callback;    
    }
}

This is more cumbersome, as the callbacks are no longer cached, but as $this->isBooted() returns true while running $this->fireAppCallbacks($this->bootedCallbacks) (without option A), the inner callback gets called right away while also being added to the array, thus later being called twice.

I think both "fixes" could be considered. Unless we want to keep track of $this->bootedCallbacks for some reason, such as Octane.

Currently, from my understanding, the $this->bootedCallbacks array isn't iterated anymore after the Application is booted.

For the record, I tested both options on a local fresh Laravel project, and both seem to resolve the issue.

Unfortunately, I won't be able to work on a proper PR for the next few days. If someone wants to give it a shot, it would be great.