Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix others row might not be replaced in subtables of others row #12541

Merged
merged 2 commits into from Mar 19, 2018

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Feb 7, 2018

No description provided.

@tsteur tsteur added the Needs Review PRs that need a code review label Feb 7, 2018
@mattab mattab added this to the 3.3.1 milestone Mar 19, 2018
@sgiehl
Copy link
Member

sgiehl commented Mar 19, 2018

@tsteur are you able to give an example where that issue appeared in the first place? Not sure how to reproduce and test it...

@tsteur
Copy link
Member Author

tsteur commented Mar 19, 2018

It's actually quite complicated to reproduce this and I don't remember 100% the structure that you need to have in order to reproduce this. You need like three levels of a dataTable and then a summary row in each of the levels or so.

We have this patch in production though for a while and can confirm the patch works.

if ($subTable) {
$this->filter($subTable);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using ->getRows() instead of ->getRowsWithoutSummaryRow() in the iteration above should have the same effect

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it needs a lot more memory and time though. We refactored this a while ago as getRows() is quite slow etc. When possible they should be always used separate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add a comment to the getRows() method in datatable class, that it should be used with caution.
At the first look it does not seem to make much difference as it simply returns a bit more:

    public function getRows()
    {
        if (is_null($this->summaryRow)) {
            return $this->rows;
        } else {
            return $this->rows + array(self::ID_SUMMARY_ROW => $this->summaryRow);
        }
    }

But for sure internally it creates a new copy of the full rows array (to attach the summary row) instead of returning a reference...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and this method might be executed many thousand times... makes a huge difference. I'll add a comment but it is mainly meant for core developer to not use it. For third party plugin developers it will be easier to use getRows() in order to prevent random bugs where they miss the summary row.

@tsteur tsteur merged commit 0f97d34 into 3.x-dev Mar 19, 2018
@tsteur tsteur deleted the subtableothers branch March 19, 2018 19:34
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
…mo-org#12541)

* Fix others row might not be replaced in subtables of others row

* add comment re performance
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants