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

Improve "Illuminate\Console\Scheduling\Schedule" configuration #28

Closed
aik099 opened this issue Sep 1, 2017 · 10 comments
Closed

Improve "Illuminate\Console\Scheduling\Schedule" configuration #28

aik099 opened this issue Sep 1, 2017 · 10 comments

Comments

@aik099
Copy link

aik099 commented Sep 1, 2017

Right now in the \Studio\Totem\Providers\ConsoleServiceProvider::register method the booted event is being listened and then app is run in console, then new schedules are added into application.

Proposing to simplify this by:

  1. using \Studio\Totem\Providers\ConsoleServiceProvider::boot method instead of \Studio\Totem\Providers\ConsoleServiceProvider::register method
  2. do the $this->app->resolving(Illuminate\Console\Scheduling\Schedule::class, function (...
  3. don't check for console

This way whoever wants the schedule class would get it with all schedules already added.

Not sure though how this thing could be tested or if resolving events are triggered in unit tests.

@aik099 aik099 changed the title Improve way, how "Illuminate\Console\Scheduling\Schedule" is configured Improve "Illuminate\Console\Scheduling\Schedule" configuration Sep 1, 2017
@roshangautam
Copy link
Collaborator

Thanks @aik099 . I will look into this. Definitely sounds like an improvement.

@roshangautam
Copy link
Collaborator

Have you tried this ? For some reason resolving callback is not triggered for Illuminate\Console\Scheduling\Schedule.

@aik099
Copy link
Author

aik099 commented Sep 2, 2017

I haven't tried that myself. Are you doing $this->app->resolving(... in the boot method of your service provider?

@roshangautam
Copy link
Collaborator

Yup, tried it in boot method. Looks like the Schedule class is not resolved until its required. Will have to dig deeper to figure out whats going on.

@aik099
Copy link
Author

aik099 commented Sep 2, 2017

Debugging revealed, that resolving callback is only called when doing Container::make method. Unfortunately schedule is created from instance in \Illuminate\Foundation\Console\Kernel::defineConsoleSchedule method:

$this->app->instance(
    Schedule::class, $schedule = new Schedule($this->app[Cache::class])
);

If that could would be written differently, e.g. via singleton method, then probably the resolving callback would have been called.

@aik099
Copy link
Author

aik099 commented Sep 2, 2017

Replacing above code (in Laravel core) with this one does the trick:

$this->app->singleton(Schedule::class, function ($app) {
    return new Schedule($app[Cache::class]);
});

$schedule = $this->app->make(Schedule::class);

I've created PR into Laravel 5.5: laravel/framework#20933

@aik099
Copy link
Author

aik099 commented Sep 2, 2017

At least you can simplify existing code (would work the same way, but look better) by moving code from booted callback into boot method of the service provider.

@roshangautam
Copy link
Collaborator

This is really nice. I guess I will have to maintain two separate branches for 5.4 and 5.5 as I dont think this will be merged to 5.4 branch. Thanks for your insight and help. I might use your expertise in one another issue I am facing here

@aik099
Copy link
Author

aik099 commented Sep 3, 2017

Since 5.5 is already released, then my change would only be in 5.5.x release. So having 2 branches won't really do the trick.

Maybe you'll have to do some feature detection and decide which code to run.

@roshangautam
Copy link
Collaborator

I have incorporated the changes in latest releases for both 5.4 and 5.5 branches. Thanks for all your help @aik099.

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

No branches or pull requests

2 participants