PR-5.2-dev Pending

User tests: Successful: Unsuccessful:

avatar zero-24
zero-24
20 Jul 2024

Pull Request for Issue #43490

Summary of Changes

Implement "none-ID" usage for the CRON Sheduler

Testing Instructions

  • Setup 5.2-dev
  • System -> Scheduled Tasks -> Options -> Disable Lazy Scheduler
  • System -> Scheduled Tasks -> Options -> Enable CRON Scheduler
  • Trigger the CRON Scheduler for example via curl or other external tool
  • Notice that nothing will be triggered

Actual result BEFORE applying this Pull Request

Before the patch the triggered method requires an $id to be passed.

Expected result AFTER applying this Pull Request

With that patch we allow the usage id as well as "none-ID". With that none-ID thing we trigger one planed task per run.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

Note to the maintainer

Please double check the #43490 before merging as well as check the upmerge of this PR to 6.0-dev Thanks!

avatar zero-24 zero-24 - open - 20 Jul 2024
avatar zero-24 zero-24 - change - 20 Jul 2024
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 20 Jul 2024
Category Administration Language & Strings Front End Plugins
avatar brbrbr
brbrbr - comment - 20 Jul 2024

The deprecated message as the same issue, it's triggered on every request if the home page menu has an ID.

PR #43164 seems to fix id's from the menu being injected. And with that the original problem. (and then the deprecated message shows when it should) .

Still a good idea to have a dedicated parameter.

avatar zero-24
zero-24 - comment - 20 Jul 2024

The deprecated message as the same issue, it's triggered on every request if the home page menu has an ID.

Hmm yes will remove the call for now. Or do you have a better alternative here?

avatar zero-24 zero-24 - change - 20 Jul 2024
Labels Added: Language Change PR-5.2-dev
avatar brbrbr
brbrbr - comment - 20 Jul 2024

not really, that would be a solution for the original problem.

Add it after #43164

In runTestCron also 'id' is used. Wouldn't it make sense to change that as well to taskid. Just to have consistent code?

avatar Hackwar
Hackwar - comment - 20 Jul 2024

I understand what you are trying to do, but I think your issue is solved with #43164. Can you check that?

avatar zero-24
zero-24 - comment - 21 Jul 2024

Works good to me thanks @Hackwar still i would recommend to apply this PR + move from id= to taskid to be sure which ID is passed over. So this here is the B/C safe way and the 6.0 patch will clean up the old Ids.

avatar Hackwar
Hackwar - comment - 21 Jul 2024

To be honest, I'm hesitant to merge this if it isn't necessary anymore due to #43164. It feels like change for the sake of change then. I will bring this up in the CMS maintenance team.

avatar zero-24
zero-24 - comment - 21 Jul 2024

Adding back the deprecated message as that issue will be fixed by #43164

To be honest, I'm hesitant to merge this if it isn't necessary anymore due to #43164. It feels like change for the sake of change then. I will bring this up in the CMS maintenance team.

Please let me know the result. What we need is the seccond change with the queue what we can revert is the id => taskid change.

avatar Fedik
Fedik - comment - 21 Jul 2024

#43164 does not fix the root issue of current PR,
When home page is set to Artcile detail (or another component with ID in request), we still will have id in the input, and this id will conflicting with Scheduler task id.
Because Scheduler plugin blindly checking for any id in the request.

avatar Fedik
Fedik - comment - 21 Jul 2024

Hm, ignore my previous coment,
I have misunderstood a few things ?

avatar joomla-cms-bot joomla-cms-bot - change - 25 Jul 2024
Category Administration Language & Strings Front End Plugins Front End Plugins
avatar zero-24 zero-24 - change - 26 Jul 2024
Labels Removed: Language Change
avatar Hackwar
Hackwar - comment - 27 Jul 2024

Can you fix the codestyle?

avatar zero-24
zero-24 - comment - 27 Jul 2024

Fixed @Hackwar

avatar richard67
richard67 - comment - 11 Aug 2024

@zero-24 Are the testing instructions still right? They refer to 6.0-dev.

avatar zero-24 zero-24 - change - 11 Aug 2024
Title
[5.2] Add the new taskid parameter as alternative to the broken 'id' parameter
[5.2] Add the "non-ID" behavior to the CRON Sheduler
avatar zero-24 zero-24 - edited - 11 Aug 2024
avatar zero-24 zero-24 - change - 11 Aug 2024
The description was changed
avatar zero-24 zero-24 - edited - 11 Aug 2024
avatar zero-24 zero-24 - change - 11 Aug 2024
Title
[5.2] Add the "non-ID" behavior to the CRON Sheduler
[5.2] Add the "non-ID" behavior to the CRON Scheduler
avatar zero-24 zero-24 - edited - 11 Aug 2024
avatar zero-24 zero-24 - change - 11 Aug 2024
The description was changed
avatar zero-24 zero-24 - edited - 11 Aug 2024
avatar zero-24 zero-24 - change - 11 Aug 2024
The description was changed
avatar zero-24 zero-24 - edited - 11 Aug 2024
avatar zero-24
zero-24 - comment - 11 Aug 2024

@richard67 I have just updated the texts, during the discussion here the code has been changed so the instructions needed to be updated too.

avatar richard67
richard67 - comment - 11 Aug 2024

@richard67 I have just updated the texts, during the discussion here the code has been changed so the instructions needed to be updated too.

@zero-24 Thanks. Description and instructions look good to me now.

avatar zero-24 zero-24 - change - 11 Aug 2024
Title
[5.2] Add the "non-ID" behavior to the CRON Scheduler
[5.2] Add the "none-ID" behavior to the CRON Scheduler
avatar zero-24 zero-24 - edited - 11 Aug 2024
avatar J-Wick4
J-Wick4 - comment - 12 Aug 2024

When trying the changes and directly accessing the URL in the browser, this error message is produced, and no tasks are executed.


Warning: Attempt to read property "id" on null in /var/www/vhosts/domain.net/subdomains/beta5/plugins/system/schedulerunner/src/Extension/ScheduleRunner.php on line 228
{"success":false,"message":"Joomla\\Plugin\\System\\ScheduleRunner\\Extension\\ScheduleRunner::runScheduler(): Argument #1 ($id) must be of type int, null given, called in \/var\/www\/vhosts\/domain.net\/subdomains\/beta5\/plugins\/system\/schedulerunner\/src\/Extension\/ScheduleRunner.php on line 228","messages":null,"data":null}
avatar richard67
richard67 - comment - 12 Aug 2024

When trying the changes and directly accessing the URL in the browser, this error message is produced, and no tasks are executed.

Warning: Attempt to read property "id" on null in /var/www/vhosts/domain.net/subdomains/beta5/plugins/system/schedulerunner/src/Extension/ScheduleRunner.php on line 228
{"success":false,"message":"Joomla\Plugin\System\ScheduleRunner\Extension\ScheduleRunner::runScheduler(): Argument #1 ($id) must be of type int, null given, called in /var/www/vhosts/domain.net/subdomains/beta5/plugins/system/schedulerunner/src/Extension/ScheduleRunner.php on line 228","messages":null,"data":null}

@J-Wick4 Have you tested on a current 5.2 nightly build? This PR here is for 5.2.

avatar J-Wick4
J-Wick4 - comment - 13 Aug 2024

When trying the changes and directly accessing the URL in the browser, this error message is produced, and no tasks are executed.
Warning: Attempt to read property "id" on null in /var/www/vhosts/domain.net/subdomains/beta5/plugins/system/schedulerunner/src/Extension/ScheduleRunner.php on line 228
{"success":false,"message":"Joomla\Plugin\System\ScheduleRunner\Extension\ScheduleRunner::runScheduler(): Argument #1 ($id) must be of type int, null given, called in /var/www/vhosts/domain.net/subdomains/beta5/plugins/system/schedulerunner/src/Extension/ScheduleRunner.php on line 228","messages":null,"data":null}

@J-Wick4 Have you tested on a current 5.2 nightly build? This PR here is for 5.2.

No sorry, only on the most current recent version of 5.1.2.

avatar Quy
Quy - comment - 14 Aug 2024

I can confirm the error when entering a non-existing id in the base URL.

avatar zero-24
zero-24 - comment - 14 Aug 2024

hmm will take a look into that. Its kind of an correct error when the ID does not exist, but as there is still the issue with the article id out in the wild. But that should be solved together with the release of this change anyway.

avatar Hackwar
Hackwar - comment - 20 Aug 2024

The article id issue is solved by now.

avatar zero-24
zero-24 - comment - 20 Aug 2024

Yes the main question is should we implement a check whether the passed ID is an valid task ID or should we issue an error like mentiond above. While the "article id issue" is solved the same problem comes up with an wrong task id too.

avatar coolcat-creations
coolcat-creations - comment - 27 Aug 2024

I think if the id is not existing or disabled or trashed it should return an error?

avatar zero-24
zero-24 - comment - 27 Aug 2024

Yes in that case the error is correct and this PR is ready and the test was ok :)

avatar HLeithner
HLeithner - comment - 2 Sep 2024

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

avatar HLeithner HLeithner - change - 2 Sep 2024
Title
[5.2] Add the "none-ID" behavior to the CRON Scheduler
[5.3] Add the "none-ID" behavior to the CRON Scheduler
avatar HLeithner HLeithner - edited - 2 Sep 2024
avatar zero-24
zero-24 - comment - 2 Sep 2024

Wasnt the plan to merge this into 5.2 still? @Hackwar ?

avatar Hackwar
Hackwar - comment - 2 Sep 2024

Yes, this was the plan. But for that to happen, it would have required to have been tested. Unfortunately that didn't happen in time. With beta 1 we had a feature freeze and I already handled that VERY flexible. But tomorrow we have beta 2 and that is a pretty hard line for me. Unfortunately it has to go into 5.3 now.

avatar coolcat-creations
coolcat-creations - comment - 2 Sep 2024

isnt this a bugfix anyway?

avatar zero-24
zero-24 - comment - 22 Sep 2024

Yes its a bug fix as since 4.x this feature (call without ID) is mentiond within the backend but it never worked. So why cant this go into 5.2.1 etc?

Given that ther are good tests up in this thread i dont see whats missing here, the only test reported an error was because of an invalid ID which kind of correctly issues an error, I can add code to check for an valid ID first when you want.

avatar richard67
richard67 - comment - 22 Sep 2024

Given that ther are good tests up in this thread

@zero-24 I don't find any human test results for this PR. Maybe you meant the other, meanwhile closed PR for 6.0-dev?

Add a Comment

Login with GitHub to post a comment