Please review and correct the codestyle used in all the xml

Looking at the language file I can see a lot of values which could be replaced with existing strings from global language files. This will reduce the workload on translators and also ensure a more consistent ui.

For example COM_CRONJOBS_FIELDSET_RULES is the string used to display a fieldset called JCONFIG_PERMISSIONS_LABEL

The feature seems to work just fine and is easy to use. The only issue I encountered has to do with the input fields on the 'new crownjob' page:

• negative input in the interval field leads to an error
• if you fill in all the necessary fields, but one with an illegal input, the 'Job Parameters' fields go missing:
  

Also, if you trash an existing crownjob, it will still apear in the crownjob list. Not sure if this is intended, but it's not realy clear where the diffrence between trashing and disabling is.

First of all

Hey, the cronjobs work well and the UI is simple and intuitive to use.

That's how I tested it

I went through the different cronjobs, created some and checked afterwards if they work. I also tested different inputs for the input options, for example the intervals.

This is what I found

Like the previous speaker, I noticed the inputs. There is also an error here if the inputs are too long for the intervals or if they contain a point/comma. In general, I think the times should not be written with a comma, there should be a separate field for seconds/minutes/hours or you can specify a format, for example day:hour:minute:second. Otherwise you could not, for example, enter 1:30 hours/minutes as a time period. I also noticed an unimportant detail in the UI, when you create a new cronejob, the plus becomes white, this should perhaps be blue or something. (But that's not important)

This happened when the input was too long.

This happened when entering a certain length.

Result

All in all, the feature is very nice and it works very well despite the little things. (They were really only small things)

Please ask your mentors how to format and indent the xml files.

JForm is @deprecated 5.0 Use Joomla\CMS\Form\Form instead.
JObject @deprecated 5.0 Use Joomla\CMS\Object\CMSObject instead (Although that class is also deprecated 4.0.0 apparently!)
getDBO is deprecated use $app->getContainer()->get('DatabaseDriver') instead

Also needs to have triggers to the User Action Log - because adding/editing/deleting of cron jobs is an action that should be logged - and therefore can be alerted on (Especially if you are going to allow crobjobs that can make external requests ... cause hackers going to love adding those kind of corncobs ;-))

Also should dispatch events in the case of success/failure of the running of a cronjob.

It also 100% MUST allow input of the cron expression using standard notation 5 digits * * * * *

Also I wonder if this feature should even be named "cronjobs" at all as they are NOT cron jobs.... they are just scheduled tasks, that are not even (optionally) run by any cron system at a operating system level.

Cron jobs are tasks added to a crontab on a unix based machine.

Cron is a technical term that probably not all Joomla users are familiar with. Something like "Scheduled Tasks" everyone can understand.

@PhilETaylor I have been pondering that myself and browsing the web looking at examples. The best I have seen to date was using "Cron Jobs, "Cron Tasks" as two separate words. But I wasn't happy that it provided the required clarity when I saw that in action in this PR.

Your suggestion, and reasoning, for "Scheduled Tasks" is much better.

Then the collective name could be simply Schedule and the individual name is Task.

Example
A new feature in 4.1 is the ability to Schedule new Tasks. You can add as many tasks as you want to the Schedule and each task can have its own time interval.

You can add as many tasks as you want

A nightmare scenario to handle with a web based pseudo cron...

each task can have its own time interval

Yes, but there is no forking of processes, so each still has to wait for the last to finish,

getDueJobs loads ALL jobs, but then executeDueJob only picks [0] from the array to run on onAfterRespond.. would be more performant to return only a single job and not all getItems.

// We only act on site requests
		if (!$this->app->isClient('site'))
		{
			return;
		}

Why? Why not run on /administrator/ as well? as there performance issues could better be tolerated.

You can add as many tasks as you want

A nightmare scenario to handle with a web based pseudo cron...

each task can have its own time interval

Yes, but there is no forking of processes, so each still has to wait for the last to finish,

It was an example of how the term could be used in text and nothing more. You were supposed to be commenting "that is so much clearer and easier to understand than using the word cron and cronjobs" ;)

I may or may not have considerable experience in creating multiple web based cron systems ;-)

Each cronjob should be capable of logging to its OWN log file and not just the central log of all cronjobs... makes debugging and also documenting each job easier.

@PhilETaylor

// We only act on site requests
		if (!$this->app->isClient('site'))
		{
			return;
		}

Why? Why not run on /administrator/ as well? as there performance issues could better be tolerated.

That's a great point I didn't consider before. If I remember right that's what the scheduler from #29592 did, so not sure if there was some reason but it made for easier testing. I can allow admin requests if there's no drawbacks.

Each cronjob should be capable of logging to its OWN log file and not just the central log of all cronjobs... makes debugging and also documenting each job easier.

Agreed, Do you think a per-job toggle for a standalone log file (besides the main log) should do?

getDueJobs loads ALL jobs, but then executeDueJob only picks [0] from the array to run on onAfterRespond.. would be more performant to return only a single job and not all getItems.

Good point! Right now its due a refactor as we move to the Console Commands (I'm thinking of moving some of the code to the component namespace or some other way it can be shared), so I'll have the fetching be more efficient when we get to that.

It also 100% MUST allow input of the cron expression using standard notation 5 digits * * * * *

This is planned as suggested by @bembelimen. We might end up completely replacing the multi-select style input with clear inputs as you described. What form design do you prefer for your web-based Cron systems?

Also should dispatch events in the case of success/failure of the running of a cronjob.

Noted!

Also needs to have triggers to the User Action Log - because adding/editing/deleting of cron jobs is an action that should be logged - and therefore can be alerted on (Especially if you are going to allow crobjobs that can make external requests ... cause hackers going to love adding those kind of corncobs ;-))

This will be done when we visit the manager next :)

JForm is @deprecated 5.0 Use Joomla\CMS\Form\Form instead.
JObject @deprecated 5.0 Use Joomla\CMS\Object\CMSObject instead (Although that class is also deprecated 4.0.0 apparently!)
getDBO is deprecated use $app->getContainer()->get('DatabaseDriver') instead

Thank you for the DBO thing, I believe we also had a todo for that. I've been curious why CMSObject is deprecated, it can be handy. 🤔

Thank you again for reviewing the code and the suggestions. I've been a little slow going through them while cleaning things up; adding the suggestions to our roadmap.

@brianteeman

Looking at the language file I can see a lot of values which could be replaced with existing strings from global language files. This will reduce the workload on translators and also ensure a more consistent ui.

For example COM_CRONJOBS_FIELDSET_RULES is the string used to display a fieldset called JCONFIG_PERMISSIONS_LABEL

Thank you for this review! I've identified the strings that have a global equivalent and will be replacing them soon.

@scout507 @socke300

Thank you for the review on the form. I'll work on fixing the discovered issues soon when we revisit management.

• if you fill in all the necessary fields, but one with an illegal input, the 'Job Parameters' fields go missing:
  

@scout507 would you mind filing an issue for this over on the project repository?
joomla-projects/soc21_website-cronjob

@PhilETaylor

Also I wonder if this feature should even be named "cronjobs" at all as they are NOT cron jobs.... they are just scheduled tasks, that are not even (optionally) run by any cron system at a operating system level.

Cron jobs are tasks added to a crontab on a unix based machine.

Cron is a technical term that probably not all Joomla users are familiar with. Something like "Scheduled Tasks" everyone can understand.

@brianteeman

@\PhilETaylor I have been pondering that myself and browsing the web looking at examples. The best I have seen to date was using "Cron Jobs, "Cron Tasks" as two separate words. But I wasn't happy that it provided the required clarity when I saw that in action in this PR.

Your suggestion, and reasoning, for "Scheduled Tasks" is much better.

Then the collective name could be simply Schedule and the individual name is Task.

Example
A new feature in 4.1 is the ability to Schedule new Tasks. You can add as many tasks as you want to the Schedule and each task can have its own time interval.

Thank you for bringing this up! I agree with that the cron term is perhaps not correct and too technical for effective marketing. My mentor and I are open to the [rebranding] in the future 😄

@ditsuke

@scout507 would you mind filing an issue for this over on the project repository?
joomla-projects/soc21_website-cronjob

Done. joomla-projects/soc21_website-cronjob#11

@\brianteeman

Looking at the language file I can see a lot of values which could be replaced with existing strings from global language files. This will reduce the workload on translators and also ensure a more consistent ui.
For example COM_CRONJOBS_FIELDSET_RULES is the string used to display a fieldset called JCONFIG_PERMISSIONS_LABEL

Thank you for this review! I've identified the strings that have a global equivalent and will be replacing them soon.

This should be resolved with 101a03d, which removes and replaces usages of all redundant strings I could identify.

I'm writing it here, but it counts for all GSoC/SoC PR thank you @brianteeman , @dgrammatiko and @PhilETaylor for the feedback, it's really appreciated!

@brianteeman
Thank you for the review! I've tried to address all issues in the latest series of commits.

Hello everyone! It's now been around 15 days since I opened this PR. I've addressed and continue to work on the feedback from the community. A basic overview for this can be had from the tasks list in the PR body.

With this, my mentor (@bembelimen) and I would now like to announce the completion of the first phase of this project. We're grateful for everyone who gave their feedback and took time to review the code. The feedback has helped a lot in ironing out many of the inconsistencies, bugs and deficiencies and we hope it continues to go that way. A special thanks to @PhilETaylor and @brianteeman who helped me clean up much of my mess and came up with the idea for the rebranding to Scheduled Tasks ! ♥

While there's still much to do in order to meet the project goals, the core goal and the supporting infrastructure has been solidified. Having said that, I stay committed to seeing this project through as a part of Joomla! core and strongly encourage further feedback, testing and ideas for task plugins.

To plugin developers, I encourage you have a look at what we have to offer. For an idea of how task plugins work, you can have a look at the existing demotasks plugin or the other task plugins that have more functionality.

This PR will continue to be regularly updated with the state of the dcon development branch from the project repository. Visitors are welcome and encouraged! 😃

@ditsuke

1. Why postgres schema of #__scheduler_tasks has "id" int GENERATED ALWAYS AS IDENTITY,?

Shouldn't it be "id" serial NOT NULL, with PRIMARY KEY ("id") as a common case for all tables in installation sql?

2. Why mysql schema has `title` varchar(128) NOT NULL UNIQUE` ?

We can safely use `title` varchar(255) NOT NULL DEFAULT '', as in other tables.

3. Missing multiple edit block via checked_out and checked_out_time

4. I am afraid of scheduler system plugin PlgSystemSchedulerunner will still attach events via getSubscribedEvents method even though com_scheduler is not enabled.

5. PlgSystemSchedulerunner actually doesn't use own language strings, what for it auto-loads language?

6. If the component name is com_scheduler, isn't it more informative to have the plugin group scheduler but not task? Even tasks is plural is better.

7. Huge issue: the component will query #__scheduler_tasks on each page load, should be optimized somehow.

8. Added some comments in PR.

@Denitz

Thank you for the suggestions and the in-depth review. I'll work on addressing all of the issues in time.

@ditsuke

1. Why postgres schema of #__scheduler_tasks has "id" int GENERATED ALWAYS AS IDENTITY,?

Shouldn't it be "id" serial NOT NULL, with PRIMARY KEY ("id") as a common case for all tables in installation sql?

1. Why mysql schema has title varchar(128) NOT NULL UNIQUE` ?

We can safely use title varchar(255) NOT NULL DEFAULT '', as in other tables.

Agreed, I'll alter the DDL as suggested!

1. Missing multiple edit block via checked_out and checked_out_time

I'm not sure if we need these in the context of tasks. Opinions?

2. I am afraid of scheduler system plugin PlgSystemSchedulerunner will still attach events via getSubscribedEvents method even though com_scheduler is not enabled.

I'll investigate and address this soon.

3. PlgSystemSchedulerunner actually doesn't use own language strings, what for it auto-loads language?

Good eye! It's a remnant from back when it did a lot more.

4. If the component name is com_scheduler, isn't it more informative to have the plugin group scheduler but not task? Even tasks is plural is better.

I'm note sure, since I observed we're using singulars in all other plugin groups. I'm a bit against 'scheduler' since in theory tasks can be unscheduled (single use and such).

5. Huge issue: the component will query #__scheduler_tasks on each page load, should be optimized somehow.

Good point! I'll talk about this with me mentor so we can think of a possible solution. Do you have any suggestions?

Multiple edit block via checked_out and checked_out_time - this feature is common in Joomla, we 100% should have it for core component!

In any case it's easier to spot that plugins of group scheduler belong to com_scheduler! finder group for com_finder, workflow - com_workflow etc., please don't break the logic.

Limiting the database query to load tasks on each page load - I guess the component should pre-compile possible execute time and date options and store them as a component param (re-compile on each task edit/create/delete/state-changed), hence it will be available via component params without extra load. Next initially we should check the available special patterns using the current date and time if the moment has come etc.
It's the complex problem, requires days... Especially problematic is handling the expired tasks. In any case, it's not the trivial select->foreach->echo coding!

Or just an idea: assuming that the number of tasks is not high, at least maybe we can store the TaskId=>NextExecutionUtcTimestamp in the component params (re-compile as noted above) and just check these timestamps against current time() - sounds good and easier for me.

Besides, I didn't check how the task is actually executed, but I guess it's not queued and executed within the current request - it's weird. Maybe a separate ajax call should be scheduled first for the page load which has triggered the task, next this separate call sets the task flag 'In Progress', hashes are updated to not trigger the race condition (at least to minimize it) and only next the task is actually executed with ignoring user abort on request, setting time limit to 0 etc.

Btw what if the task was expired but didn't run in time? Will it be executed later on the first next page load?

Multiple edit block via checked_out and checked_out_time - this feature is common in Joomla, we 100% should have it for core component!

These have been added to the table now.

In any case it's easier to spot that plugins of group scheduler belong to com_scheduler! finder group for com_finder, workflow - com_workflow etc., please don't break the logic.

We'll likely make this switch :)

Limiting the database query to load tasks on each page load - I guess the component should pre-compile possible execute time and date options and store them as a component param (re-compile on each task edit/create/delete/state-changed), hence it will be available via component params without extra load. Next initially we should check the available special patterns using the current date and time if the moment has come etc.
It's the complex problem, requires days... Especially problematic is handling the expired tasks. In any case, it's not the trivial select->foreach->echo coding!

Or just an idea: assuming that the number of tasks is not high, at least maybe we can store the TaskId=>NextExecutionUtcTimestamp in the component params (re-compile as noted above) and just check these timestamps against current time() - sounds good and easier for me.

So the way we're handling task execution right now is we load only a single entry from the database on each load. This is possible because the next execution is pre-computed each time a task is executed, so then we only have to rely on a simple logic to check if the task is due and then get queue-like behavior by sorting on basis of priority and how far back the execution was supposed to happen. We are open to considering a more performant solution but this might just be good enough.

Besides, I didn't check how the task is actually executed, but I guess it's not queued and executed within the current request - it's weird. Maybe a separate ajax call should be scheduled first for the page load which has triggered the task, next this separate call sets the task flag 'In Progress', hashes are updated to not trigger the race condition (at least to minimize it) and only next the task is actually executed with ignoring user abort on request, setting time limit to 0 etc.

I really like the AJAX idea! Indeed right now we execute it within the current request, but I think once we have the Scheduler API extension ready we can work on an alternative AJAX solution. I'll discuss this with my mentor when we get to this.

Btw what if the task was expired but didn't run in time? Will it be executed later on the first next page load?

That's right, expired or overdue tasks are executed lazily. We use a queue like behavior that takes into account task priority + (planned) execution time if multiple tasks are queued. This is all done within the database query (the Scheduler class for reference).

@nibra

In a quick review, I found a few little things. What I really still miss are automated tests.

Thank you for the review! I also feel we can use tests, much helpful here with how much things have grown. Will try my hands at them when possible. 🙂

@ditsuke Im not sure if you are aware, but for 99% of the suggested changes I and other make, when a suggestion is given as a code diff, you can simply go to the PR list of files here: https://github.com/joomla/joomla-cms/pull/35143/files and click Apply Suggestion next to them, and then do a git pull to get those changes into your local repo.

Its much easier that way to merge the million small changes and not lose them. Just a tip.

Read The Joomla! coding standards document

On 2 Oct 2021, at 07:37, Tushar @.***> wrote:


@ditsuke commented on this pull request.

In administrator/components/com_scheduler/src/Field/CronField.php:

• • @var string[]

• • @SInCE DEPLOY_VERSION
    Do we have this spacing elsewhere for property docblocks?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.

Hello everyone, I'm glad to announce that the Scheduler is now open for testers and that at last this PR has graduated from its Draft status!

In the months since the opening of this PR, we have gone through a lot of code review, community discussions and brainstormed over what we need to get in first and the things that need to be fixed. While we have added a lot of functionality and refined yet more thanks all the generous code review, here's some notable features that are relatively new:

1. Client-side AJAX based lazy runner so we no longer have to give visitors a bad experience,
2. Email notifications (configurable on a per-task basis),
3. Manual "play" button for tasks in the admin backend,
4. CLI commands to trigger and list tasks (we still need creation and deletion!),
5. Webcron functionality, i.e., an endpoint (not quite API) that can be enabled, protected and used to trigger tasks from an external service,
6. Extensive component configuration options - so you get to choose to enable lazy scheduling, or to rely on native cron, and some granular control on the client-side scheduler trigger,
   .. and many other improvements/features.

While there's still some unresolved feedback and much more cool features I wish we'll implement in time, the Scheduler now needs testers so we can find bugs and deficiencies where they matter most. More code review and PRs are welcome so we can get the Scheduler and the Task infrastructure at the best place possible in time for 4.1.

While there's still some unresolved feedback and much more cool features I wish we'll implement in time, the Scheduler now needs testers so we can find bugs and deficiencies where they matter most. More code review and PRs are welcome so we can get the Scheduler and the Task infrastructure at the best place possible in time for 4.1.

Thank you very much @ditsuke for your superb effort to bring it on this state. And thanks for all the helper so far. Now the final spurt starts, ironing out things, make it stable and the target should be to get it merged for the Alpha 3 (November 23th) to have more than 2 month to work with it in core.

@ditsuke I applied the files and in Discover I see an untranslated string for 1 plugin

All the other plugins show the full description.

@ditsuke After installing via Discover I go to Scheduled tasks and see this error:

Table 'joomla4_db.e0f7f_scheduler_tasks' doesn't exist 

I then went to System -> Database and tried to update structure but it does not fix it either. The table is not created. So I tried to create the table myself but I see the following DB error:

Specified key was too long; max key length is 3072 bytes

I am running MySQL 5.7.29

@ditsuke My final error, as I will stop now, I click on Create a new task and I am greeted by:

There is no "com_scheduler.admin-view-select-task-css" asset of a "style" type in the registry. 

I suggest a description or expanded label here. What is the value? second, minute, hour?

@roland-d

@ditsuke After installing via Discover I go to Scheduled tasks and see this error:

Table 'joomla4_db.e0f7f_scheduler_tasks' doesn't exist 

I then went to System -> Database and tried to update structure but it does not fix it either. The table is not created. So I tried to create the table myself but I see the following DB error:

Specified key was too long; max key length is 3072 bytes

I am running MySQL 5.7.29

Thank you for taking the time to debug this issue. I think I have now a good idea what's causing this. Will try a fix soon 🙂

@ditsuke My final error, as I will stop now, I click on Create a new task and I am greeted by:

There is no "com_scheduler.admin-view-select-task-css" asset of a "style" type in the registry. 

This is a media asset that needs to be built first with the build tools. I'm not sure, how does the patchtester (i'm assuming) handle these?

I suggest a description or expanded label here. What is the value? second, minute, hour?

Thanks. I think an expanded label should do. Does this work?

@roland-d Installing with discovery installation would require that the component does not only provide SQL statements in the core scripts for new installation (e.g. base.sql) and update SQL scripts but also a component set of SQL scripts like we have them for historic reasons for some components and which people always forget to maintain. As we have stopped to provide them for our core extensions because there is no need to discover them under normal conditions, I think it is not needed to add them here.

Instead of testing with discovery installation I suggest to test with updating a clean 4.1-dev or latest 4.1 nightly to the patched update package or custom update URL created by drone for this PR.

Hmm, I updated from a 4.0 installation and got some errors

and autoload_psr4.php is not updated on joomla update (unreleated to this pr)
0 Class "Joomla\Component\Scheduler\Administrator\Extension\SchedulerComponent" not found

after recreating the autoloader I get this error message on openening the tasks view
0 count(): Argument #1 ($value) must be of type Countable|array, bool given

the db fixer tells me:
Table 'vi3ow_scheduler_tasks' does not exist. (From file 4.1.0-2021-08-08.sql.)

reason for this is that the table couldn't be created because the index is too long.
type has 1024 characters which is with utf8_mb4 4096 bytes, allowed is 767, I didn't looked at the code yet but don't understand way a unique key have to be 1024 chars long when UUID has only 36 chars.

Creating a key for the webcron didn't worked, at least it didn't created a key when I saved the configuration.

I created a test cron and executed it manually, seems to work but doesn't updated the "last run" information.

@ditsuke After your commit to merge the update SQL scripts, I still see 2 for each database type, "4.1.0-2021-08-04.sql" and "4.1.0-2021-08-08.sql". Please combine these, too, and maybe give them a younger date, i.e.

• Rename "4.1.0-2021-08-08.sql" to "4.1.0-2021-11-20.sql" for each database type with one commit.
• Then copy content of "4.1.0-2021-08-04.sql" into "4.1.0-2021-11-20.sql" and remove the "4.1.0-2021-08-04.sql" for each database type with another commit.

To have separate commits for rename and content change makes GitHub show the history right and not show it as a deleted and another created file.

More SQL fixes see joomla-projects/soc21_website-cronjob#49 .

When updating from a clean 4.1-dev installation to a patched update package which contains the changes from this PR plus my corrections on the SQL scripts from joomla-projects/soc21_website-cronjob#49 , it ends with following:

During the update there are no SQL errors, so I think the SQL is ok for at least MySQL (I haven't tested PostgreSQL yet).

See joomla-projects/soc21_website-cronjob#50 for updating the list of core extensions in the extensions helper so that on update of the core. the manifest cache is updated for the new core extensions.

It does not solve the above issue but is required, too.

@richard67

When updating from a clean 4.1-dev installation to a patched update package which contains the changes from this PR plus my corrections on the SQL scripts from joomla-projects/soc21_website-cronjob#49 , it ends with following:

During the update there are no SQL errors, so I think the SQL is ok for at least MySQL (I haven't tested PostgreSQL yet).

See joomla-projects/soc21_website-cronjob#50 for updating the list of core extensions in the extensions helper so that on update of the core. the manifest cache is updated for the new core extensions.

It does not solve the above issue but is required, too.

Thanks for the fixes on the SQL.

It appears the issue appears because the autoload file (JOOMLA_ROOT/administrator/cache/autoload_psr4.php) is not refreshed. Shouldn't it be deleted by default by the update script, or do we need something else so its done?

Shouldn't it be deleted by default by the update script, or do we need something else so its done?

@ditsuke I have no idea. Maybe @bembelimen knows, or knows who knows?

Shouldn't it be deleted by default by the update script, or do we need something else so its done?

@ditsuke I have no idea. Maybe @bembelimen knows, or knows who knows?

So talking with the maintanier team, this is not an issue of this PR but a general issue in Joomla! (this one was just the unlucky one to discover it). We'll take care of it.

So as we have the issue with the update from 4.0, I'll not merge this PR now but for beta 1, so we have enough time to implement a proper fix (independend from this PR).

made a quick test from CLI

I have tested this item ✅ successfully on d77353d

made a quick test from CLI

@alikon we tried to reproduce the issue, but so far no success, could you share the options you set for the task?

this is my task a GET one

...

changing the state from CLI doesn't work

running the test task from admin

Last run not updated it is expected ?

Thank you @ditsuke , @brianteeman , @PhilETaylor , @richard67 , @HLeithner , @Denitz , @alikon , @dgrammatiko , @nibra , @chmst , @softforge (and many more) who made this possible and happen.

from when a pr it's merged without 2 test
...really surprised..