Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixed scheduler time issue #1047

Closed
wants to merge 2 commits into from
Closed

Fixed scheduler time issue #1047

wants to merge 2 commits into from

Conversation

girishpanchal30
Copy link
Contributor

Summary

Improve scheduler util class

Check before Pull Request is ready:

Closes #827

@girishpanchal30 girishpanchal30 added the pr-checklist-skip Allow this Pull Request to skip checklist. label Dec 27, 2024
@pirate-bot pirate-bot added the pr-checklist-complete The Pull Request checklist is complete. (automatic label) label Dec 27, 2024
@pirate-bot
Copy link
Contributor

pirate-bot commented Dec 27, 2024

Plugin build for a4db08f is ready 🛎️!

@vytisbulkevicius
Copy link
Contributor

@girishpanchal30 @HardeepAsrani,

I installed These PRs:
Pro: https://github.com/Codeinwp/feedzy-rss-feeds-pro/pull/809
Lite: #1043

And manually aplied this fix from current PR.

The behavior changed that now I don't see import job next run time at all:
image

And running feedzy_cron Event:
image

Doesn't trigger the import jobs:
image

Does it work for you?

@HardeepAsrani
Copy link
Member

@vytisbulkevicius Can you check now?

@selul
Copy link
Contributor

selul commented Dec 29, 2024

@girishpanchal30 @vytisbulkevicius @HardeepAsrani this has already been fixed by me here 21e3b69, which is on dev.

Also this was not fixing the inconsistency with is_scheduled function @HardeepAsrani, the original method you developed is returning either true false or a timestamp based on the environment, it should return only false or a timestamp.

@girishpanchal30
Copy link
Contributor Author

@selul @HardeepAsrani

it should return only false or a timestamp.

The get_next method returns a timestamp if a scheduled job is found; otherwise, it returns false. Refer to the diff below for details.

 	public static function is_scheduled( string $hook, array $args = array() ) {
-		if ( function_exists( 'as_has_scheduled_action' ) ) {
-			return as_has_scheduled_action( $hook, $args );
-		}
-
-		if ( function_exists( 'as_next_scheduled_action' ) ) {
-			// For older versions of AS.
-			return as_next_scheduled_action( $hook, $args );
-		}
-
-		return wp_next_scheduled( $hook, $args );
+		return self::get_next( $hook, $args );
 	}

@selul
Copy link
Contributor

selul commented Dec 30, 2024

yes, thats exactly what is_scheduled is doing atm in my fix.

@girishpanchal30
Copy link
Contributor Author

@selul

I've checked with dev and it seems the incorrect time is being displayed.

image

With this PR, the correct time is displayed.

image

Should I update the is_scheduled method to use the get_next method?

@selul
Copy link
Contributor

selul commented Dec 30, 2024

We dont need any next method, the is scheduled is enough.

@girishpanchal30
Copy link
Contributor Author

@selul Okay, In this case, we don’t need to merge this PR. Please close this PR when you can.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
pr-checklist-complete The Pull Request checklist is complete. (automatic label) pr-checklist-skip Allow this Pull Request to skip checklist.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants