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 divide by zero error in cloud visualisation #20386

Closed
wants to merge 1 commit into from

Conversation

bx80
Copy link
Contributor

@bx80 bx80 commented Feb 20, 2023

Description:

Fixes 20384

Use safe method for division to avoid potential divide by zero error

Review

@bx80 bx80 added the Bug For errors / faults / flaws / inconsistencies etc. label Feb 20, 2023
@bx80 bx80 added this to the 4.13.4 milestone Feb 20, 2023
@bx80 bx80 self-assigned this Feb 20, 2023
@bx80 bx80 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 Feb 20, 2023
@@ -194,7 +195,7 @@ private function getPercentage($popularity, $maxValue)
return 0;
}

$percent = ($popularity / $maxValue) * 100;
$percent = Piwik::getQuotientSafe($popularity, $maxValue, 4) * 100;
Copy link
Member

Choose a reason for hiding this comment

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

This actually changes the resulting precision of the number.
Also it actually only covers the real problem why the calculation fails. I'll add a comment to the original issue around that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking here is that the percent is being used to decide the size of the words in the cloud visualization, the original unsafe division wasn't applying a precision and four digit precision should be enough accuracy to differentiate between words.

It looks like this is too simplistic an approach and would be better fixed in #19940 - I'll close this PR as it was only intended as a possible quick fix.

@bx80 bx80 closed this Feb 21, 2023
@bx80 bx80 deleted the m20384-fix-divide-by-zero branch February 21, 2023 22:00
@bx80 bx80 removed this from the 4.13.4 milestone Feb 21, 2023
@bx80 bx80 removed the Needs Review PRs that need a code review label Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants