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

Min/max aspect ratios should be be rounded to a fixed nearest decimal #1765

Open
westonruter opened this issue Dec 20, 2024 · 8 comments · May be fixed by #1776
Open

Min/max aspect ratios should be be rounded to a fixed nearest decimal #1765

westonruter opened this issue Dec 20, 2024 · 8 comments · May be fixed by #1776
Labels
[Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Bug An existing feature is broken

Comments

@westonruter
Copy link
Member

Looking at a site in the wild I see the following in the page source:

"minViewportAspectRatio":0.40000000000000002220446049250313080847263336181640625,"maxViewportAspectRatio":2.5

This is a laughable minViewportAspectRatio. Nevertheless, the values 0.4 and 2.5 are what show up in the source:

return (float) apply_filters( 'od_minimum_viewport_aspect_ratio', 0.4 );

return (float) apply_filters( 'od_maximum_viewport_aspect_ratio', 2.5 );

Aren't floating point numbers fun.

I'm not sure how having such a long decimal would be prevented, as the return value of the functions there is sent straight to be exported as JSON here:

$detect_args = array(
'minViewportAspectRatio' => od_get_minimum_viewport_aspect_ratio(),
'maxViewportAspectRatio' => od_get_maximum_viewport_aspect_ratio(),

wp_json_encode( $detect_args )

Not a big issue.

@westonruter westonruter added [Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Bug An existing feature is broken labels Dec 20, 2024
@github-project-automation github-project-automation bot moved this to Not Started/Backlog 📆 in WP Performance 2024 Dec 20, 2024
@westonruter westonruter changed the title Min/max aspect ratios should be be rounded to the nearest 10s place decimal Min/max aspect ratios should be be rounded to a fixed nearest decimal Dec 20, 2024
@ShyamGadde
Copy link
Contributor

Just thinking out loud here...

When exploring potential solutions, one idea that came to mind was to convert the float values to strings before JSON encoding. Then on the client side, we could convert them back to numbers when needed. This approach seems straightforward, though it introduces an extra step in handling these values.

I also came across another potential solution involving PHP's serialize_precision setting:

$original_precision = ini_get( 'serialize_precision' );
ini_set( 'serialize_precision', -1 );
// ... wp_json_encode( $detect_args ) ...
ini_set( 'serialize_precision', $original_precision );

This seems to address the issue as well, but I'm not entirely sure about the potential pitfalls of changing configuration values at runtime. It doesn’t feel like the cleanest approach.

@westonruter
Copy link
Member Author

Another option would be simply to export the minViewportAspectRatio and maxViewportAspectRatio multiplied by 100 and cast as an integer. Then in JS, the numbers could be simply divided by 100. The key names would be confusing remaining as is, though.

@westonruter
Copy link
Member Author

@pbearne I see in #1773 you tried adding round(). Did this not work?

@pbearne
Copy link
Contributor

pbearne commented Jan 6, 2025

@westonruter confused by your comment
Are saying rounding does not work?
If so should code the multiplied by 100 solution?

@westonruter
Copy link
Member Author

@pbearne well, I see you closed your PR so I assumed it hadn't worked.

@pbearne
Copy link
Contributor

pbearne commented Jan 6, 2025

no, my mistake
I create branch here and then in my repo with the same name I delete the one here it seems to causing the issue

Create new pull request
I belive it will work

@westonruter westonruter moved this from Not Started/Backlog 📆 to Code Review 👀 in WP Performance 2024 Jan 7, 2025
@westonruter
Copy link
Member Author

Well, I tested it and now I'm getting the desired output:

{"minViewportAspectRatio":0.4,"maxViewportAspectRatio":2.5

But I'm now seeing the same on trunk as well, even without your change, which is frustrating. It's also not clear how round() would actually resolve the issue, if the original value being passed into the function is 0.4, since the output already has one digit of precision. So I would think that 0.4 would naturally be the same as round( 0.4, 2), unless PHP has some internal representation for floats that limits the precision, which I'm not aware of.

@ShyamGadde
Copy link
Contributor

ShyamGadde commented Jan 8, 2025

@westonruter I did some testing and found that this behavior seems to depend on the environment configuration. When using wp-env, I consistently got the desired output:

{"minViewportAspectRatio":0.4,"maxViewportAspectRatio":2.5}

However, I set up another site using LocalWP and noticed the output was different. After investigating, I discovered that the root cause is tied to the serialize_precision directive in PHP. LocalWP auto-generates a configuration where serialize_precision is explicitly set to 17, which leads to the excessive floating-point precision. In contrast, wp-env doesn’t override this value, leaving it at its default of -1 (as of PHP 7.1 and later).

According to the PHP documentation:

Prior to PHP 7.1.0, the default value was 17.
(Reference)

This means that environments running PHP 7.1 or later should not encounter this issue unless serialize_precision is explicitly overridden by the hosting configuration or user settings.

As for the use of round(), I don’t believe it addresses the underlying issue. The original problem stems from how wp_json_encode (which relies on PHP's json_encode) is influenced by the serialize_precision value in php.ini. The rounding feels unnecessary since the excessive precision wasn’t introduced by the code itself but rather by the environment settings.

Given this, it might not be necessary to take any specific action, as the default configuration (-1) works as intended. However, if this is a major concern, maybe we could explore adding a Site Health check to alert users about non-default serialize_precision values, but I’m not sure if this edge case justifies the additional complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Bug An existing feature is broken
Projects
Status: Code Review 👀
3 participants