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.5] Use singleton during Schedule creation to allow "resolving" callbacks #20933

Merged

Conversation

aik099
Copy link
Contributor

@aik099 aik099 commented Sep 2, 2017

By using singleton vs instance method it's now possible for packages to hook into resolving process of Schedule class. For example adding scheduled events from database would look like this:

// In `boot` method of package Service Provider:
$this->app->resolving(\Illuminate\Console\Scheduling\Schedule::class, function ($schedule) {
  // add schedules here
});

Before similar functionality would require code like this:

// In `boot` method of package Service Provider:
if ($this->app->runningInConsole()) {
  // add schedules here
};

The problem is checking for Console for every request, including Http ones.

@aik099
Copy link
Contributor Author

aik099 commented Sep 2, 2017

Or maybe the Container::instance method can be improved to send resolving and resolved events.

@laurencei
Copy link
Contributor

I dont see what the limitation is with the current implementation? It works perfectly fine - many packages already interact with the scheduler without issue...

@aik099
Copy link
Contributor Author

aik099 commented Sep 2, 2017

The limitation is in fact, that you can't alter class instance bound to container via Container::instance method (Schedule in this case) at resolve time only. But you can do that with classes bound via Container::singleton method.

This results in a need of writing code, like checking if we're in Console or not, just to avoid getting object from container before it's created naturally in the application.

Maybe we should do #20933 (comment) instead.

@taylorotwell taylorotwell merged commit 58c179b into laravel:5.5 Sep 2, 2017
@aik099
Copy link
Contributor Author

aik099 commented Sep 2, 2017

Ah, and I forgot to mention, that I've removed passing Cache instance to the Schedule class, because it's not using it. I guess it was refactored some time ago to let CacheMutex to get Cache directly.

@GrahamCampbell GrahamCampbell changed the title Use singleton during Schedule creation to allow "resolving" callbacks [5.5] Use singleton during Schedule creation to allow "resolving" callbacks Sep 5, 2017
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.

3 participants