Skip to content

Embedded graphs in reports render with raw metrics.#24442

Open
nathangavin wants to merge 2 commits into5.x-devfrom
dev-20025
Open

Embedded graphs in reports render with raw metrics.#24442
nathangavin wants to merge 2 commits into5.x-devfrom
dev-20025

Conversation

@nathangavin
Copy link
Copy Markdown
Contributor

Description

Addresses #24273

When generating a report with embedded graphs in a language that uses different delimiters with numbers (e.g. '1.2' becomes '1,2'), the ImageGraph API was fetching metrics in the translated form, and using them to create the graphs to embed. Older PHP versions would implicitly handle these metrics (on PHP 7.2, '1,2' would be intepreted as 1), however newer PHP versions will hard fail on the ceil() method when passed a string.

This fix addresses this by first requesting raw metrics when generating the graph. This means that metrics should always be formatted in the standard style, which is intepreted by PHP type casting more consistently. Secondly, the method for processing ordinance values for the ImageGraph API has been hardened to always return a Float, which is what the graph generation code is expecting.

Since the raw metrics are only used to generate the graph shape (bar size, line shape etc), the end user should not be able to see any difference.

Checklist

  • [✔] I have understood, reviewed, and tested all AI outputs before use
  • [✔] All AI instructions respect security, IP, and privacy rules

Review

@nathangavin nathangavin added this to the 5.10.0 milestone Apr 29, 2026
@nathangavin nathangavin requested a review from a team April 29, 2026 05:52
Copy link
Copy Markdown
Contributor

@chippison chippison left a comment

Choose a reason for hiding this comment

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

A few simplfications and everythings all good!
👍

Comment thread plugins/ImageGraph/API.php Outdated
Comment on lines +615 to +619
if ($ordinateValue === null || $ordinateValue === '' || $ordinateValue === '.') {
return 0.0;
}

return is_numeric($ordinateValue) ? (float) $ordinateValue : 0.0;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can be replaced by:
After the preg_replace, the value can only be digits and dots, is_numeric does not add any value anymore. The function empty() also covers null and empty string

Suggested change
if ($ordinateValue === null || $ordinateValue === '' || $ordinateValue === '.') {
return 0.0;
}
return is_numeric($ordinateValue) ? (float) $ordinateValue : 0.0;
if (empty($ordinateValue) || $ordinateValue === '.') {
return 0.0;
}
return (float) $ordinateValue;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yep agreed, makes sense.

Comment thread plugins/ImageGraph/API.php Outdated
Comment on lines +597 to +601
if (is_int($ordinateValue) || is_float($ordinateValue)) {
return (float) $ordinateValue;
}

$ordinateValue = @str_replace(',', '.', (string) $ordinateValue);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can be simplified a bit more by:
is_numeric will cover both is_int and is_float
no need to add @ as you have force $ordinateValue to be a string

Suggested change
if (is_int($ordinateValue) || is_float($ordinateValue)) {
return (float) $ordinateValue;
}
$ordinateValue = @str_replace(',', '.', (string) $ordinateValue);
if (is_numeric($ordinateValue)) {
return (float) $ordinateValue;
}
$ordinateValue = str_replace(',', '.', (string) $ordinateValue);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

same here, good suggestion.

@nathangavin nathangavin requested a review from chippison April 30, 2026 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants