Standard interface to run cron jobs, precluding the need for routes

[ameenross]

It isn't clear to me where to put my cron definitions. I am writing a package that requires cron to do some pulling of data from a remote source. I defined a cron rule in my service provider's boot method, as that seems like the right place for it.

Apparently through, the facade for the "cron" class is added after my boot method ran, which means I can only use it with the fully qualified class name. I could, however, get it working by editing CronServiceProvider.php to directly add the facade in the register method, like so:

$loader = \Illuminate\Foundation\AliasLoader::getInstance();
$loader->alias('Cron', 'Liebig\Cron\Facades\Cron');

Am I doing something wrong, should I put my definition somewhere else? Is the above suggestion flawed for some reason? Or should I just put the facade in my app.config?

[ameenross]

I should note that I don't want to use a route for my cron definitions. I also figured loading the cron definitions on every bootstrap introduces unnecessary system load. So I can only think of 2 "proper" ways to add cron runs now: via a route or via an artisan command (both manually added). IMO both are undesirable, as they require more than just defining a cron job.

A better solution IMO would be that Cron defines its own artisan command (and optionally a route), which then collects all cron definitions, perhaps by triggering an event. When all event listeners have registered their cron definitions, the command can run Cron::run()

An simple example of code implementing a cron run using such a mechanism would be:

Event::listen('cron.collect', function() {
    // A cronjob which will run every minute.
    Cron::add('SomeCronJob', '* * * * *', function() {
        return SomeCronJobHandler::cron();
    });
});

[liebig]

Hi ameenross,

thanks for using Cron. Maybe your event listener is a good idea. But where will you define your jobs if you don't use a route or a command? Then it will be somewhere in the bootstrap and this will lead to an unnecessary system load.

[ameenross]

Yeah, I mentioned that concern as well, but in this case the only thing that is defined is an event listener and I assume that it incurs a lower overhead. It also makes it much easier for package developers to define their cron runs, and additionally means that app developers don't need to worry about creating a route. All they have to do is put php artisan cron:run in their crontab.
And maybe the solution is not optimal, another approach could be for the artisan command to execute some code of each service provider (for example ServiceProvider::cronJob if it exists) which would amount to the same functionality, but I figured that is a bit of an archaic way of implementing a hook system.

[ameenross]

I just did a small benchmark using tinker to test my assumption. Here is the code and results:

/* Add 1000 event listeners in a loop and measure the time taken. */
$start = microtime(TRUE);

for ($i = 0; $i < 1000; $i++) {
    Event::listen('cron.collect', function() {
        Cron::add("SomeCronJob$i", '* * * * *', function() {
            return SomeCronJobHandler::cron();
        });
    });
}

$time = microtime(TRUE) - $start;
print 'Time taken: ' . $time . "\n"; /* ~0.013s */

/* Add 1000 cron jobs in a loop and measure the time taken. */
$start = microtime(TRUE);

for ($i = 0; $i < 1000; $i++) {
    Cron::add("SomeCronJob$i", '* * * * *', function() {
        return SomeCronJobHandler::cron();
    });
}

$time = microtime(TRUE) - $start;
print 'Time taken: ' . $time . "\n"; /* ~0.23s */

So it's nearly a factor 20 faster to add an event listener, and an overhead of about a millisecond for 100 cron jobs is negligable IMO.

[liebig]

That looks good, thank you for the benchmark. Okay I will fire an event before calling the run method and I will add a simple command which will run Cron and one command which will list all registered jobs. Did I miss something?

[ameenross]

I think that's about it, so basically we have:

php artisan cron:list

• Fires cron.collect event
• Prints the list of registered jobs

php artisan cron:run

• Fires cron.collect event
• Runs Cron::run()

By the way, as Cron::cronJobs is a private property, I think a function like Cron::listJobs is needed to return all registered cron jobs.

[liebig]

This is a great plan. I will work on this next week. Thank you very much.

[ameenross]

Great! Any progress so far? I'm willing to create a pull request implementing it, if you want.

[liebig]

Yes, everything should be finished. I tested everything with Windows, Debian is still outstanding but you can start testing, too.

For example add

Event::listen('cron.collectJobs', function($rundate) {
    Cron::add('example1', '* * * * *', function() {
                    // Do some crazy things successfully every minute
                    return 'Wuhu';
                });

    Cron::add('example2', '* * * * *', function() {
        // Do some crazy things successfully every minute
    });

    Cron::add('new name for disabled job', '* * * * *', function() {
        // Do some crazy things successfully every minute
    }, false);
});

to your global.php.

You can run the commands with php artisan cron:run and php artisan cron:list. I have added some more Events like cron.beforeRun, cron.collectJobs and cron.afterRun.

I hope you like it :)

[ameenross]

Cool! I hadn't noticed. Did you push the last stuff though? Because I don't see the beforeRun and afterRun events being fired. Also not seeing php artisan cron:run fire cron.collectJobs. Will test soon anyway.

[liebig]

Yes, everything is pushed. Have a look at the last commit 42dc9eb. The Debian tests are passed now.

[ameenross]

Ah right, I see you put the cron.collectJobs event trigger in Cron::run(). I think it would be better to put it in RunCommand::fire(). People using both the old and new method of running cron jobs might run into issues where their new cron jobs will also run when their legacy cron jobs are being run.

The cron.beforeRun and cron.afterRun events should stay in Cron::run(), as that is the right place for them and won't cause any compatibility breakage.

[liebig]

I thought about this but I want to give all people (using route and commands) the possibility to use this event for registering jobs. For example, if you want to switch from command to route (maybe only temporary), you can let your job definitions unchanged but can use a route where you only call the run method. The jobs in the boot file will be loaded, too. I like your idea with this event listeners so I think that route users maybe will use this functionality, too? What do you think?

[ameenross]

Well I agree with your reasoning, but I'm concerned about backward compatibility. Lets say an app developer has a route setup which runs a few cronjobs with the old method. They then install a package that defines a cronjob with the new method. The app developer ran a composer update and configured his crontab. Now he's in a situation where the legacy cronjobs he wrote will also run the newly installed package's cronjob (unless he first updates his code to use the new system). That means duplicate cron runs. Might not sound like a serious issue, but I know of situations (payment systems, I'm looking at you guys) where that would result in serious problems.

As for people needing to use the new system, just document it in the Readme and people will start picking it up. They will just have to move their Cron::add() calls somewhere else and change their route handler to run artisan cron:run. Simple and effective!

[liebig]

You win. I will move the \Event::fire('cron.collectJobs'); to the command.

[liebig]

Okay, it's done. Tests passed on Windows and Debian. Now it is your turn ;)

[moleculezz]

Hi, great feature. Just wanted to add, that you can always use semver, as a way to specify breaking changes.

[ameenross]

The way it's implemented now is not a breaking change though. Will test tomorrow but am confident it will work!

[moleculezz]

I was referring to the previous implementation. If you want to introduce a
breaking change, you can/should use semver to specify a breaking change.
A project/package consuming the package should define in his composer.json
to only allow non-breaking versions to be updated for the project.
These slides explains it nicely.

[liebig]

Thanks for your introduction, Moleculezz. I will take this into account in the future. But for now I have a problem. The new release has new features but does not break backwards compatibility. But the current release is v0.9.5. So what should I do? Make a v0.9.6, a v0.10.1 or a v1.0.0?

[moleculezz]

Semver defines versioning as the following: MAJOR.MINOR.PATCH
From what I understand, a new features without breaking backwards compatibility is a minor version.
So v0.10.0, would be appropriate.

[liebig]

Yes, I read the documentation and I agree with you. But I can't find a version in the document which has a number higher than 9. So I was confused.

[moleculezz]

If you look at definition number 2 on semver, you will see an example.

[ameenross]

From the FAQ:

How should I deal with revisions in the 0.y.z initial development phase?

The simplest thing to do is start your initial development release at 0.1.0 and then increment the minor version for each subsequent release.

How do I know when to release 1.0.0?

If your software is being used in production, it should probably already be 1.0.0. If you have a stable API on which users have come to depend, you should be 1.0.0. If you're worrying a lot about backwards compatibility, you should probably already be 1.0.0.

So I think personally that it would be good to release 1.0.0 soon. Maybe after you provide a route that calls Artisan::call('cron:run') with some sort of auto-generated security token stored in a configuration file. That way it will be easy, but safe for app developers to setup a third party cron service. I'll open up a separate issue for that, because I think it's another good idea :)

[ameenross]

By the way, I tested it and it works. Just have 2 ideas for cosmetics, first is to add a line ending after the output of the commands, the other is to create an output similar to the php artisan routes command. There might be a function to produce that output, but Laravel's API docs feel like a maze in the dark so I haven't found it yet (if it exists).

[ameenross]

Just found it, it's Symfony: http://symfony.com/doc/current/components/console/helpers/tablehelper.html

[liebig]

Table Helper is awesome! I will add it ;)

[ameenross]

Just created a pull request for it :)

[liebig]

Yeah, the request was added. Thank you. I was too slow :D The pull request for the run command was merged, too.

[liebig]

Can I close this? I think we implemented all requested features.

[ameenross]

yeah of course :) mission accomplished

[liebig]

Great, thank you :)