Write correct output when a process is skipped #13866
Conversation
Prevents errors like ` Error unserializing the following response from ?module=API&method=API.get&idSite=1&period=year&date=last3&format=php&trigger=archivephp: Skipped` When I tested locally it also avoided the message `WARNING [2018-12-16 22:40:18] 5677 piwik/core/Common.php(271): Notice - unserialize(): Error at offset 0 of 7 bytes ` refs DEV-1521
tsteur
added
not-in-changelog
core/CliMulti.php
Outdated
| @@ -142,7 +142,7 @@ private function start($piwikUrls) | |||
| if ($shouldStart === Request::ABORT) { | |||
| // output is needed to ensure same order of url to response | |||
| $output = new Output($cmdId); | |||
| $output->write('Skipped'); | |||
| $output->write(serialize(array('skipped' => '1', 'nb_visits' => 0))); | |||
There was a problem hiding this comment.
the output doesn't really matter as long as it is a non-empty serialized array as we check with empty($metric['nb_visits']) etc.
There was a problem hiding this comment.
In theory the response would need to depend on the caller but maybe in this case it isn't too important? checked it is used as well in ScheduledTaskRunner but it doesn't use the "abort" feature and the response format doesn't matter there
There was a problem hiding this comment.
Perhaps in this case since the request is aborted, the output should be a null value? Like serialize(null)? Would have to change any uses to check that the value could be null. Or could be like API output w/ serialize(['result' => 'error']). I guess in any case callers would have to be aware of this new behavior.
There was a problem hiding this comment.
It's not really an error I think. It is just not needed to execute it anymore basically (eg because the same job is already running). Setting null could be difficult maybe as it would be hard to differentiate between the aborted case and eg no response received etc.
So far we call CLI Multi only in Task Runner and Cron Jobs and there it is all fine. The abort feature is not really any documented feature for plugins so shouldn't be a problem. Could set it to $output->write(serialize(array('aborted' => '1'))) though and explain the output in the description of the constant... maybe that makes it more clear. A caller has to specifically implement some support to ABORT a command so I don't think it's a problem.
There was a problem hiding this comment.
Just changed the output... the ABORT feature in there is currently actually specifically in there for the archiver anyway.
Prevents errors like
Error unserializing the following response from ?module=API&method=API.get&idSite=1&period=year&date=last3&format=php&trigger=archivephp: SkippedWhen I tested locally it also avoided the message
WARNING [2018-12-16 22:40:18] 5677 piwik/core/Common.php(271): Notice - unserialize(): Error at offset 0 of 7 bytesrefs DEV-1521
It is unserialized eg here: https://github.com/matomo-org/matomo/blob/3.x-dev/core/CronArchive.php#L867 and then evaluated here: https://github.com/matomo-org/matomo/blob/3.x-dev/core/CronArchive.php#L1433-L1463 (the other one expects an array of array but still works cause of
emptycheck)