@sgiehl opened this Pull Request on January 13th 2022 Member

Description:

I have been able to reproduce #18608 with the dataset provided in https://github.com/matomo-org/matomo/issues/18608#issuecomment-1011370361

On PHP 7.2 I actually get a

WARNING CustomDimensions[2022-01-13 15:00:37 UTC] [faa1f] /srv/matomo/core/Archive/Chunk.php(33): Warning - A non-numeric value encountered - Matomo 4.7.0-b2 - Please report this message in the Matomo forums: https://forum.matomo.org (please do a search first as it might have been reported already) <a href='/0'>#0</a>/core/Archive/Chunk.php(33),<a href='/1'>#1</a>/core/DataAccess/ArchiveSelector.php(293),<a href='/2'>#2</a>/core/Archive.php(528),<a href='/3'>#3</a>/core/Archive.php(373),<a href='/4'>#4</a>/core/Archive.php(464),<a href='/5'>#5</a>/plugins/CustomDimensions/API.php(63),[internal function]: Piwik\Plugins\CustomDimensions\API->getCustomDimension(),<a href='/7'>#7</a>/core/API/Proxy.php(244),<a href='/8'>#8</a>/core/Context.php(28),<a href='/9'>#9</a>/core/API/Proxy.php(335)

PHP 8 throws the error describe in the issue.

I took a deep dive through the code and the issue now appears to be clear to me.

The report generator uses the API Proxy to fetch API results. Therefor the parameters are determined using this method:

https://github.com/matomo-org/matomo/blob/a01eea35273f7c569d060ec455ec0c0de07d1582/core/API/Proxy.php#L409-L441

The API definition for CustomDimensions is this one:
https://github.com/matomo-org/matomo/blob/a01eea35273f7c569d060ec455ec0c0de07d1582/plugins/CustomDimensions/API.php#L54

So the default value for $idSubtable is actually null. In the request variables idSubtable is set to an empty string.

This causes the method to fetch the parameter value here:
https://github.com/matomo-org/matomo/blob/a01eea35273f7c569d060ec455ec0c0de07d1582/core/API/Proxy.php#L422
to throw an exception as the $defaultValue is null.
See
https://github.com/matomo-org/matomo/blob/4f5cda954a6b64677f9b48223a91e0ec8785e055/core/Common.php#L507-L515
Note: $requestArrayToUse[$varName] is set to the empty string here

which then causes the value to be set to an empty string here:
https://github.com/matomo-org/matomo/blob/a01eea35273f7c569d060ec455ec0c0de07d1582/core/API/Proxy.php#L426-L430

The idSubtable is then passed through various methods, finally causing the issue here:
https://github.com/matomo-org/matomo/blob/115527353a9e75e01aa4d263408956ae45403bea/core/Archive/Chunk.php#L31-L33


Regarding the solution there are actually two ways to fix that.
One way is to simply handle an empty string for the idSubtable as null, like false is already handled.

Another fix would be to change the definition of the API method to $idSubtable = false instead of null.
The reason why that would work is, as this part
https://github.com/matomo-org/matomo/blob/a01eea35273f7c569d060ec455ec0c0de07d1582/core/API/Proxy.php#L422
will no longer throw an exception. The false is a valid default value, which will simply be returned. And false is already handled correctly for idSubtable in the code.

This PR actually applies both adjustments. In all places I could find, we are currently using $idSubtable = false, so there is no good reason to use null in that one case. On the other side it can happen any time again, that someone defines null as default value again, which then could cause the same issue again.

fixes #18608

Review

@tsteur commented on January 13th 2022 Member

Nice find! Looks good. Is it maybe possible to write a test for it or rather not?

@sgiehl commented on January 14th 2022 Member

@tsteur I have added a test, that generates a PDF report for custom dimensions. On 4.x-dev this test generates a PDF as well, but the graphs for custom dimension reports simply contain the message there is not data for this graph. With the fixes here, the graphs are displayed correctly.

This Pull Request was closed on January 16th 2022
Powered by GitHub Issue Mirror