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

Console Schedule call does not call before() #18851

Closed
codepilotde opened this issue Apr 18, 2017 · 5 comments
Closed

Console Schedule call does not call before() #18851

codepilotde opened this issue Apr 18, 2017 · 5 comments

Comments

@codepilotde
Copy link

codepilotde commented Apr 18, 2017

  • Laravel Version: 5.3.9
  • PHP Version: 7.0.11

Console Schedule call does not call before() Callbacks :

Steps To Reproduce:

Create a simple call schedule with on callBackfunction and
a before() and after call(). In my example the before method dd('before') the after dd('after).

    /**
     * Define the application's command schedule.
     *
     * @param  \Illuminate\Console\Scheduling\Schedule $schedule
     * @return void
     */
    protected function schedule(Schedule $schedule)
    {
        $schedule->call(function() {
            echo "this is the output of the cron".PHP_EOL;
        })->everyMinute()
            ->before(function () {
               echo("before").PHP_EOL;
            })
            ->after(function () {
              echo("after").PHP_EOL;
            })->sendOutputTo("/tmp/test.txt")
            ->emailOutputTo('[email protected]');
    }

I chose a minutely interval so you can easily run
php artisan schedule:run

The expected behavior and output should be

Running scheduled command: Closure
before
this is the output of the cron
after
@jhoff
Copy link
Contributor

jhoff commented Apr 19, 2017

Tested this locally and it seems to be isolated to closure based scheduling events:

This does not echo "before"

$schedule->call(function() {
    echo "this is the output of the cron".PHP_EOL;
})
->everyMinute()
->before(function () {
    echo("before").PHP_EOL;
});

However, this does output "before", as expected:

$schedule->command('inspire')
->everyMinute()
->before(function () {
    echo("before").PHP_EOL;
});

@jhoff
Copy link
Contributor

jhoff commented Apr 19, 2017

Dug a little more...

With a command based scheduling event, by default will run in the foreground, meaning it will call the before callbacks, execute and wait for the command to finish and then call the after callbacks. If you add ->runInBackground() to the schedule, then it will call the before callbacks, start the command but release the process and finish immedately, so there is no reason to call the after callbacks.

However, in this case, with a closure based event, I'm thinking it may be intentional that the before callbacks are never called. I suppose the thought process might be that the code to be executed is already right there... why not just put the before code in the closure? Example:

$schedule->call(function() {
    echo("before").PHP_EOL;
    echo "this is pretty much functionally the same".PHP_EOL;
})
->everyMinute();

It'd be easy enough to add parent::callBeforeCallbacks($container); to Illuminate\Console\Scheduling\CallbackEvent::run() which I've confirmed would fix the issue and as far as I can tell shouldn't hurt anything.

@jhoff
Copy link
Contributor

jhoff commented Apr 19, 2017

Submitted a pull request for 5.5 to fix this issue.

@codepilotde
Copy link
Author

I agree with what you say. And I reproduced the same with call,command and exec exec as @jhoff wrote above.

$schedule->call(function() {
    echo("before").PHP_EOL;
    echo "this is pretty much functionally the same".PHP_EOL;
})
->everyMinute();

Right from functional aspects, but what I wanted to do was to generalize some logging functionality for cronjobs with the before() and after() methods and to make it clean and pretty.
But so far I'll use a work around with artisan functions for my jobs. And use a custom repo with the same fix you provided above.

@themsaid
Copy link
Member

Thank you for the report, moving the discussion to the opened PR #18861

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

3 participants