? ? Pending

User tests: Successful: Unsuccessful:

avatar vorayash
vorayash
22 Mar 2022

Pull Request for Issue Schedules Tasks #37313

Summary of Changes

Changed "joomla-cms\administrator\components\com_scheduler\forms\task.xml" by making cron fields required so that if user tries to save task without checking all the field he/she will get warning.

Testing Instructions

  1. Create Demo Task - Sleep
  2. Select Cron Expression Advanced as Execution Rule
  3. Do not select all values in area Crone match.

Actual result BEFORE applying this Pull Request

image

Expected result AFTER applying this Pull Request

image

avatar vorayash vorayash - open - 22 Mar 2022
avatar vorayash vorayash - change - 22 Mar 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 22 Mar 2022
Category Administration
avatar richard67
richard67 - comment - 22 Mar 2022

What happens if you chose another method than cron expression in the drop down? The cron expression fields should not be required in this case.

avatar vorayash vorayash - change - 23 Mar 2022
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 23 Mar 2022
Category Administration Administration Libraries
avatar joomla-cms-bot joomla-cms-bot - change - 23 Mar 2022
Category Administration Libraries Libraries
avatar vorayash
vorayash - comment - 23 Mar 2022

I have removed required from cron expression and have modified FormField.php. Please check.

avatar richard67
richard67 - comment - 23 Mar 2022

@vorayash Your PR has code style errors. You can see that when going to the bottom of the PR and see the drone CI is failing. With the "Details" link you can get to the details.

2022-03-23_pr-37350_drone_1

When following to that link you come to a page where on the left hand side you can see the failed PHPCS test. By selecting that test, the right hand side shows the log from the test, which tells the details about the code style errors:

2022-03-23_pr-37350_drone_2

Could you fix them? If you need help let me know, then I will propose a code change which you can accept with one mouse click. But I wanted to give you the chance first to fix them yourself.

Thanks in advance.

avatar richard67
richard67 - comment - 23 Mar 2022

P.S.: You should adjust your text editor or IDE editor so that it shows you white space like spaces and tabulators. This helps to avoid the white space at the end of line errors.

avatar vorayash
vorayash - comment - 23 Mar 2022

I will surely do it. Thank you sir for your support.

avatar vorayash
vorayash - comment - 23 Mar 2022

Please check this.

avatar richard67
richard67 - comment - 24 Mar 2022

I think the testing instructions are not complete. It does not only need to test if the PR does what is required for the task form where it has a field set "execution_rules", it also needs to be tested that nothing is broken in forms which use the FormField class or a class derived from it and do NOT have that field set.

I think in this case we will get PHP errors related to unset array elements, e.g. $execution_rules['rule-type'] or $execution_rules['cron-expression'] and so on.

I haven't tested this PR yet, but I am pretty sure it will raise these PHP errors when doing the additional test which I proposed above.

avatar richard67
richard67 - comment - 24 Mar 2022

Another question is if it is a good idea to place the checks for this fieldset which only exists in that particular form in a class which is base of many other types of fields used in many other types of forms, so the check is performed in 99.99 % of all cases for nothing.

Maybe it should be placed in the component's model instead, or in the table class for the task table (if we have such, I haven't checked)?

avatar richard67
richard67 - comment - 24 Mar 2022

I have tested this item ? unsuccessfully on 6b30770

Saving other forms in backend causes PHP Notice "undefined index". The error is just not visible in backend, but when you check your PHP error log file you will see lots of the following entries (one for each field) when saving any form in the backend, e.g. Global Configuration or Article Edit:

PHP Notice: Undefined index: rule-type in /home/richard/lamp/public_html/joomla-cms-4.1-dev/libraries/src/Form/FormField.php on line 1294, referer: https://www.joomla-41-dev.vmubu01.vmnet2.local/administrator/index.php?option=com_config

Another question is if it is really clever to check at validation of (almost) every field for the cron related specials.

These checks belong into either the model of the corresponding component https://github.com/joomla/joomla-cms/blob/4.1-dev/administrator/components/com_scheduler/src/Model/TaskModel.php or in the table class of the task table https://github.com/joomla/joomla-cms/blob/4.1-dev/administrator/components/com_scheduler/src/Table/TaskTable.php .


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37350.
avatar richard67 richard67 - test_item - 24 Mar 2022 - Tested unsuccessfully
avatar laoneo
laoneo - comment - 25 Mar 2022

Catching the rule error must be added to the scheduler component and not the FormField class as it should not be aware about any context. Correctly the controller must pass the valid data to the model as the model and the table should not depend on the input either (I know the model has populatestate where we are suffering since ages from it).

avatar vorayash
vorayash - comment - 25 Mar 2022

Thank you for your guidance. I will solve this issues.

avatar ditsuke
ditsuke - comment - 11 Apr 2022

@vorayash are you still working on this?

avatar vorayash
vorayash - comment - 11 Apr 2022

Sir I am right now busy with my GSOC task. Is it possible that after completing it I work on this?

avatar bembelimen
bembelimen - comment - 11 Apr 2022

I'm closing this as this is, as stated above, the wrong approach.

If you are available again, please feel free to create a new PR.

avatar bembelimen bembelimen - close - 11 Apr 2022
avatar bembelimen bembelimen - change - 11 Apr 2022
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2022-04-11 14:57:45
Closed_By bembelimen
Labels Added: ?
avatar vorayash
vorayash - comment - 11 Apr 2022

Ok sir.

Add a Comment

Login with GitHub to post a comment