Skip to content

Remove WP CronInterval rule #831

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Remove WP CronInterval rule #831

wants to merge 1 commit into from

Conversation

davidsword
Copy link

@davidsword davidsword commented Mar 25, 2025 •
edited
Loading

VIP's cron setup

https://wpvip.com/blog/wordpress-cron/
https://docs.wpvip.com/wordpress-on-vip/cron-control/

can handle frequent heavy cron schedules. On VIP increased job frequency does not equate to slower site performance as the jobs are run async on dedicated batch containers, not front end requests.

@davidsword davidsword requested a review from a team as a code owner March 25, 2025 22:21
@jrfnl
Copy link
Collaborator

jrfnl commented Mar 26, 2025

@davidsword Without giving an opinion on this PR, could you please rebase this PR on the current develop branch ? Otherwise you will never get a passing build.

@GaryJones
Copy link
Contributor

Rather than remove the rule, could we make it 60s instead of 900s?

@GaryJones GaryJones changed the base branch from main to develop March 27, 2025 05:33
@GaryJones GaryJones force-pushed the remove/wp-croninterval-rule branch from 10afe5c to ff6d85b Compare March 27, 2025 05:45
@GaryJones
VIP-Go: change cron interval min value interval
231ea65 
VIP's cron setup

https://wpvip.com/blog/wordpress-cron/
https://docs.wpvip.com/wordpress-on-vip/cron-control/

Can handle frequent heavy cron schedules. Increase job frequency does not equate to slower site performance as the jobs are run async on dedicated batch containers, not front end requests.
@GaryJones GaryJones force-pushed the remove/wp-croninterval-rule branch from ff6d85b to 231ea65 Compare March 27, 2025 05:48
@GaryJones
Copy link
Contributor

Pulled against develop instead of main, rebased onto the current develop, added ruleset test for VIP-Go-Minimum.

@GaryJones GaryJones requested a review from jrfnl March 27, 2025 05:52
jrfnl
jrfnl reviewed Mar 27, 2025
View reviewed changes
Copy link
Collaborator

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good ✔️

The only thing I'm wondering about is VIP-Go vs VIPMinimum.
I've just checked and the VIPMinimum ruleset includes the sniff as well and explicitly sets it to 15 minutes.

VIP-Coding-Standards/WordPressVIPMinimum/ruleset.xml

Lines 46 to 51 in ac9c5fc

<rule ref="WordPress.WP.CronInterval">
<properties>
<!-- Set to 900 (15 minutes), which is the requirement for the VIP platform. -->
<property name="min_interval" value="900"/>
</properties>
</rule>

Is that still correct ?

@GaryJones
Copy link
Contributor

Yes - we've not made a start on consolidating these, so while it's not quite aligned with the ultimate end goal, it is aligned with other rules that currently exist in the initial standard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrfnl jrfnl jrfnl left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@davidsword @jrfnl @GaryJones