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
Conversation
@tsteur are you able to give an example where that issue appeared in the first place? Not sure how to reproduce and test it... |
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); | ||
} | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
…mo-org#12541) * Fix others row might not be replaced in subtables of others row * add comment re performance
No description provided.