Page MenuHomePhorge

Fix PHP 8.1 "implicit conversion from float to int" exception on certain avatar colors which blocks rendering user pages
ClosedPublic

Authored by aklapper on May 9 2023, 19:02.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jun 15, 12:59
Unknown Object (File)
Thu, Jun 13, 13:03
Unknown Object (File)
Wed, Jun 12, 16:07
Unknown Object (File)
Wed, Jun 12, 14:18
Unknown Object (File)
Tue, Jun 11, 23:00
Unknown Object (File)
Sun, Jun 9, 13:36
Unknown Object (File)
Thu, May 30, 11:36
Unknown Object (File)
Thu, May 30, 11:36

Details

Summary

Since PHP 8.1, conversion from float to integer should be explicit.

https://wiki.php.net/rfc/implicit-float-int-deprecate

According to phlog, the alpha channel value of the automatically created user
avatar image for a new user account sometimes is a float.

Thus always round() the alpha channel value to be an integer.

Closes T15375

Test Plan

Applied this change, created five user accounts via http://phorge.localhost/people/new/standard/ , and each resulting alpha value in PhabricatorFilesComposeAvatarBuiltinFile.php was an integer according to phlog (see corresponding Maniphest task). Rendering of each new user page always succeeded.

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

aklapper requested review of this revision.May 9 2023, 19:02

I DON'T BELIEVE THIS

You fixed this damn stuff

Let's try that immediately

Just to clarify, do you have an idea about the input domain of $a? (I will test this tomorrow)

Just to clarify, do you have an idea about the input domain of $a? (I will test this tomorrow)

Honestly: No. The array name implies that it's an RGBA color scheme. So I naïvely expected three integers in the range of 0 to 255, and the alpha value between 0 and 1. :P

I tried to write down the input domain:

$a (float)old: (1-$a)*255old implicit bitwise cast to intnew: round(((1 - $a) * 255), 0) (floats)new implicit bitwise cast to int
0float 255255float 255.0255
0.1float 229.5229 <deprecation warning>float 230.0230
0.11float 226.95226 <deprecation wraning>float 227.0227
0.2float 204204float 204.0204
0.5float 127.5127 <deprecation warning>float 128.0128
0.8float 5151float 51.051
0.9float 25.525 <deprecation warning>float 26.026
0.95float 12.7512 <deprecation warning>float 1313
1float 00.0float 0.00

I would say: thank you PHP 8.1 for introducing this deprecation warning, since now we are aware that we were lossing precision.

The proposed general approach from aklapper seems more precise than before to me. Thank you!

I accept this as behalf of myself to allow more people to share an opinion

src/applications/files/builtin/PhabricatorFilesComposeAvatarBuiltinFile.php
190

I tried to write down the input domain:

$a(1 - $a) * 255
0255
0.1229.5
0.2204
0.3178.5
0.4153
0.5127.5
0.6102
0.776.5
0.851
0.925.5
10
191

Note that the bitwise operator automatically casts to int. That is why the exception is raised, since we loss precision in some cases like 122.7.

https://www.php.net/manual/en/language.operators.bitwise.php

200

Personally I would suggest something like this in order to explicit the most optimal type for the consequent bitwise operator (that works with integers):

$a = (int) round(((1 - $a) * 255), 0);

In any case, according to this page:

https://www.php.net/manual/en/language.types.integer.php

The proposed use of round($a, 0) seems the minimal change necessary to obtain an "integral float" that could be converted to int without any loss of precision, and so without any deprecation warning.

✅ So green light from me

I double-checked this multiple times and I think the approach is really the minimal necessary to achieve this goal in a safe way.

Any follow-up patch is welcome to further improving the situation.

Hoping to be useful I will add some inline documentation from what we found.

In short, I would say, thanks and green light.

sgtm

src/applications/files/builtin/PhabricatorFilesComposeAvatarBuiltinFile.php
169

Interestingly here we are doing a totally-undocumented thing: the function imagerectangle() should only accept identifiers created from imagecolorallocatealpha() but here we don't use it but a value received from our self-made rgba2gd().

Just to clarify I like this patch but I don't like the already existing source and I'm sorry for that.

This revision is now accepted and ready to land.May 12 2023, 07:47

add minor inline documentation

Thanks for the additional checking and docs!