@tsteur opened this Issue on March 28th 2022 Member

Recently we fixed this issue https://github.com/matomo-org/matomo/issues/18608

We are now seeing the same errors happening when trying to archive for example events. See below. We are getting this error many times a day and stops the archiving from working. not sure if the same could also happen for other reports.

Error: {"message":"Unsupported operand types: string \/ int","file":"\/core\/Archive\/Chunk.php","line":33,"request_id":"b406a","backtrace":" on \/core\/Archive\/Chunk.php(33)\n#0 \/core\/DataAccess\/ArchiveSelector.php(293): Piwik\Archive\Chunk->getRecordNameForTableId('Events_category...', 'undefined')\n#1 \/core\/Archive.php(528): Piwik\DataAccess\ArchiveSelector::getArchiveData(Array, Array, 'blob', 'undefined')\n#2 \/core\/Archive.php(373): Piwik\Archive->get(Array, 'blob', 'undefined')\n#3 \/core\/Archive.php(464): Piwik\Archive->getDataTable('Events_category...', 'undefined')\n#4 \/plugins\/Events\/API.php(157): Piwik\Archive::createDataTableFromArchive('Events_category...', '1', 'day', '2022-03-25', 'deviceType==sma...', false, false, 'undefined')\n#5 \/plugins\/Events\/API.php(193): Piwik\Plugins\Events\API->getDataTable('getActionFromCa...', '1', 'day', '2022-03-25', 'deviceType==sma...', false, 'undefined')\n#6 [internal function]: Piwik\Plugins\Events\API->getActionFromCategoryId('1', 'day', '2022-03-25', 'undefined', 'deviceType==sma...')\n#7 \/core\/API\/Proxy.php(244): call_user_func_array(Array, Array)\n#8 \/core\/Context.php(28): Piwik\API\Proxy->Piwik\API\{closure}()\n#9 \/core\/API\/Proxy.php(335): Piwik\Context::executeWithQueryParameters(Array, Object(Closure))\n#10 \/core\/API\/Request.php(266): Piwik\API\Proxy->call('\\Piwik\\Plugins\\...', 'getActionFromCa...', Array)\n#11 \/plugins\/API\/Controller.php(46): Piwik\API\Request->process()\n#12 [internal function]: Piwik\Plugins\API\Controller->index()\n#13 \/core\/FrontController.php(626): call_user_func_array(Array, Array)\n#14 \/core\/FrontController.php(168): Piwik\FrontController->doDispatch('API', false, Array)\n#15 \/core\/dispatch.php(32): Piwik\FrontController->dispatch()\n#16 \/index.php(25): require_once('\/c...')\n#17 {main}","safemode_backtrace":"#0 [internal function]: Piwik\Plugins\Cloud\Controller->safemode(Array)\n#1 \/core\/FrontController.php(626): call_user_func_array(Array, Array)\n#2 \/core\/FrontController.php(168): Piwik\FrontController->doDispatch('Cloud', 'safemode', Array)\n#3 \/core\/FrontController.php(99): Piwik\FrontController->dispatch('CorePluginsAdmi...', 'safemode', Array)\n#4 \/core\/FrontController.php(139): Piwik\FrontController::(Array)\n#5 \/core\/FrontController.php(189): Piwik\FrontController::(Object(TypeError))\n#6 \/core\/dispatch.php(32): Piwik\FrontController->dispatch()\n#7 \/index.php(25): require_once('\/c...')\n#8 {main}"}

@sgiehl commented on March 28th 2022 Member

The problem here seems that for some reason the string undefined is passed as idSubtable, which seems weird.
Are you sure that happens while archiving? Wondering why the archiving should query the events API with such an idSubtable

@tsteur commented on March 28th 2022 Member

Indeed, it's not happening during archiving. Sorry about that @sgiehl

Here's an example URL (seems the user actually passed undefined as ID, it's an API call not coming from the UI ):

URL: https://example.matomo.org/index.php?module=API&method=Events.getActionFromCategoryId&idSite=1&period=day&format=json&token_auth=XYZANONYMIZED&showColumns=nb_events&filter_limit=-1&idSubtable=undefined&date=2022-03-27&segment=deviceType==smartphone

@sgiehl commented on March 29th 2022 Member

@tsteur So it's actually a fail on user side, as a invalid parameter value is provided. I wouldn't handle that as a regression, as it might never have worked. It's also a more general problem. We do not have some proper API parameter handling / validation to avoid incorrect values. Personally I would prefer a global solution, so we don't end up fixing such issues all the time.
I haven't checked that yet, but maybe we can add type hints to the API method parameters. And at the point were we try to match the provided URL parameters to API parameters using reflection we could check if provided value can be casted to the expected type. If that is not the case we can throw a generic "invalid parameter value provided - expected XY" exception.
In addition we should consider introducing some parameter validation that can be easily called in API methods. There's no benefit in checking correct types / values across all APIs again and again. Like e.g. idSite can often be an integer, but in some cases also a list separated by , or the string all,...

@tsteur commented on March 29th 2022 Member

@sgiehl I consider it a regression since it worked with older PHP versions and didn't trigger a fatal error. We just want to avoid fatal errors. We don't need to look for a global solution right now, we could simply fix that case where something like below may be good enough (not tested):

diff --git a/core/Archive.php b/core/Archive.php
index 09227df4d8..0f13e3b43f 100644
--- a/core/Archive.php
+++ b/core/Archive.php
@@ -449,6 +449,10 @@ class Archive implements ArchiveQuery
     {
         Piwik::checkUserHasViewAccess($idSite);

+        if (!empty($idSubtable) && !is_numeric($idSubtable)) {
+            throw new \Exception("idSubtable needs to be a number, '$idSubtable' given.");
+        }
+
         if ($flat && !$idSubtable) {
             $expanded = true;
         }
This Issue was closed on April 5th 2022
Powered by GitHub Issue Mirror