User tests: Successful: Unsuccessful:
Pull Request for Issue Schedules Tasks #37313
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.
Status | New | ⇒ | Pending |
Category | ⇒ | Administration |
Labels |
Added:
?
|
Category | Administration | ⇒ | Administration Libraries |
Category | Administration Libraries | ⇒ | Libraries |
I have removed required from cron expression and have modified FormField.php. Please check.
@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.
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:
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.
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.
I will surely do it. Thank you sir for your support.
Please check this.
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.
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)?
I have tested this item
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 .
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).
Thank you for your guidance. I will solve this issues.
Sir I am right now busy with my GSOC task. Is it possible that after completing it I work on this?
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.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-04-11 14:57:45 |
Closed_By | ⇒ | bembelimen | |
Labels |
Added:
?
|
Ok sir.
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.