joomla-cms icon indicating copy to clipboard operation
joomla-cms copied to clipboard

[5.2] Remove ExecRuleHelper

Open ditsuke opened this issue 3 years ago • 13 comments

Summary of Changes

Replace the ExecRuleHelper class with Task::computeNextExecution().

Testing Instructions

  • Creating new tasks and modifying existing tasks works as expected.
  • Triggered tasks computer next execution date as expected from configuration.

Actual result BEFORE applying this Pull Request

ExecRuleHelper exists, for little reason.

Expected result AFTER applying this Pull Request

ExecRuleHelper is safely replaced by Task::computeNextExecution()

Documentation Changes Required

-

ditsuke avatar Dec 27 '21 15:12 ditsuke

I applied this patch and tried to change settings of a task: settings-task

When saving this settings: error-task

PHP 8.0, win11 and xampp

chmst avatar Dec 31 '21 11:12 chmst

The removed helper PHP will have to be added to the list of files to be deleted on update in script.php. I will do that when this PR has been merged.

richard67 avatar Dec 31 '21 12:12 richard67

@ditsuke I think @chmst indeed found an issue with your changed code, possibly here: https://github.com/joomla/joomla-cms/pull/36439/files#diff-b9f75ae33fad36d7a8968d12a2b78ec972c5e6d2c6fd7ed0024d764191ec48ecR572 .

Possibly a type cast to object is not sufficient. I don't have looked up now what the constructor expects.

richard67 avatar Dec 31 '21 12:12 richard67

@chmst @richard67 thanks for identifying the issue and the source! The latest commit should solve it. Please test.

ditsuke avatar Jan 01 '22 06:01 ditsuke

You fixed the code error, but I don't understand why the Task Params are twice. First in tab "New Task" and in tab "Advanced"?

chmst avatar Jan 01 '22 09:01 chmst

You fixed the code error, but I don't understand why the Task Params are twice. First in tab "New Task" and in tab "Advanced"?

@chmst I addressed that in https://github.com/joomla/joomla-cms/pull/36515 (needs test). We need the routine parameter fieldset to have a showFront="true" attribute at the moment. Although I don't think that's ideal, this PR should work for now. Let's try to work on removing this requirement in the future!

ditsuke avatar Jan 01 '22 10:01 ditsuke

Could we please get a couple tests on this 😃

ditsuke avatar Jan 15 '22 19:01 ditsuke

This pull request has automatically rebased to 4.2-dev.

HLeithner avatar Jun 27 '22 13:06 HLeithner

@ditsuke this PR is still open. could you please resolve the conflicts so that we can re-test?

chmst avatar Oct 22 '22 10:10 chmst

This pull request has been automatically rebased to 5.0-dev.

HLeithner avatar May 02 '23 16:05 HLeithner

This pull request has been automatically rebased to 5.1-dev.

HLeithner avatar Sep 30 '23 22:09 HLeithner

This pull request has been automatically rebased to 5.2-dev.

HLeithner avatar Apr 24 '24 09:04 HLeithner