Do not confuse normal rows with the label "-1" w/ the summary row #17517
Conversation
There was a problem hiding this comment.
@diosmosis haven't tested it yet but left few quick comments. I'm still seeing heaps of calls to LABEL_SUMMARY_ROW and getRowFromLabel. Not sure if this can cause various side effects?
| } | ||
| } else { | ||
| $rowFound->sumRow($row, $copyMeta = true, $columnAggregationOps); | ||
| $thisRow->sumRow($otherRow, $copyMeta = true, $columnAggregationOps); |
There was a problem hiding this comment.
if $thisRow is not new and summary row will be aggregated based on the label or so, is it possible that we maybe would be aggregating a regular row with a summary row or the other way around and logic might not work 100% correct? Like would maybe need to call setIsNotSummaryRow or something and make sure it only aggregates summary rows together?
I'm thinking when it goes into aggregateRowWithLabel() and does $rowFound = $this->getRowFromLabel($labelToLookFor); then it might receive the summary row?
There was a problem hiding this comment.
I'm thinking when it goes into aggregateRowWithLabel() and does $rowFound = $this->getRowFromLabel($labelToLookFor); then it might receive the summary row?
It might... we only do that for BC in getRowFromLabel(). Maybe we can introduce a getRowFromLabelWithoutSummaryRow() function or something?
There was a problem hiding this comment.
@tsteur updated w/ the smallest fix I could find.
|
@tsteur note: I didn't fix every place where the error could occur, since there would be a lot to look for, but I can if needed. Or I can create a new issue to make sure we don't use getRowFromLabel/getRowIdFromLabel w/ LABEL_SUMMARY_ROW. |
|
@diosmosis seems a lot of uses of the label constant are in other plugins anyway so that might be fine for now. I'm thinking though it might be useful to call Thinking https://github.com/matomo-org/matomo/blob/4.x-dev/core/DataTable.php#L1319-L1321 might need to be tweaked too to not break anything. Be good to check. Eg in truncate check there might be a Also noticed there is a class I guess the other uses of the LABEL constant won't really cause any issues but be good to look over it. I'm thinking a different issue might not be needed then. |
We could do this, though it's added as the summary row here: https://github.com/matomo-org/matomo/blob/4.x-dev/plugins/Actions/ArchivingHelper.php#L388 If we do this for a row that isn't added with that method, however, it could cause other unforeseen problems, I think...
Already saw this. This is fine since the Truncate filter just sets the label of a new summary row which it adds. It doesn't search for a row by that label.
We could... however if the row is used as a normal row (for whatever reason) then it wouldn't be accurate and would lead to problems.
I did look at those, BUT I didn't look at every use of getRowFromLabel. It's possible code, for example, loops through every row and uses getRowFromLabel (in fact I think I found one place looking quickly a bit before):
|
|
Just tried local archiving with those changes, and the warning that was triggered before, isn't triggered anymore. But not yet sure if those changes might have other implications. |
|
The changes look good for me now. Not sure if we might have any issues with archives that have been built before, with mixed up row with |
|
@sgiehl I think the tables themselves will be handled fine, they'll just have been summed incorrectly (eg, adding the -1 row to the summary row or the summary row to the -1 row). The data would be off but we can't fix that w/o invalidating. |
|
Looks good @diosmosis
I would maybe make that change just in case it'll be used differently in the future. Otherwise once tests pass all good 👍 |
Description:
In several places, matomo will look for the summary row if the label == -1. This causes problems (sometimes fatal) if data is tracked w/ the value
-1. In this case there will be a normal row w/ the same label as the summary row, causing strange things to happen.This PR fixes this by relying less on using the label of a row to identify if it is a summary row. Changes include:
Row-1, then return the summary row (this is mainly for BC)Row,isSummaryRow, which is set by DataTable. this is now returned fromRow::isSummaryRow()instead of checking the labelReview