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

Branch

createUserPageIntFloat (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 386
Build 386: arc lint + arc unit

Event Timeline

aklapper created this revision.May 9 2023, 19:02

Owners added a reviewer: O1: Blessed Committers.May 9 2023, 19:02

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptMay 9 2023, 19:02

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

Harbormaster completed remote builds in B386: Diff 703.May 9 2023, 19:03

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)

In D25209#6204, @valerio.bozzolan wrote:

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

valerio.bozzolan edited the summary of this revision. (Show Details)May 10 2023, 06:47

I tried to write down the input domain:

$a (float)	old: (1-$a)*255	old implicit bitwise cast to int	new: round(((1 - $a) * 255), 0) (floats)	new implicit bitwise cast to int
0	float 255	255	float 255.0	255
0.1	float 229.5	229 <deprecation warning>	float 230.0	230
0.11	float 226.95	226 <deprecation wraning>	float 227.0	227
0.2	float 204	204	float 204.0	204
0.5	float 127.5	127 <deprecation warning>	float 128.0	128
0.8	float 51	51	float 51.0	51
0.9	float 25.5	25 <deprecation warning>	float 26.0	26
0.95	float 12.75	12 <deprecation warning>	float 13	13
1	float 0	0.0	float 0.0	0

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
0	255
0.1	229.5
0.2	204
0.3	178.5
0.4	153
0.5	127.5
0.6	102
0.7	76.5
0.8	51
0.9	25.5
1	0

191

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

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

valerio.bozzolan mentioned this in D25213: Fix PHP 8.1 "strlen(null)" exception[s] which block rendering "Browse Projects" overlay dialog.May 11 2023, 17:00

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.