-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[5.3] Improve inline comment for the "none-ID" behavior within the CRON Scheduler #43817
Conversation
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. |
Hmm yes will remove the call for now. Or do you have a better alternative here? |
not really, that would be a solution for the original problem. Add it after #43164
|
I understand what you are trying to do, but I think your issue is solved with #43164. Can you check that? |
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. |
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. |
Adding back the deprecated message as that issue will be fixed by #43164
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. |
This comment was marked as outdated.
This comment was marked as outdated.
Hm, ignore my previous coment, |
Co-authored-by: Richard Fath <[email protected]>
Can you fix the codestyle? |
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. |
@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? |
Yes looks like i got that mixed up. Still this is the same code so a test against this PR should be simple so this logterm bug could actually be fixed soon :) |
without the pr not passing the id the executed task is the same as if we run from cli so what this pr is fixing ? |
Hmm thanks, interesting my tests back than when i created the PR where acutally different now the tests show the same behavior at to you, will adjust the PR to only improve the inline comments to better reflect the code executed. |
I have tested this item ✅ successfully on 0c05584 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43817. |
I have tested this item ✅ successfully on 1ae95bf This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43817. |
setting RTC cause successfully tested by @Quy too |
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43817. |
Thanks |
Pull Request for Issue #43490
Summary of Changes
Implement "none-ID" usage for the CRON Sheduler
Testing Instructions
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!