[RFC|Merged] Modular cron

[Oleg]

Something which ought to be improved in my implementation is specifying 'enabled' cron tasks. I took the simplest approach that works: a config setting consisting of enabled task modules (to be changed to classes, probably) separated by commas:

standard,antispam_mod,thanks_mod

This approach however is not user-friendly:

1. There is no list of all possible modules that can be entered
1a. If a module is removed temporarily and is not added back, and later someone views this setting, it is virtually impossible to realize that a module is missing
2. It is easy to make a typo and add/remove a letter somewhere in this string, breaking functionality

Better UI is clearly desirable. A solution would need to account for modules installed by modifications.

[Oleg]

It would be nice if the tasks were more extensible/generic, with one file per task, either implementing or extending an interface or base class.

While I see some benefits of this, would you mind writing them down? Doing so will probably formalize some of the requirements and/or 'nice to have' items that the implementation can be judged against.

Perhaps CRON_ID can be defined outside of the cron_lock class or replaced with a static property (which isn't great either, but at least more namespaced).

I only kept it because it was in original code. There is no other reason to keep it as a constant. In my implementation CRON_ID should actually be a plan instance variable.

Are we targeting 3.x line and php4 compatibility?

[igorw]

While I see some benefits of this, would you mind writing them down? Doing so will probably formalize some of the requirements and/or 'nice to have' items that the implementation can be judged against.

There are two main reasons for putting each task in it's own class. The code is less messy to begin with and follows a specific pattern. It allows you to add functionality simply by placing a new class in a specific folder, without editing any files.

There are additional benefits. The whole system is more extensible, because you can extend existing tasks, overwriting parts of them. You can apply a wide range of OO-patterns if needed. By defining an interface you decouple your code from any specific implementation, allowing any class to become a task.

I only kept it because it was in original code. There is no other reason to keep it as a constant. In my implementation CRON_ID should actually be a plan instance variable.

You're right. I assumed it would be accessed somewhere from within phpBB3 since it's a constant, but this is not the case.

Are we targeting 3.x line and php4 compatibility?

I would say we are targeting 3.1 or 3.2, which will both be PHP 5.2. What a relief!

[naderman]

I would say this should really target 3.1 and PHP 5.2+, should this be merged with the other cron topic?

[igorw]

Fine with me. nn-, your call.

[Oleg]

I'm ok with merging.

[Oleg]

I started redoing tasks as classes and I just realized that I changed all underscored function names to camel case. Should I change them back to underscores or do we eventually plan on moving toward camel case?

[igorw]

I doubt the naming convention will change with 3.x, so I'd go with underscores.

[Oleg]

Proposed cron task interface: http://gist.github.com/366810#file_cron_task.php

Obviously method names would be changed to underscores.

Parametrized cron task is for prune single forum task, which is half cron task and half delayed job in that it is invoked explicitly in one location.

Implementation looks to be much cleaner now, I will commit it once I have renamed everything.

We need a naming convention for cron tasks. Do we organize them in modules (subdirectories)? How would they be discovered? How would users name them when deciding which tasks to enable?

Current structure is thus:

Code: Select all

includes/cron_tasks/standard/prune_all_forums.php
includes/cron_tasks/standard/prune_forum.php
includes/cron_tasks/standard/queue.php
includes/cron_tasks/standard/tidy_cache.php
includes/cron_tasks/standard/tidy_database.php
includes/cron_tasks/standard/tidy_search.php
includes/cron_tasks/standard/tidy_sessions.php
includes/cron_tasks/standard/tidy_warnings.php


[igorw]

For the filenames, perhaps:

Code: Select all

includes/cron/tasks/prune_all_forums.php

The class name being "cron_task_prune_all_forums". Or "cron_prune_all_forums". Or "phpbb_cron_prune_all_forums".

There is not much consistency within phpBB, looking at dbal, captcha, cache, auth plugins/modules. In the case of separating the "default" tasks from custom ones, I'd suggest using the term "core".

Interface looks good. Could you give a slightly more verbose description of how getUrlQueryString() works? In what cases would isShutdownFunctionSafe() return false? We should also take a look at the proposed dependency injection, I'd be glad to provide some more description of how that could work.