Page MenuHomePhorge

Make PhabricatorSearchIntField validate its input
Needs ReviewPublic

Authored by aklapper on Jan 2 2024, 11:51.
Referenced Files
Unknown Object (File)
Mon, Feb 19, 13:32
Unknown Object (File)
Mon, Feb 19, 07:31
Unknown Object (File)
Sat, Feb 17, 00:21
Unknown Object (File)
Mon, Feb 12, 04:00
Unknown Object (File)
Sun, Feb 11, 21:50
Unknown Object (File)
Wed, Jan 31, 16:47
Unknown Object (File)
Tue, Jan 30, 14:47
Unknown Object (File)
Tue, Jan 30, 14:14


Group Reviewers
O1: Blessed Committers
Maniphest Tasks
T15700: An "Integer" search field

Allow only positive and negative integer values as input in a PhabricatorSearchIntField.

Use getStr() instead of getInt() to allow differentiation in validateControlValue() between a literal value 0 versus 0 only returned by PHP casting a random string value to an int.
The validateControlValue() function does not use is_numeric($value) as this would also accept float values, and does not use is_int($value) as that is impossible due to the previous getStr().
Generally, the code is similar to PhabricatorSearchDateField.php.

See T15700

Test Plan

In /src/applications/project/query/PhabricatorProjectSearchEngine.php, change the two only uses of PhabricatorSearchIntField to ->setIsHidden(false). Then go to /project/query/advanced/ and enter example values into the Maximum Depth and Maximum Depth fields and click the Search button.
Example values: 13, 0, -13, foobar, foo13bar, -foo1313, 13.5, -13.5, 13,5, -13,5, -, -0 (the latter is still accepted as 0).

It seems that there are no API calls to test - /conduit/method/project.query/ does not offer passing depth values.

Diff Detail

rP Phorge
T15700checkInt (branched from master)
Lint Passed
Tests Passed
Build Status
Buildable 1062
Build 1062: arc lint + arc unit

Event Timeline

aklapper requested review of this revision.Jan 2 2024, 11:51

Replace minus dash by its UTF8 representation to pass lint

Would anyone fancy reviewing this? :)

For the records, adding public function getValueForQuery($value) is needed because otherwise Expected a numeric scalar or null for %d conversion. Query: project.projectDepth >= %d

Is it possible to check if anything is relying on this not working with negative numbers?


None of the returns in validateControlValue return anything.

In D25502#15384, @speck wrote:

Is it possible to check if anything is relying on this not working with negative numbers?

In the current codebase, there are only two uses of PhabricatorSearchIntField in /src/applications/project/query/PhabricatorProjectSearchEngine.php and both make no sense for negative integers. Same for my intended third use in T15723.
If there was a future use case to allow negative integers, this check code be adjusted accordingly.

None of the returns in validateControlValue return anything.

Eh, good catch, thank you! I should remove the very last return, indeed.

It is supposed to fail only if "wrong" data is found by reaching the addError line, otherwise the check passes (via returning nothing). I basically copied pre-existing here.

Remove unreached return call as pointed out by speck