Fix possible error when generating reports for custom dimensions #18614

sgiehl · 2022-01-13T17:00:38Z

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

tsteur · 2022-01-13T19:48:24Z

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

sgiehl · 2022-01-14T09:24:10Z

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

sgiehl added 2 commits January 12, 2022 10:26

handle an empty string as null for subtable ids

f56e740

Adjust API definition

081be75

sgiehl added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Jan 13, 2022

sgiehl added this to the 4.7.0 milestone Jan 13, 2022

sgiehl requested a review from tsteur January 13, 2022 17:00

Merge branch '4.x-dev' into archivefix

352844e

Adds test

2fd1493

sgiehl force-pushed the archivefix branch from 08d6ce6 to 2fd1493 Compare January 14, 2022 10:24

tsteur approved these changes Jan 16, 2022

View reviewed changes

sgiehl merged commit d1989fe into 4.x-dev Jan 16, 2022

sgiehl deleted the archivefix branch January 16, 2022 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix possible error when generating reports for custom dimensions #18614

Fix possible error when generating reports for custom dimensions #18614

sgiehl commented Jan 13, 2022 •

edited

tsteur commented Jan 13, 2022

sgiehl commented Jan 14, 2022

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

	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 {

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

Fix possible error when generating reports for custom dimensions #18614

Fix possible error when generating reports for custom dimensions #18614

Conversation

sgiehl commented Jan 13, 2022 • edited

Description:

Review

tsteur commented Jan 13, 2022

sgiehl commented Jan 14, 2022

sgiehl commented Jan 13, 2022 •

edited