[RFC|Merged] Modular cron

[Oleg]

Commit: http://github.com/p/phpbb3/commit/6429e ... 681e807b66

One of run methods is missing its implementation, and the new classes are not hooked up into execution path yet.

For the filenames, perhaps:

Code: Select all

includes/cron/tasks/prune_all_forums.php

Are you suggesting all mods put their cron tasks in the same directory as phpbb core? I think some sort of namespacing by mod is necessary.

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

I used prune_all_forums_cron_task, but I'm open to inverting it to cron_task_prune_all_forums.

Perhaps cron_task_core_prune_all_forums, and the file location includes/cron/tasks/core/prune_all_forums.php?

Could you give a slightly more verbose description of how getUrlQueryString() works?

Consider forum pruning. In 3.0 whenever a forum is viewed, it may be scheduled for pruning. Scheduling piggybacks onto other queries and requires no additional queries. Thus one forum is pruned at a time, and the forum id is passed from viewforum.php to cron.php.

This forum id parameter is required for 'prune one forum' task to function. It is passed from cron scheduler to cron runner, in 3.0 case, via the url. get_url_query_string takes whatever parameters a cron task takes (through its constructor, see http://github.com/p/phpbb3/blob/6429e83 ... _forum.php) and returns them as (part of) query string, which will then be used by cron class to construct correct cron.php url.

Reference: http://tps.projects.bsdpower.com/browse ... m.php#L195

If pruning now is invoked from system cron, we are going to check all forums for prunability and prune all forums as necessary. This explains two pruning tasks.

In what cases would isShutdownFunctionSafe() return false?

It's a port of this: http://tps.projects.bsdpower.com/browse ... on.php#L82

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.

To be honest I have my hands full just shuffling the code around. I certainly have not planned on altering actual code until the shuffling is finished. If you think dependency injection should happen in parallel with the shuffling, or that it needs to be considered in the architecture, please do elaborate.

[nickvergessen]

Instead of include_once you should use

Code: Select all

if (!function_exists(''))
{
   include();
}

f.e. in http://github.com/p/phpbb3/blob/6429e83 ... gs.php#L34

same file:

Code: Select all

return !!$config['warnings_expire_days'];

is also a little lalala, we do not use such !! in phpBB code.

and http://github.com/p/phpbb3/blob/6429e83 ... gs.php#L54 should see some brackets

But the direction is right to go I think.

[Oleg]

Instead of include_once you should use

Code: Select all

if (!function_exists(''))
{
   include();
}

f.e. in http://github.com/p/phpbb3/blob/6429e83 ... gs.php#L34

Ok.

same file:

Code: Select all

return !!$config['warnings_expire_days'];

is also a little lalala, we do not use such !! in phpBB code.

!!value is a pretty common idiom for converting a value to boolean. It has the property of working (and making sense) on all types. What do you suggest instead?

and http://github.com/p/phpbb3/blob/6429e83 ... gs.php#L54 should see some brackets

Are you really that unsure of operator precedence? I don't believe any self-respecting developer can claim that arithmetic operations have lower precedence than comparisons.

Additional brackets are an extra burden to read when the expression is perfectly clear without them.

[nickvergessen]

!!value is a pretty common idiom for converting a value to boolean. It has the property of working (and making sense) on all types. What do you suggest instead?

(bool) ?

Are you really that unsure of operator precedence? I don't believe any self-respecting developer can claim that arithmetic operations have lower precedence than comparisons.
Additional brackets are an extra burden to read when the expression is perfectly clear without them.

http://area51.phpbb.com/docs/coding-gui ... codelayout see "Operator precedence:"
so

Code: Select all

return ($config['warnings_last_gc'] < (time() - $config['warnings_gc']));

is the coding guidelines version of it

[bantu]

About the coding guidelines thing I'd say the current coding guidelines should be applied to code that is supposed to be commited into the main repository.

However, I think there is some room for improvement in the coding guidelines. If you're not happy with something in the current coding guidelines, feel free to come up with a patch and/or post an RFC. We should probably do this sooner rather than later.

[Oleg]

I added missing run method implementations: http://github.com/p/phpbb3/commit/d05cd ... ca85cbfadc

I'd like to reach consensus on file naming at least, and ideally on configuration too, before reworking the runner code.

[igorw]

I think some sort of namespacing by mod is necessary.
...
I used prune_all_forums_cron_task, but I'm open to inverting it to cron_task_prune_all_forums.

This might actually be a good idea to make sure there are no naming conflicts. In phpBB there is "class phpbb_captcha_nogd extends phpbb_default_captcha", I am leaning towards the inversion you suggested. It is awfully long, but cron_task_core_prune_all_forums sounds like the most fail-proof convention.

This is a potential RFI/LFI: http://github.com/p/phpbb3/blob/feature ... on.php#L41
basename() may not be possible if we want a core/ subfolder. But it should at least check for ".." and null bytes.

http://github.com/p/phpbb3/blob/feature ... n.php#L106
should probably be $mode_param, also bad formatting on following line.

Looks pretty good, some parts still seem a little complex and could perhaps be simplified. get_url_query_string() is an easy way of doing it, but it strongly depends on HTTP. A CLI script cannot set any of these options (without ugly hacks) because they are obtained through request_var(). A more generic way of passing these options may be useful.

To be honest I have my hands full just shuffling the code around. I certainly have not planned on altering actual code until the shuffling is finished. If you think dependency injection should happen in parallel with the shuffling, or that it needs to be considered in the architecture, please do elaborate.

Considering that cron relies on phpBB's API which heavily relies on any globals it's probably a waste of time to rip out those dependencies for the cron system. All cron does is call phpBB functions that have exactly the same constraints.

[bantu]

I'd like to reach consensus on file naming at least, and ideally on configuration too, before reworking the runner code.

I'd suggest to use cron_tasks/ for the phpBB tasks and add the folder "cron_tasks/mods/" for modifications. That would be consistent with language and other locations.

[ToonArmy]

If there is a cron folder inside includes do we really need a cron_tasks folder as well, surely just tasks inside there as Igor suggested.

[Oleg]

Update:

1. Renamed task files. I went with includes/cron/tasks/core/tidy_search.php because it makes discovery code simpler and naming more consistent.

2. Moved all cron-related classes into includes/cron.

3. Dropped configuration pertaining to enabling/disabling cron tasks. It is not present in 3.0, UI I had for it was pretty bad, cron tasks can already disable themselves and some do.

4. Changed parametrization to operate on arrays of parameters instead of url parts. This should make cron classes more usable.

5. Added cron task wrapper class. There are several methods that are convenient to have on cron tasks, but whose implementation is identical across all tasks and that really have more to do with invoking cron tasks than defining them. Cron task interface continues to concentrate on important aspects of task definition. http://github.com/p/phpbb3/blob/db86340 ... rapper.php

http://github.com/p/phpbb3/compare/git- ... ystem-cron

Now I need to figure out how to test this code efficiently.

One thing I'm wondering is why does cron.php need append_sid?