@tsteur opened this Pull Request on December 20th 2018 Member

Got this error warning

WARNING: Trying to add two strings in DataTable\Row::sumRowArray: 35.8% + 48.6% for row # ['label' => -2, 'nb_visits' => 379, 'growth_percent' => '35.8%', 'growth_percent_numeric' => '35.8', 'grown' => 1, 'value_old' => 162, 'value_new' => 220, 'difference' => 58, 'importance' => 58, 'isDisappeared' => , 'isNew' => , 'isMover' => 1] [] [idsubtable = ]<br />

The totals row should be only needed for HTML table visualisations. Bar and Pie, etc should not need this totals row and therefore disabled it by default so other visualisations don't need to disable it. Makes viewing reports faster...

@tsteur commented on December 20th 2018 Member

Checked on our instance and the notice don't seem to come up anymore.

@sgiehl commented on December 21st 2018 Member

Tried to have a closer look. But actually it's a bit confusing how that's currently working:

In visualizations (except of HtmlTable) totals is always set to false, but the calculation in PHP defaults to true. e.g. https://github.com/matomo-org/matomo/blob/a860a9b7e8dcbb1489d7ed017c31c35c937fb234/core/API/DataTableManipulator/ReportTotalsCalculator.php#L83 and https://github.com/matomo-org/matomo/blob/40d68b9d36723468659819b731a33d4c9c365e89/core/API/DataTablePostProcessor.php#L199

That means whenever the totals parameter is missing it will be generated, which is also the case when fetching Insights.getInsights, or viewing charts like Pie.

To fully disable total row calculation for Insights, you might need to add totals => 0 here: https://github.com/matomo-org/matomo/blob/7e05d97b42e380acfdd166a6ad2a6fc78ea02340/plugins/Insights/Visualizations/Insight.php#L48-L59

Summarized it seems like totals is never set in any request, except of those where you now manually added the total => 0 to the request.

Btw. this variable naming is also bit confusing, or the comment is wrong:
https://github.com/matomo-org/matomo/blob/53c4d95a4b60ee099400db8c36ac64a68ffa37a3/plugins/CoreVisualizations/Visualizations/HtmlTable/Config.php#L90-L95

@tsteur commented on December 23rd 2018 Member

cheers @sgiehl fixed it 👍 There was also an issue that when switching visualisation from HtmlTable to Insights it would still send totals=1 in the URL.

And the $totals = false didn't work cause here it would add the parameter only when the default is not false : https://github.com/matomo-org/matomo/blob/3.8.0-b5/core/ViewDataTable/Request.php#L82-L83 Set it to 0 instead.

@diosmosis commented on December 25th 2018 Member

Code seems to work, looks ok and build passes, will merge. @sgiehl if you want to review more, feel free to do so, will create a new issue or just apply a fix if everyone's on vacation.

This Pull Request was closed on December 25th 2018
Powered by GitHub Issue Mirror