@bx80 opened this Pull Request on February 14th 2022 Contributor

Description:

Fixes #18773

When archiving a week, month or year some sub-periods may fall in the future (eg. it's Thursday so we want to archive this week, but Friday, Saturday and Sunday are in the future).

Writing blank archive data for the future causes problems (and is inefficient) so the code in Archive.php#L629-L643 skips any archives for periods with a start date in the future or before the website was created. However for some reason it only skips archives with start dates two days greater than the current date, this causes blank archives to be written for today+1 and today+2.

This is a simple fix to skip any periods which start on a date later than the current date.

Prior to this change a running ./console core:archive command would result in Archive::prepareArchive() being called for dates two days into the future, after the change the method is only called up to and include the current date.

Review

@sgiehl commented on February 14th 2022 Member

@bx80 could you maybe add some tests for that? Could be a simple test that triggers the archiving and checks which records are created in the archive tables.
Note: To ensure the test assumes a certain date is in the future, you can manipulate the "current time" by setting Date::$now

@sgiehl commented on February 21st 2022 Member

The code looks fine. But what I'm not fully sure about is, if that might cause issues when using other timezones. But guess even if the website is in a timezone that is ahead UTC, the start date should still not be in the future. 🤔 🤯

@sgiehl commented on February 23rd 2022 Member

@tsteur would you mind having a quick look at this one. I guess it should be fine. But I'm always unsure if those changes could result in any timezone related issues. Maybe we should check if we have enough tests that are using other timezones to ensure this won't cause any problems?

@sgiehl commented on February 24th 2022 Member

Ok. Guess we should then start adding some archiving tests for sites with timezones ahead of UTC, while faking a specific time to ensure the timezone is already a day ahead.

@bx80 commented on February 28th 2022 Contributor

I've added an additional test which will expect the current local date to be included for archiving even if it's ahead of UTC. The range in the future check is now based of the site local time date rather than the UTC date at midnight.

@sgiehl commented on February 28th 2022 Member

@bx80 guess that logically makes sense. Would you mind also adding a test case where the site's timezone is behind UTC and UTC time is already a day ahead? Guess in that case it shouldn't create archives as well, right?

This Pull Request was closed on March 8th 2022
Powered by GitHub Issue Mirror