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

[8.x] Simulate exit code for closure scheduled tasks #33914

Merged
merged 2 commits into from
Aug 18, 2020
Merged

[8.x] Simulate exit code for closure scheduled tasks #33914

merged 2 commits into from
Aug 18, 2020

Conversation

jszobody
Copy link
Contributor

@jszobody jszobody commented Aug 17, 2020

Currently, the onSuccess and onFailure hooks do not work for closure scheduled tasks. This is because these hooks rely on the exitCode, which is only tracked for external process calls.

That means in this case...

$schedule->call(function() {
    info('hello world');
})->onFailure(function() {
    info('task failed');
})->everyMinute();

... you will always find that the "task failed".

This PR aims to simulate a 0 (success) or 1 (general failure) exit code for closure tasks. An exit code of 1 is set if an exception is thrown OR false is returned from the closure. All other results are considered successful.

This allows the onSuccess and onFailure hooks (along with the related emailOutputOnFailure type methods) to work with closure tasks.

@taylorotwell taylorotwell merged commit 42b4fed into laravel:master Aug 18, 2020
@mdevo
Copy link

mdevo commented Sep 2, 2020

Does this fixes applied on LTS also?

@driesvints
Copy link
Member

@mdevo no this is a breaking change.

@ariaieboy
Copy link

in what version of laravel 8 this bug will get fix ?

@GrahamCampbell
Copy link
Member

GrahamCampbell commented Sep 19, 2020

in what version of laravel 8 this bug will get fix ?

Every version of Laravel 8.

@GrahamCampbell
Copy link
Member

GitHub tells you:

image

@ariaieboy
Copy link

i test this on laravel version 8.3.0 and 8.5.0 and its still not work
this is my scheduled job
$schedule->call(function (){ var_dump('test'); })->onFailure(function (){ var_dump('job failed'); })->onSuccess(function (){ var_dump('job succesd'); });
and its the output of my job
Running scheduled command: Closure string(4) "test" string(10) "job failed"

i tested it with closure and __invoke method in a class and with $schedule->job()
and in all this situations it always run the onFailure function;

@jszobody
Copy link
Contributor Author

jszobody commented Sep 19, 2020

Confirmed. My mistake. Currently the callbacks are running before the exitCode is being set, looks like.

Line 90 here needs to be moved up, probably up to right after the $response is obtained inside the try block.

parent::callAfterCallbacks($container);
}
$this->exitCode = $response === false ? 1 : 0;

@GrahamCampbell I can submit a new PR to fix this, will try to get to it soon.

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