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
Ensure to use correct date while archiving #15728
Conversation
@@ -991,7 +990,7 @@ public function isThereAValidArchiveForPeriod($idSite, $period, $date, $segment | |||
if ($isTodayIncluded | |||
&& !$isLast | |||
) { | |||
return [false, null]; | |||
return [false, $date]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to return the date here, as otherwise the date used for archiving will be null here:
Line 902 in 9969201
list($isThereArchive, $newDate) = $this->isThereAValidArchiveForPeriod($idSite, 'day', $date, $segment = ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sgiehl did you notice this by fixing some test or was something not working? I assume there was some core:archive used with specifically today?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see the linked issue #15727
@@ -978,10 +977,10 @@ protected function processArchiveDays($idSite, $lastTimestampWebsiteProcessedDay | |||
public function isThereAValidArchiveForPeriod($idSite, $period, $date, $segment = '') | |||
{ | |||
if (Range::isMultiplePeriod($date, $period)) { | |||
$rangePeriod = Factory::build($period, $date, Site::getTimezoneFor($idSite)); | |||
$rangePeriod = PeriodFactory::build($period, $date, Site::getTimezoneFor($idSite)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sgiehl is that change needed or is it actually causing an issue? Just asking out of curiosity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really needed. But it's a bit messy to import the same class twice with different aliases...
LGTM, there should be some tests covering this behavior that will need to be udpated. |
Yes
|
refs #15727