[RFC|Merged] Modular cron

These requests for comments/change have lead to an implemented feature that has been successfully merged into the 3.1/Ascraeus branch. Everything listed in this forum will be available in phpBB 3.1.
Post Reply
Oleg
Posts: 1150
Joined: Tue Feb 23, 2010 2:38 am
Contact:

Re: System cron and cron tasks in mods

Post by 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
Posts: 1150
Joined: Tue Feb 23, 2010 2:38 am
Contact:

Re: System cron and cron tasks in mods

Post by Oleg »

eviL3 wrote: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
Registered User
Posts: 500
Joined: Thu Jan 04, 2007 11:47 pm

Re: System cron and cron tasks in mods

Post by igorw »

nn- wrote: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.
nn- wrote: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.
nn- wrote: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!

User avatar
naderman
Consultant
Posts: 1727
Joined: Sun Jan 11, 2004 2:11 am
Location: Berlin, Germany
Contact:

Re: System cron and cron tasks in mods

Post by naderman »

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

igorw
Registered User
Posts: 500
Joined: Thu Jan 04, 2007 11:47 pm

Re: System cron and cron tasks in mods

Post by igorw »

Fine with me. nn-, your call.

Oleg
Posts: 1150
Joined: Tue Feb 23, 2010 2:38 am
Contact:

Re: System cron and cron tasks in mods

Post by Oleg »

I'm ok with merging.

Oleg
Posts: 1150
Joined: Tue Feb 23, 2010 2:38 am
Contact:

Re: System cron and cron tasks in mods

Post by 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
Registered User
Posts: 500
Joined: Thu Jan 04, 2007 11:47 pm

Re: System cron and cron tasks in mods

Post by igorw »

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

Oleg
Posts: 1150
Joined: Tue Feb 23, 2010 2:38 am
Contact:

Re: System cron and cron tasks in mods

Post by 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
Registered User
Posts: 500
Joined: Thu Jan 04, 2007 11:47 pm

Re: System cron and cron tasks in mods

Post by 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.

Post Reply