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 for deprecation warning in SimpleGraphHelper::bar() #981

Merged
merged 3 commits into from
Dec 21, 2023
Merged

Fix for deprecation warning in SimpleGraphHelper::bar() #981

merged 3 commits into from
Dec 21, 2023

Conversation

jharder
Copy link
Contributor

@jharder jharder commented Dec 21, 2023

When using DebugKit, on the Timer tab the bar graphs all display an error message:

Deprecated (8192): Implicit conversion from float 126.51705741882324 to int loses precision [in /var/cake/vendor/cakephp/debug_kit/src/View/Helper/SimpleGraphHelper.php, line 55]

The bar() function currently uses these parameters like this:
$value:
$graphValue = $value / $max * $width;

$offset:
$graphOffset = $offset / $max * $width;

Both new $graph* values are then provided to round(), which handles conversion to int. The raw values are then only used in displaying the message. From what I can tell, this function can handle float as well as int values, and this fix updates the type hint to allow the actual consumers of the function to use it as they have historically.

Added a new test to cover the explicit case. All test cases pass.

* @param array $options Graph options
* @return string Html graph
*/
public function bar(float $value, int $offset, array $options = []): string
public function bar(float|int $value, float|int $offset, array $options = []): string
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? Couldnt we just cast to int beforehand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then you would lose precision in the text output of the alt text "Starting 0ms into the request, taking 263.582ms". If we cast to int before calling this function, then probably a bunch of the times would round down to 0ms, which is incorrect.

As far as I can tell there is only one caller to this method, and that's in templates/element/timer_panel.php. You could cast to int there, but looking at a my own time graphs, there are quite a few elements that would lose information by reducing the input precision.

@dereuromark dereuromark merged commit ffb0746 into cakephp:5.x Dec 21, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants