Paths

Table of Contentst

Differential D25502

Make PhabricatorSearchIntField validate its input
Needs ReviewPublic
Actions

Authored by aklapper on Jan 2 2024, 11:51.

Details

Reviewers

None

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15700: An "Integer" search field

Summary

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

Make sure that rPfbe07fbeefcadea3abdb4dc3e4d4558c2b91ada9 is included
Go to /maniphest/query/advanced/ and enter example values into the Page Size field and click the Search button - get Integer value for "Page Size" can not be parsed. now for invalid example values.
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).
API call /conduit/method/project.query/ does not offer passing depth values to test.
API call for /conduit/method/maniphest.search/ and entering a non-integer value as limit triggers an error but this code path does not call PhabricatorSearchIntField.php at all, so this is unrelated and a separate issue.

Diff Detail

Repository

rP Phorge

Branch

T15700checkInt (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 998
Build 998: arc lint + arc unit

Event Timeline

aklapper created this revision.Jan 2 2024, 11:51

Owners added a reviewer: O1: Blessed Committers.Jan 2 2024, 11:51

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptJan 2 2024, 11:51

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

Harbormaster completed remote builds in B997: Diff 1605.Jan 2 2024, 11:51

Replace minus dash by its UTF8 representation to pass lint

Harbormaster completed remote builds in B998: Diff 1606.Jan 2 2024, 11:58

aklapper edited the test plan for this revision. (Show Details)Jan 2 2024, 12:00

aklapper mentioned this in T15723: Convert "Page Size" field in ManiphestTaskSearchEngine.php to PhabricatorSearchIntField.Jan 26 2024, 10:55

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?

src/applications/search/field/PhabricatorSearchIntField.php
17	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 https://we.phorge.it/source/phorge/browse/master/src/applications/search/field/PhabricatorSearchDateField.php here.

Remove unreached return call as pointed out by speck

Harbormaster completed remote builds in B1062: Diff 1700.Feb 13 2024, 15:58

aklapper marked an inline comment as done.Mar 13 2024, 14:32

Would anyone give this another review? Might make sense to apply the one-liner in D25527 to have a simple test case on the Maniphest Advanced Search page

I'm pretty sure getValueForQuery() should return some value?

Maybe adding the use-case would be good.

src/applications/search/field/PhabricatorSearchIntField.php
24–30	Can't this just be `is_numeric()` ?

aklapper added inline comments.May 3 2024, 15:35

src/applications/search/field/PhabricatorSearchIntField.php
24–30	@avivey: Makes a difference for float values: `is_numeric((string)$value)` allows entering e.g. the value `13.7` (resulting in 13 task results). Current `ctype_digit((string)$value)` throws `Query Errors - Integer value for "Page Size" can not be parsed.` Question is how lenient we want to be?

aklapper edited the test plan for this revision. (Show Details)May 3 2024, 15:46

Add return value (though it seems to make no difference)

Harbormaster completed remote builds in B1216: Diff 1928.May 3 2024, 15:48

valerio.bozzolan added inline comments.Jul 12 2024, 10:07

src/applications/search/field/PhabricatorSearchIntField.php
24	Stupid question: In any case, I don't understand here, how is this able to store negative numbers, if we strip the sign. Thanks for the clarification
26	I don't know what is `\xe2\x88\x92` but `substr("\xe2\x88\x92123", 1)` will not do what we want here. Evaluate `str_replace()` instead. E.g. this, even without `if`: // Handle negative number strings by stripping the minus $value = str_replace(array("\xe2\x88\x92", '-'), '', "\xe2\x88\x921238768");
28	This utility gives better error messages with incoming aliens

valerio.bozzolan added inline comments.Jul 12 2024, 10:10

src/applications/search/field/PhabricatorSearchIntField.php
28	Edited: if a cast is ever needed, in any way, we should probably cast this earlier, since the whole method seems to assume to receive wild things and work on strings internally. I still do not understand if this method is supposed to return a string or the original type, anyway.

Revision Contents
Changeset List

Path

Size

Packages

src/

applications/

search/

field/

PhabricatorSearchIntField.php

26 lines

O1: Blessed Committers

Diff 1606

View Options

src/applications/search/field/PhabricatorSearchIntField.php

Make PhabricatorSearchIntField validate its inputNeeds ReviewPublicActions