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

[6.x] Add events to signal when scheduled task runs #29888

Merged
merged 6 commits into from
Sep 13, 2019
Merged

[6.x] Add events to signal when scheduled task runs #29888

merged 6 commits into from
Sep 13, 2019

Conversation

michaeldyrynda
Copy link
Contributor

We already have the Illuminate\Console\Events\CommandStarting and Illuminate\Console\Events\CommandFinished events, but these are fired for any command that is run, be it via the CLI directly, or via schedule:run.

Having the scheduled-task specific events makes it much easier to identify when a scheduled task runs, allowing better monitoring of these tasks in a more dynamic manner.

@taylorotwell
Copy link
Member

taylorotwell commented Sep 6, 2019

On single server events it seems to fire even when the event is never actually run on the given server.

@michaeldyrynda
Copy link
Contributor Author

Right you are. I’ll update this PR to get the events to dispatch only within the runEvent method.

@michaeldyrynda
Copy link
Contributor Author

Should be good for another look @taylorotwell

@driesvints
Copy link
Member

@michaeldyrynda definitely not a requirement but any chance you can use rebase instead of merging in the 6.x branch into your PR? Keeps the commit history nice and clean.

@michaeldyrynda
Copy link
Contributor Author

Did the rebase @driesvints 😘

@taylorotwell taylorotwell merged commit 07cf996 into laravel:6.x Sep 13, 2019
@simonschaufi
Copy link

Out of curiosity, shouldn't the new tag be 6.1 for new features?

MINOR version when you add functionality in a backwards compatible manner

@GrahamCampbell
Copy link
Member

Yeh, it should have been.

@michaeldyrynda
Copy link
Contributor Author

The rest of that line says “... to the public API.”

It’s not clear to me if adding events is part of the public API, though. For the record, I was expecting a minor bump with this change, but it’s a gray area imo.

The other stuff that went into this release added test helper methods and I feel that those aren’t public API of the framework, either.

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.

None yet

5 participants