https://github.com/p/phpbb3/commit/170f ... 79f1f7c033
This is all I can manage.
[RFC|Merged] Modular cron
Re: [RFC|Accepted] Modular cron
Thanks! I'll see if I can come up with some more. And then we should finally be able to merge this.
Re: [RFC|Accepted] Modular cron
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:
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()
- bantu
- 3.0 Release Manager
- Posts: 557
- Joined: Thu Sep 07, 2006 11:22 am
- Location: Karlsruhe, Germany
- Contact:
Re: [RFC|Accepted] Modular cron
You do the caching now, you can remove the comment.
* Todo: consider caching found task file list in global cache.
Re: [RFC|Accepted] Modular cron
I reinstated changes from my last amend here: https://github.com/p/phpbb3/tree/feature/system-cron (2 commits).
Re: [RFC|Accepted] Modular cron
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?
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?
Re: [RFC|Accepted] Modular cron
I will address everything else.
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.naderman wrote: Or rather what is that test meant to do at all?
Re: [RFC|Accepted] Modular cron
Ah I see. Well I'd say either remove it from that commit or make it work
Re: [RFC|Accepted] Modular cron
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
Added two commits and rebased everything for good measure: https://github.com/p/phpbb3/commits/fea ... ystem-cron