Skip to content
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

Unsupported operand types: string / int in Archive/Chunk.php when archiving events #19009

Closed
tsteur opened this issue Mar 28, 2022 · 4 comments · Fixed by #19027
Closed

Unsupported operand types: string / int in Archive/Chunk.php when archiving events #19009

tsteur opened this issue Mar 28, 2022 · 4 comments · Fixed by #19027
Assignees
Labels
Potential Bug Something that might be a bug, but needs validation and confirmation it can be reproduced. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Milestone

Comments

@tsteur
Copy link
Member

tsteur commented Mar 28, 2022

Recently we fixed this issue #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}"}

@tsteur tsteur added Regression Indicates a feature used to work in a certain way but it no longer does even though it should. Potential Bug Something that might be a bug, but needs validation and confirmation it can be reproduced. labels Mar 28, 2022
@tsteur tsteur added this to the 4.9.0 milestone Mar 28, 2022
@sgiehl
Copy link
Member

sgiehl commented Mar 28, 2022

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
Copy link
Member Author

tsteur commented Mar 28, 2022

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
Copy link
Member

sgiehl commented Mar 29, 2022

@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
Copy link
Member Author

tsteur commented Mar 29, 2022

@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;
         }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Potential Bug Something that might be a bug, but needs validation and confirmation it can be reproduced. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants