Skip to content
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

Open
wants to merge 30 commits into
base: trunk
Choose a base branch
from

Conversation

nhaagen
Copy link
Contributor

@nhaagen nhaagen commented Dec 16, 2024

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.

@nhaagen nhaagen marked this pull request as ready for review December 16, 2024 12:25
Copy link
Member

@klees klees left a 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:

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!

Copy link
Contributor

@thibsy thibsy left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants