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

[5.3] Use bootstrappers instead of events #16012

Merged
merged 1 commit into from
Oct 22, 2016
Merged

[5.3] Use bootstrappers instead of events #16012

merged 1 commit into from
Oct 22, 2016

Conversation

themsaid
Copy link
Member

@themsaid themsaid commented Oct 19, 2016

This PR switches to using a static object to collect console application bootstrappers that needs to run on starting Artisan, this is an attempt to deal with the situation where you use $this->withoutEvents(); while running tests & try to call a console command which will attempt to listen to the ArtisanStarting event, but since the Dispatcher is mocked no events get fired.

@GrahamCampbell
Copy link
Member

Nice idea.

@nWidart
Copy link
Contributor

nWidart commented Oct 31, 2016

Hello,

This was a breaking change on my application.
A failing job after the update: https://travis-ci.org/AsgardCms/Platform/jobs/171405604
A successful job before the update: https://travis-ci.org/AsgardCms/Platform/jobs/169778742

Reverting back locally to 5.3.19 made the tests pass again so I could pinpoint to issue to something done in between those releases.

I stumbled upon this PR.
When I revert line 180 in the Illuminate\Support\ServiceProvider class back to its original state:

$events = $this->app['events'];
    $events->listen(ArtisanStarting::class, function ($event) use ($commands) {
            $event->artisan->resolveCommands($commands);
});

It works.

But when using the updated version

Artisan::starting(function ($artisan) use ($commands) {
    $artisan->resolveCommands($commands);
});

It fails.

I don't understand why though. Something in particular that could point me in the right direction ?

Thanks,

public static function starting(\Closure $callback)
{
static::$bootstrappers[] = $callback;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should automatically run $callback when calling starting() after Illuminate\Console\Events\ArtisanStarting has been called.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Illuminate\Foundation\Console\Kernel::registerCommand() will resolve Illuminate\Console\Application when it call getArtisan(), if any part of the code execute this too early (before all events registered) it's not going to be called again.

@themsaid
Copy link
Member Author

@nWidart how do you register the command.media.refresh command?

@nWidart
Copy link
Contributor

nWidart commented Oct 31, 2016

Hello, thanks for answering.

It's registered here: https://github.com/Idavoll/Media/blob/2.0/Providers/MediaServiceProvider.php#L97

$this->app->singleton('command.media.refresh', function ($app) {
    return new RefreshThumbnailCommand($app['Modules\Media\Repositories\FileRepository']);
});
$this->commands('command.media.refresh');

About the Kernel::registerCommand() usage, I don't recall having used that.

Thanks,

@themsaid
Copy link
Member Author

The error means command.media.refresh isn't registered in the container yet, this is really confusing. I can't regenerate it on a fresh 5.3 installation, looks like there's something special your doing in this app.

@mzur
Copy link
Contributor

mzur commented Jan 4, 2017

This PR makes my tests 50% slower because the bootstrappers keep stacking up for each test case. If I implement clearing the bootstrappers like this after each test case, the tests run as fast as before. @themsaid can you please have a look at the changes and see if it makes sense? I'll open a PR then.

Edit: I can just as well create the PR right away and continue the discussion there.

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.

6 participants