Skip to content
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

Merged
merged 19 commits into from
Jan 7, 2025

Conversation

zero-24
Copy link
Contributor

@zero-24 zero-24 commented Jul 20, 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!

@brbrbr
Copy link

brbrbr commented Jul 20, 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.

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 20, 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?

@brbrbr
Copy link

brbrbr commented Jul 20, 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?

@Hackwar
Copy link
Member

Hackwar commented Jul 20, 2024

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

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 21, 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.

@Hackwar
Copy link
Member

Hackwar commented Jul 21, 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.

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 21, 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.

@Fedik

This comment was marked as outdated.

@Fedik
Copy link
Member

Fedik commented Jul 21, 2024

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

@joomla-cms-bot joomla-cms-bot removed the Language Change This is for Translators label Jul 25, 2024
@Hackwar
Copy link
Member

Hackwar commented Jul 27, 2024

Can you fix the codestyle?

@Hackwar Hackwar removed the PR-5.2-dev label Sep 3, 2024
@zero-24
Copy link
Contributor Author

zero-24 commented Sep 22, 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.

@richard67
Copy link
Member

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?

@zero-24
Copy link
Contributor Author

zero-24 commented Dec 17, 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?

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 :)

@alikon
Copy link
Contributor

alikon commented Dec 17, 2024

without the pr not passing the id the executed task is the same as if we run from cli
without the pr passing the id the executed task is the corresponding task id

so what this pr is fixing ?

@zero-24
Copy link
Contributor Author

zero-24 commented Dec 17, 2024

without the pr not passing the id the executed task is the same as if we run from cli

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.

@zero-24 zero-24 changed the title [5.3] Add the "none-ID" behavior to the CRON Scheduler [5.3] Improve inline comment for the "none-ID" behavior within the CRON Scheduler Dec 17, 2024
@zero-24 zero-24 removed the Feature label Dec 17, 2024
@zero-24 zero-24 changed the base branch from 5.3-dev to 5.2-dev December 17, 2024 11:55
@zero-24 zero-24 changed the base branch from 5.2-dev to 5.3-dev December 17, 2024 11:56
@alikon
Copy link
Contributor

alikon commented Dec 17, 2024

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.

@alikon
Copy link
Contributor

alikon commented Dec 19, 2024

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.

@alikon
Copy link
Contributor

alikon commented Dec 19, 2024

setting RTC cause successfully tested by @Quy too

@alikon
Copy link
Contributor

alikon commented Dec 19, 2024

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43817.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 19, 2024
@rdeutz rdeutz merged commit 3dcafb5 into joomla:5.3-dev Jan 7, 2025
4 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 7, 2025
@rdeutz
Copy link
Contributor

rdeutz commented Jan 7, 2025

Thanks

@zero-24 zero-24 deleted the webcron52 branch January 7, 2025 08:56
@rdeutz rdeutz added this to the Joomla! 5.3.0 milestone Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.