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

Already valid archives for yesterday might get invalidated when using another timezone than UTC #22299

Merged
merged 5 commits into from
Jun 28, 2024

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Jun 10, 2024

Description:

Archiving automatically invalidates archives for today and yesterday if needed.

The checks for this including comparing the archived period for including today, as well as checking if the ts_archived for yesterdays archived was done after yesterday.

It seems that those checks are not performed using the correct timezones.
While the archiving period is provided in the sites timezone, ts_archived is always stored as UTC. Therefore checks with those dates need to use the same timezone.

There actually had been tests for the method, which were all using UTC. Running the tests with other timezones actually caused failures. With the adjustments I did in the PR the tests are now passing for all timezones.

Review

@sgiehl sgiehl marked this pull request as ready for review June 19, 2024 14:11
@sgiehl sgiehl added Bug For errors / faults / flaws / inconsistencies etc. Needs Review PRs that need a code review labels Jun 19, 2024
@sgiehl sgiehl added this to the 5.2.0 milestone Jun 19, 2024
@sgiehl sgiehl changed the title Ensure to check dates using same timezone for invalidating recent archives Already valid archives for yesterday might get invalidated when using another timezone than UTC Jun 19, 2024
@sgiehl sgiehl requested a review from a team June 19, 2024 14:14
@sgiehl sgiehl removed the request for review from a team June 19, 2024 15:39
@sgiehl sgiehl removed the Needs Review PRs that need a code review label Jun 19, 2024
@sgiehl sgiehl force-pushed the archivecheck branch 2 times, most recently from 4666205 to f1e315d Compare June 19, 2024 20:22
@sgiehl sgiehl requested a review from a team June 19, 2024 22:06
@sgiehl sgiehl added the Needs Review PRs that need a code review label Jun 19, 2024
@sgiehl sgiehl force-pushed the archivecheck branch 11 times, most recently from 5b65a56 to 7ce864f Compare June 21, 2024 13:10
Copy link
Contributor

@michalkleiner michalkleiner left a comment

Choose a reason for hiding this comment

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

I've suggested a change from 'day switch' to 'midnight' as I think that is what was meant by it, when one day rolls over to a new day (and the date changes, time goes from 23:59:59 to 0:00:00). I asked ChatGPT and midnight was the best suggestion.

tests/PHPUnit/Integration/CronArchiveTest.php Outdated Show resolved Hide resolved
tests/PHPUnit/Integration/CronArchiveTest.php Outdated Show resolved Hide resolved
tests/PHPUnit/Integration/CronArchiveTest.php Outdated Show resolved Hide resolved
tests/PHPUnit/Integration/CronArchiveTest.php Outdated Show resolved Hide resolved
tests/PHPUnit/Integration/CronArchiveTest.php Outdated Show resolved Hide resolved
tests/PHPUnit/Integration/CronArchiveTest.php Outdated Show resolved Hide resolved
@michalkleiner
Copy link
Contributor

With some of the tests where we now have a comment that the test may not be correct, what do we want to do about those? Should we remove the comments and create a new issue instead to look into those?

@sgiehl sgiehl requested a review from a team June 24, 2024 07:51
@sgiehl
Copy link
Member Author

sgiehl commented Jun 24, 2024

With some of the tests where we now have a comment that the test may not be correct, what do we want to do about those? Should we remove the comments and create a new issue instead to look into those?

There is #22331 to change that directly.

@mneudert mneudert merged commit ece230f into 5.x-dev Jun 28, 2024
25 checks passed
@mneudert mneudert deleted the archivecheck branch June 28, 2024 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants