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

Application booted callbacks called twice #53589

Closed
simonworkhouse opened this issue Nov 20, 2024 · 14 comments
Closed

Application booted callbacks called twice #53589

simonworkhouse opened this issue Nov 20, 2024 · 14 comments

Comments

@simonworkhouse
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

What's your actual use case for this?

@simonworkhouse
Copy link
Contributor Author

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
Copy link
Member

crynobone commented Nov 20, 2024

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)
    $this->booted(function () {
    $this->setRootControllerNamespace();
    if ($this->routesAreCached()) {
    $this->loadCachedRoutes();
    } else {
    $this->loadRoutes();
    $this->app->booted(function () {
    $this->app['router']->getRoutes()->refreshNameLookups();
    $this->app['router']->getRoutes()->refreshActionLookups();
    });
    }
    });

@simonworkhouse
Copy link
Contributor Author

  1. Certainly not as early as 5.1, I think that you might find that is the offending commit 9eadb7f
  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 [8.x] Allow queueing application and service provider callbacks while callbacks are already being processed #39175 so clearly it's a desirable use-case.

@rodrigopedra
Copy link
Contributor

I think we could either:

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

public function boot()
{
if ($this->isBooted()) {
return;
}
// Once the application has booted we will also fire some "booted" callbacks
// for any listeners that need to do work after this initial booting gets
// finished. This is useful when ordering the boot-up processes we run.
$this->fireAppCallbacks($this->bootingCallbacks);
array_walk($this->serviceProviders, function ($p) {
$this->bootProvider($p);
});
$this->booted = true;
$this->fireAppCallbacks($this->bootedCallbacks);
}

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:

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

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.

@simonworkhouse
Copy link
Contributor Author

@crynobone Maybe the needs more info label could be removed now?

@crynobone
Copy link
Member

I still don't have enough information to verify that a change really needed (valid use case). However, feel free to send in a PR to fixed this if you're free.

@simonworkhouse
Copy link
Contributor Author

simonworkhouse commented Nov 27, 2024

@crynobone Did you not notice the previous comment indicating that this bug was introduced in this commit 9eadb7f which was released in version 8.65.0?

This is obviously buggy and undesired behaviour, do you genuinely need a specific use-case before it will be considered for fixing?

@simonworkhouse
Copy link
Contributor Author

@crynobone PR created #53683

@crynobone
Copy link
Member

Did you not notice the previous comment indicating that this bug was introduced in this commit 9eadb7f which was released in version 8.65.0?

8.65.0 was released 3 years ago, it is not as if the bug was introduced last month.

Do you genuinely need a specific use-case before it will be considered for fixing?

If I need to submit a bugfix PR I need to be able to:

  1. fully understand and can explain why it's needed
  2. able to back up any argument against it
  3. look into adding tests (if needed)

As of now, I would believe using ServiceProvider::booted() with Application::booted() should cover what you described above.

Thanks for the PR, Taylor will review it soon.

@simonworkhouse
Copy link
Contributor Author

Here's an alternative implementation for consideration #53695

@crynobone
Copy link
Member

Looks like Taylor not changing the behavior right now. Closing this for now.

@Abstechs
Copy link

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.

I'm experiencing the same issue... Did you get it fixed ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants