-
Notifications
You must be signed in to change notification settings - Fork 343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Component/Cron: init cron jobs via SetupAgent and Component's seek #8731
base: trunk
Are you sure you want to change the base?
Conversation
2b14c19
to
6192bd9
Compare
6192bd9
to
3bad331
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @nhaagen,
thanks a lot for looking into this. Looks promising afaic.
Please answer the following questions:
- hello world: Why do we want this change in
hello_world.php
? - BookingManager: Why do we need this
$registration
construction? When would$registration = false
? - Metrics: Could you explain a little, what the metrics for the cronjobs will do?
Please consider the following suggestions. You do not need to follow those, but but please indicate shortly why you prefer to do otherwise:
- named objective: Lets publish
ilCronjobsRegisteredObjective
as a named objective too. People could then be really sure that all cronjobs are registered indeed...
I guess we should do one iteration before handing this over to @mjansenDatabay. Also: Please remember to open an according PR for the test plugin repo.
Kind regards!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @nhaagen,
I was curious about the integration mechanism here, so I briefly looked over the changes. I have some questions in addition to @klees' review - could you answer them as well?
- why do contributors of cron job instances need to prepend
components\
to the component class name? Is this important or just an aesthetic? Would it make sense to centralise this? - I was having an issue with
ILIAS\Language\Language::loadLanguageModule()
calls inside the constructor of classes during the migration of the UI framework initialisation ([FEATURE] UI: Implement initialisation using new component mechanism #7969). Did you not run into the same issue here? - why are you using a "mock" instance of
ILIAS\Language\Language
inside the new cron objectives? Isn't there a dedicated implementation for the setup available?
Thx for your time!
kind regards,
@thibsy
Move registration of cron-jobs to component-level with contribute / seek and remove xml-based processor.
Construction of CronJobs is still an issue (as stated in the roadmap already); I circumvented this with an additional flag in __construct, which was the easiest to stay in the scope of this PR.