[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: [RFC|Accepted] Modular cron

Post by Oleg »


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

Re: [RFC|Accepted] Modular cron

Post by igorw »

Thanks! I'll see if I can come up with some more. And then we should finally be able to merge this. :)

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

Re: [RFC|Accepted] Modular cron

Post by naderman »

Alright I've rebased this onto a current version of develop and made some changes. See the list of commits here https://github.com/naderman/phpbb3/comm ... ystem-cron

The changes were:
  • Moved the cron lock into a generic databse locking class
  • Caching cron task names for 1 hour
  • Added a few type hints
  • Using RecursiveDirectoryIterator instead of readdir() - less code, and tasks can also be stored in subdirectories
  • Removed global use from the manager
  • Integrated the new config and cache classes into manager and tests
  • Modified the tests to reference dummy tasks inside of the test directory
  • Renamed lock::lock() to lock::acquire() and lock::unlock() to lock::release()
What we are still missing in my opinion are tests for the actual cron tasks as well as some refactoring of their code to avoid the extensive use of globals.

User avatar
bantu
3.0 Release Manager
3.0 Release Manager
Posts: 557
Joined: Thu Sep 07, 2006 11:22 am
Location: Karlsruhe, Germany
Contact:

Re: [RFC|Accepted] Modular cron

Post by bantu »

You do the caching now, you can remove the comment.
* Todo: consider caching found task file list in global cache.

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

Re: [RFC|Accepted] Modular cron

Post by naderman »

Fixed.

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

Re: [RFC|Accepted] Modular cron

Post by Oleg »

I reinstated changes from my last amend here: https://github.com/p/phpbb3/tree/feature/system-cron (2 commits).

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

Re: [RFC|Accepted] Modular cron

Post by naderman »

Thanks, a few things

https://github.com/p/phpbb3/commit/cbf2 ... e982a4b56a
The doc comment no longer matches what the actual function does. The parameter is called $task_path and it must not be null. Whether or not the path ends in a slash is irrelevant in the current implementation.

https://github.com/p/phpbb3/commit/e2a0 ... 57eb34b9d2
We should test equals here, not greater than. To make sure the same tasks are not listed multiple times or anything like that.
I think assertContains should be used instead of assertTrue(array_contains, to make the resulting error message more precise. Besides, there's really no reason to write a wrapper around in_array just because you personally dislike its argument order.
There should not be any global functions in a test file. make them protected methods if they are necessary.
Why are the assertions in tests/cron/task_core_queue_test.php commented out? Or rather what is that test meant to do at all?

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

Re: [RFC|Accepted] Modular cron

Post by Oleg »

I will address everything else.
naderman wrote: Or rather what is that test meant to do at all?
That was as far as I got with testing the actual tasks. That test may go into a separate branch/issue for testing the tasks.

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

Re: [RFC|Accepted] Modular cron

Post by naderman »

Ah I see. Well I'd say either remove it from that commit or make it work :D

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

Re: [RFC|Accepted] Modular cron

Post by Oleg »

The empty test is here for future reference: https://gist.github.com/797930

Added two commits and rebased everything for good measure: https://github.com/p/phpbb3/commits/fea ... ystem-cron

Post Reply