Fix possible error when generating reports for custom dimensions

[sgiehl]

Description:

I have been able to reproduce #18608 with the dataset provided in #18608 (comment)

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) #0/core/Archive/Chunk.php(33),#1/core/DataAccess/ArchiveSelector.php(293),#2/core/Archive.php(528),#3/core/Archive.php(373),#4/core/Archive.php(464),#5/plugins/CustomDimensions/API.php(63),[internal function]: Piwik\Plugins\CustomDimensions\API->getCustomDimension(),#7/core/API/Proxy.php(244),#8/core/Context.php(28),#9/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:

matomo/core/API/Proxy.php

Lines 409 to 441 in a01eea3

	private function getRequestParametersArray($requiredParameters, $parametersRequest)
	{
	$finalParameters = array();
	foreach ($requiredParameters as $name => $defaultValue) {
	try {
	if ($defaultValue instanceof NoDefaultValue) {
	$requestValue = Common::getRequestVar($name, null, null, $parametersRequest);
	} else {
	try {
	if ($name == 'segment' && !empty($parametersRequest['segment'])) {
	// segment parameter is an exception: we do not want to sanitize user input or it would break the segment encoding
	$requestValue = ($parametersRequest['segment']);
	} else {
	$requestValue = Common::getRequestVar($name, $defaultValue, null, $parametersRequest);
	}
	} catch (Exception $e) {
	// Special case: empty parameter in the URL, should return the empty string
	if (isset($parametersRequest[$name])
	&& $parametersRequest[$name] === ''
	) {
	$requestValue = '';
	} else {
	$requestValue = $defaultValue;
	}
	}
	}
	} catch (Exception $e) {
	throw new Exception(Piwik::translate('General_PleaseSpecifyValue', array($name)));
	}
	$finalParameters[$name] = $requestValue;
	}
	return $finalParameters;
	}

The API definition for CustomDimensions is this one:

matomo/plugins/CustomDimensions/API.php

Line 54 in a01eea3

public function getCustomDimension($idDimension, $idSite, $period, $date, $segment = false, $expanded = false, $flat = false, $idSubtable = null)

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:

matomo/core/API/Proxy.php

Line 422 in a01eea3

$requestValue = Common::getRequestVar($name, $defaultValue, null, $parametersRequest);

to throw an exception as the $defaultValue is null.
See

matomo/core/Common.php

Lines 507 to 515 in 4f5cda9

	if (empty($varName)
	|| !isset($requestArrayToUse[$varName])
	|| (!is_array($requestArrayToUse[$varName])
	&& strlen($requestArrayToUse[$varName]) === 0
	)
	) {
	if (is_null($varDefault)) {
	throw new Exception("The parameter '$varName' isn't set in the Request, and a default value wasn't provided.");
	} else {

Note: $requestArrayToUse[$varName] is set to the empty string here

which then causes the value to be set to an empty string here:

matomo/core/API/Proxy.php

Lines 426 to 430 in a01eea3

	if (isset($parametersRequest[$name])
	&& $parametersRequest[$name] === ''
	) {
	$requestValue = '';
	} else {

The idSubtable is then passed through various methods, finally causing the issue here:

matomo/core/Archive/Chunk.php

Lines 31 to 33 in 1155273

	public function getRecordNameForTableId($recordName, $tableId)
	{
	$chunk = (floor($tableId / self::NUM_TABLES_IN_CHUNK));

---

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

matomo/core/API/Proxy.php

Line 422 in a01eea3

$requestValue = Common::getRequestVar($name, $defaultValue, null, $parametersRequest);

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

• Functional review done
• Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
• Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
• Security review done
• Wording review done
• Code review done
• Tests were added if useful/possible
• Reviewed for breaking changes
• Developer changelog updated if needed
• Documentation added if needed
• Existing documentation updated if needed

[tsteur]

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

[sgiehl]

@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.