Severity	Location	Code	Message
Warning	support/startup/__tests__/PreambleUtilsTestCase.php:106	XHP132	Partial Catch
Advice	support/startup/__tests__/PreambleUtilsTestCase.php:85	XHP55	Unnecessary Final Modifier

Unit

No Test Coverage

Build Status

Buildable 241
Build 241: arc lint + arc unit

Event Timeline

avivey created this revision.Apr 7 2023, 10:25

Owners added a reviewer: O1: Blessed Committers.Apr 7 2023, 10:25

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptApr 7 2023, 10:25

avivey requested review of this revision.Apr 7 2023, 10:25

Harbormaster completed remote builds in B231: Diff 465.Apr 7 2023, 10:25

avivey edited the summary of this revision. (Show Details)Apr 7 2023, 10:25

avivey added a reviewer: valerio.bozzolan.

avivey added a task: T15064: Make Phorge compatible with PHP 8.1/8.2/8.3.

valerio.bozzolan added inline comments.Apr 7 2023, 13:33

support/startup/preamble-utils.php
24	I suggest a strict check there. I also strongly would like to push this that is what we exactly want, so, exclude NULL, have a string, and have it non-empty: if(is_string($forwarded_for) && $forwarded_for !== '') {

valerio.bozzolan edited the summary of this revision. (Show Details)Apr 7 2023, 13:36

valerio.bozzolan added inline comments.Apr 7 2023, 13:39

support/startup/preamble-utils.php
24	Ouch note that both the patch and my suggestions are incorrect. The correct way is the total reverse, so: if ($forwarded_for === null \|\| !strlen($forwarded_for)) { Or, what I would like to use: if(!is_string($forwarded_for) \|\| $forwarded_for === '') {

valerio.bozzolan requested changes to this revision.Apr 7 2023, 13:39

This revision now requires changes to proceed.Apr 7 2023, 13:39

Hi @avivey thanks for this fix in my regression. Feel free to update this to just negate the if and make it correct, or here there is a quick fix ready to be accepted:

D25119: Fix regression in preamble

(I've done that just in order to be nice and don't bother you with additional fatigue for my regression. So, if you have time just continue here, or move to D25119)

Since the bug you found was major, and my tests didn't catch it, I'm going to do another round of more in-depth testing for this...

avivey added inline comments.Apr 8 2023, 08:01

support/startup/preamble-utils.php
24	To reduce count of double-negatives, let's add `phutil_is_empty_string()` function.

avivey added inline comments.Apr 8 2023, 08:03

support/startup/preamble-utils.php
24	actually, it might end up more confusing with `NULL` inputs.

avivey mentioned this in 2023 Week 2.Apr 9 2023, 12:25

add tests and keep simplest code

Harbormaster completed remote builds in B241: Diff 477.Apr 9 2023, 14:36

Added a test-script to check all possible inputs. It's not run automatically, because this isn't part of the main code.

Anyway, turns out that $forwarded_for in line 23 is never null, because "isset() will return false when checking a variable that has been assigned to null." (https://www.php.net/manual/en/function.isset); so strlen is fine here.

avivey edited the test plan for this revision. (Show Details)Apr 9 2023, 14:39

catch Throwable

Harbormaster completed remote builds in B242: Diff 478.Apr 9 2023, 18:11

Thank you for this fix and for the tests

I still recommend this:

if(!is_string($forwarded_for) || $forwarded_for === '') {

In order to exclude non-strings, non empty strings, and with O(1) complexity instead of O(N) or something depending on the length of the string, so that we minimize CPU cycles and Greta Thunberg goes like "wooooow Phorge"