Add convenience method for rescheduling archiving for a single plugin. #16727
Conversation
| if (empty($lastCronArchiveTime)) { | ||
| $dateTime = $lastDeactivationTime; | ||
| } else if (empty($lastDeactivationTime)) { | ||
| $dateTime = Date::factory($lastCronArchiveTime); |
There was a problem hiding this comment.
@diosmosis if Option::get(CronArchive::OPTION_ARCHIVING_FINISHED_TS); returns false then here it would end up being Date::factory(0) which may cause issues? It would be
The date '1970-01-01' is a date before first website was online. Try date that's after Aug 6, 1991 (timestamp 681436800).: 0
There was a problem hiding this comment.
in that case the first if branch should be hit (empty($lastCronArchiveTime))
There was a problem hiding this comment.
👍 didn't check but might be good to have a test for it if it's not the case yet
There was a problem hiding this comment.
should be covered by the first test case in PluginTest
| if (empty($lastCronArchiveTime)) { | ||
| $dateTime = $lastDeactivationTime; | ||
| } else if (empty($lastDeactivationTime)) { | ||
| $dateTime = Date::factory($lastCronArchiveTime); |
There was a problem hiding this comment.
Shouldn't $dateTime be null if the plugin had not been deactivated before, so it triggers archiving for all dates in the past? 🤔
There was a problem hiding this comment.
@diosmosis correct me if I'm wrong but the rearchive method should then limit it to say max 6 months (or whatever value is configured)
There was a problem hiding this comment.
it would be limited here: https://github.com/matomo-org/matomo/blob/4.0.0-rc4/core/Archive/ArchiveInvalidator.php#L470-L472
There was a problem hiding this comment.
@diosmosis did you have a look at my comment? Still thinking using $lastCronArchiveTime in this case seems wrong.
There was a problem hiding this comment.
@sgiehl it should then be limited see link above or not? also if lastCronArchiveTime is empty then it would have went in the first if
There was a problem hiding this comment.
But $dateTime is set to the last archiving time, so the reports would be built from that date on, or not?
There was a problem hiding this comment.
I see. Assuming it last ran today it would only rearchive today? Is this what you mean? We could set it to null in that case?
There was a problem hiding this comment.
I think it's not expected for the plugin deactivation time to be unset except in the case of installing a plugin (ie, the normal workflow is deactivate plugin, later activate, which triggers this code). In that case we can just set it to null?
| $dateTime = Date::factory($lastCronArchiveTime); | ||
| } else { | ||
| $lastCronArchiveTime = Date::factory($lastCronArchiveTime); | ||
| $dateTime = $lastDeactivationTime->isEarlier($lastCronArchiveTime) ? $lastDeactivationTime : $lastCronArchiveTime; |
There was a problem hiding this comment.
Assuming that the last archive run time is smaller than the deactivation date, wouldn't it make sense to simply skip calling scheduleReArchiving. As no archives should have been built for any newer dates yet, they should be build with the next archiving process nevertheless, shouldn't they?
There was a problem hiding this comment.
last archive run time is not really so reliable. AFAIK it gets updated even if only few archives finish etc but doesn't mean they all finished. I reckon it's fine in worst case to rather rearchive a few days of reports
There was a problem hiding this comment.
they wouldn't because I don't think core:archive will create invalidations for them, but that's another problem I think... @tsteur if core:archive hasn't run for a few days, should we automatically create invalidations for those days? We'd check the last run time and if it's more than just today/yesterday, add more invalidations.
|
@diosmosis looking through all the comments etc this one looks good to merge for me. Can you confirm? |
|
@tsteur will merge |
Description:
Add new method Plugin::schedulePluginReArchiving() to allow rescheduling archiving for a plugin.
Review