Bug/12102 timezone fix by zutigrm · Pull Request #12303 · google/site-kit-wp

zutigrm · 2026-03-11T15:31:34Z

Summary

Addresses issue:

Email Report Scheduler time does not account for timezone #12102

Relevant technical choices

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 5.2 and PHP 7.4.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.
Ensure there are no unexpected significant changes to file sizes.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

github-actions · 2026-03-11T15:36:46Z

Build files for a889481 are ready:

Download development build
Download production build

benbowler

A few inline comments.

After doing this I noticed that the plugin is still showing the difference in timings in the tester plugin:

I found a few bugs in date/timezone shifting in the tester plugin, so I've fixed it, as well as updated it to pass the timestamp, and will share it in Slack for testing.

benbowler · 2026-03-16T11:25:16Z

tests/phpunit/integration/Core/Email_Reporting/Initiator_TaskTest.php

 			self::factory()->user->create(),
 		);

+		$scheduled_timestamp = 1_700_010_000;


It's worth adding a comment for readability here or using strtotime to make this readable.

benbowler · 2026-03-16T11:25:31Z

tests/phpunit/integration/Core/Email_Reporting/Initiator_TaskTest.php

 	}

 	public function test_handle_callback_action_without_subscribers_still_schedules_follow_up_events() {
+		$scheduled_timestamp = 1_700_020_000;


benbowler · 2026-03-16T11:27:46Z

includes/Core/Email_Reporting/Email_Reporting_Scheduler.php

 			return;
 		}

 		$next = $this->frequency_planner->next_occurrence( $frequency, time(), wp_timezone() );


This code is adding wp_timezone calls, which are not compatible with WP 5.2. Can this be updated here to us BC_Functions so that it doesn't need to be captured in #12311

That's a good point, it will be faster to include it here

benbowler · 2026-03-16T11:27:54Z

includes/Core/Email_Reporting/Email_Reporting_Scheduler.php

@@ -93,7 +93,59 @@ public function schedule_initiator_once( $frequency ) {
 	public function schedule_next_initiator( $frequency, $timestamp ) {
 		$next = $this->frequency_planner->next_occurrence( $frequency, $timestamp, wp_timezone() );


tests/phpunit/integration/Core/Email_Reporting/Email_ReportingTest.php

…-fix.

benbowler

✅ CR

…-fix.

github-actions · 2026-03-17T11:41:55Z

🤖 This comment is automatically updated by CI workflows. Each section is managed independently.

📦 Build files for 50da565:

Download development build
Download production build

🎭 Playwright reports for 50da565:

benbowler

Thanks @zutigrm, I addressed two missed uses within tests for consistency, even though tests are likely to run above PHP 5.2.

✅ CR

tofumatt

This looks good, but I have concerns about the private API usage 😅

tofumatt · 2026-03-17T20:50:25Z

includes/Core/Email_Reporting/Email_Reporting_Scheduler.php

+	 * @return int|false Timestamp if found, otherwise false.
+	 */
+	public function get_initiator_timestamp( $frequency ) {
+		$cron = _get_cron_array();


🤔 This is a private function (clearly marked as such with the leading underscore) and is explicitly marked as not being for third-parties to use/rely on: https://developer.wordpress.org/reference/functions/_get_cron_array/

Using private functions like this could cause Site Kit to break unexpectedly in the future—I know we test nightly versions of WordPress right now to check on this sort of thing, but is there any way we can get this data without relying on a private API?

I'd really rather us not use APIs like this, and it wasn't mentioned in the IB.

If we do need to use this, at the absolute minimum, we should thoroughly document our reasoning for using this private API (why public APIs can't provide the data we need).

I can see we have tests for this function so that's good 😄.

We are using it here because of dynamic timestamp argument, so default wp_next_scheduled check won;t do as we don't have static arguments for hook to be correctly identified. The reasoning is explained in detail already in the method header doc:

/**
* Gets the timestamp of the next initiator event for a frequency.
*
* We intentionally scan cron entries instead of using wp_next_scheduled()
* because initiators are scheduled with dynamic args:
* [ $frequency, $scheduled_timestamp ]. wp_next_scheduled() requires an
* exact args match, but here we only need to know whether any initiator
* exists for a given frequency regardless of its scheduled timestamp.
*
* @SInCE n.e.x.t
*
* @param string $frequency Frequency slug.
* @return int|false Timestamp if found, otherwise false.
*/

Similar reason we also use this internal check on uninstallation hook:

site-kit-wp/includes/Core/Util/Uninstallation.php

Lines 160 to 173 in 424e7e4

private function is_event_scheduled( $hook ) {

$crons = _get_cron_array();

if ( ! is_array( $crons ) || empty( $crons ) ) {

return false;

}

foreach ( $crons as $events ) {

if ( isset( $events[ $hook ] ) ) {

return true;

}

}

return false;

tofumatt

Cool, makes sense, thanks! 👍🏻

zutigrm added 9 commits March 11, 2026 16:22

Pass scheduled timestamp to initiator hook.

f62b80c

Schedule initiator with timestamp arg.

0321542

Use scheduled timestamp as batch anchor.

56c00a9

Check initiator existence by frequency.

474bab0

Update initiator assertions for new args.

7e4a157

Assert initiator args include scheduled timestamp.

3352908

Add timezone midnight scheduling test.

bcec24f

Assert worker/fallback use scheduled timestamp.

982fed1

Mock frequency-based initiator checks.

c071d39

benbowler requested changes Mar 16, 2026

View reviewed changes

zutigrm added 7 commits March 16, 2026 16:24

Merge remote-tracking branch 'origin/develop' into bug/12102-timezone…

377df77

…-fix.

Add BC wp_timezone fallback utility.

2143a0a

Use BC timezone.

2c47c48

Use scheduler helper for initiator timestamp assertions.

7556872

Make initiator test timestamp human-readable.

bf5faf9

Fix date range mismatch - extra day excluded.

eb7eca5

Update tests to verify correct date ranges.

a889481

benbowler approved these changes Mar 17, 2026

View reviewed changes

Merge remote-tracking branch 'origin/develop' into bug/12102-timezone…

2eeba57

…-fix.

zutigrm and others added 4 commits March 17, 2026 13:06

Add wp_timezone_string method.

4da1fce

Add tests.

b4dcf6a

Consistently apply strtotime over hard coded unix timestamp.

130fa18

Fix uses of wp_timezone within Frequency_PlannerTest.

cd6c4bc

benbowler approved these changes Mar 17, 2026

View reviewed changes

tofumatt requested changes Mar 17, 2026

View reviewed changes

Add comment to private function usage.

50da565

tofumatt approved these changes Mar 18, 2026

View reviewed changes

tofumatt merged commit 3821979 into develop Mar 18, 2026
18 of 20 checks passed

tofumatt deleted the bug/12102-timezone-fix branch March 18, 2026 18:34

		@@ -93,7 +93,59 @@ public function schedule_initiator_once( $frequency ) {
		public function schedule_next_initiator( $frequency, $timestamp ) {
		$next = $this->frequency_planner->next_occurrence( $frequency, $timestamp, wp_timezone() );

	private function is_event_scheduled( $hook ) {
	$crons = _get_cron_array();

	if ( ! is_array( $crons ) \|\| empty( $crons ) ) {
	return false;
	}

	foreach ( $crons as $events ) {
	if ( isset( $events[ $hook ] ) ) {
	return true;
	}
	}

	return false;

Conversation

zutigrm commented Mar 11, 2026

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

Uh oh!

github-actions bot commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benbowler left a comment

Choose a reason for hiding this comment

Uh oh!

benbowler Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

benbowler Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

benbowler Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

zutigrm Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

benbowler Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benbowler left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benbowler left a comment

Choose a reason for hiding this comment

Uh oh!

tofumatt left a comment

Choose a reason for hiding this comment

Uh oh!

tofumatt Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

zutigrm Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

tofumatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Mar 11, 2026 •

edited

Loading

github-actions bot commented Mar 17, 2026 •

edited

Loading