@flamisz opened this Pull Request on March 9th 2021 Contributor

Description:

fixes #17092

The package we use for rendering graphs has a bug. To avoid run into that bug, we can check the values we send to the package and if the values would cause the modulo zero error we throw an exception instead.
Our plugin throws the exceptions as an image by default, but for this error it's not that useful, because usually it happens when the height value is too small, so the user wouldn't be able to read the error message.
For this specific exception I used the default way.

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@sgiehl commented on March 9th 2021 Member

Wondering if it might make more sense to define lower limits for height & weight and automatically set those parameters to the lower limit if they are smaller. That way we could avoid any exception at all.

@flamisz commented on March 9th 2021 Contributor

Wondering if it might make more sense to define lower limits for height & weight and automatically set those parameters to the lower limit if they are smaller. That way we could avoid any exception at all.

Hey @sgiehl, me and @tsteur were talking about that, and I could calculate that, but if the user wants a lower number, and the graph will be higher, maybe she/he will think it is a bug. Hard to tell which is the better solution, but probably it is something that happens very rarely.

@tsteur commented on March 9th 2021 Member

@sgiehl the thought was that if we automatically change a height or width then it will look like a bug and we're getting bug reports and emails about it vs this way it's clear what the problem is and they can change the parameters to a higher number.

@sgiehl commented on March 10th 2021 Member

Thought we would have it that way in other places, but wasn't able to find one right now. But it's also fine this way.

This Pull Request was closed on March 11th 2021
Powered by GitHub Issue Mirror