Page MenuHomePhorge

Allow setting default value for SearchFields; set Maniphest Page Size to 100
Changes PlannedPublic

Authored by aklapper on Jan 17 2024, 14:46.
Tags
None
Referenced Files
F2310933: D25518.1721426241.diff
Thu, Jul 18, 21:57
F2310890: D25518.1721425449.diff
Thu, Jul 18, 21:44
F2310848: D25518.1721424570.diff
Thu, Jul 18, 21:29
Unknown Object (File)
Wed, Jul 17, 23:27
Unknown Object (File)
Sat, Jul 13, 22:57
Unknown Object (File)
Fri, Jul 12, 20:23
Unknown Object (File)
Fri, Jul 12, 11:35
Unknown Object (File)
Wed, Jul 10, 00:51

Details

Summary

On the Advanced Search page for Maniphest at /maniphest/query/advanced/, the "Page Size" field is empty.
Thus it does not expose that by default Maniphest queries via the web UI are limited to only 100 results - 100 is the "limit" value defined in the parent class src/applications/search/engine/PhabricatorApplicationSearchEngine.php via getPageSize().
This leads to users confused about missing results.

Historically, the 100 value for the "Page Size" in ManiphestTaskSearchEngine.php was introduced in rPa82c3a89144eba7c41ade2b1db01260cd92273f0 (and at that time exposed in the UI) and removed in rPb8b4279a8ad2351a9d8ae5367ba2b4174cb3d1d1#change-DofiWYgC7zEt when introducing SearchField classes.

Thus introduce a setDefaultValue() function for SearchFields.
Then make the "Page Size" field definition in the Maniphest Query UI use it to set 100.

This change does not affect the Conduit API because setPagerLimitForConduit() in src/applications/search/engine/PhabricatorApplicationSearchEngine.php remains unchanged.

Closes T15676

Test Plan
  • Go to /maniphest/query/advanced/ and see that the Page Size field now displays 100 (which allows users to realize the default query behavior).
  • Run random Maniphest queries, change parameters and the Page Size field.
  • Remove the 100 value from the Page Size field and run an Advanced Maniphest query: Still max 100 results displayed.

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D25518
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1363
Build 1363: arc lint + arc unit

Event Timeline

This patch is a cleaner approach to tackle T15676 than D25479 was.
In future steps,

speck added inline comments.
src/applications/maniphest/query/ManiphestTaskSearchEngine.php
146

Maybe we should also add a description to clarify that 100 is the maximum. Any value over 100 will… be clamped to 100 presumably? Or does an error get thrown?

src/applications/maniphest/query/ManiphestTaskSearchEngine.php
146

100 is _not_ the maximum when using the web UI; 100 is only the default (but the web UI does not make that clear without this patch). See also the corresponding comments in T15676 for use cases etc.

src/applications/maniphest/query/ManiphestTaskSearchEngine.php
146

Okay, I misunderstood the term “limit”

src/applications/search/field/PhabricatorSearchTextField.php
6–9 ↗(On Diff #1641)

Should this class still somehow set the default value to empty string instead null?

src/applications/search/field/PhabricatorSearchTextField.php
6–9 ↗(On Diff #1641)

Doing that would collide with our ->setDefaultValue('100') as return an empty string here would overwrite our "100" with that empty string again in the "Page Size" field on http://phorge.localhost/maniphest/query/advanced/

20after4 added inline comments.
src/applications/maniphest/query/ManiphestTaskSearchEngine.php
146

Is there any actual limit or could someone presumably set the page size to 10000 and cause a minor DOS on the server? I say minor because the max execution time should kill the request after 30 seconds, however, that's still significant load on the database and web server.

src/applications/maniphest/query/ManiphestTaskSearchEngine.php
146

I do not know, and the change proposed in this ticket does not change the situation anyway - what you describe could also already be done without this code change.

This revision is now accepted and ready to land.Sat, Jun 22, 03:26

Current patch does not work anymore since merging rPfbe07fbeefcadea3abdb4dc3e4d4558c2b91ada9; need to remove getDefaultValue() from PhabricatorSearchIntField instead of PhabricatorSearchTextField now

We pushed rPfbe07fbeefcadea3abdb4dc3e4d4558c2b91ada9 in the meantime so no more String but Int field

Make Celerity happy (I hope)

Meh, after removing the value from the "Page Size" field on phorge.localhost/maniphest/query/advanced/, field entry will incorrectly display "0" but list 100 results. Need to find out how to keep the value displayed/reset after manually removing it to avoid PHP converting an empty integer to a 0.

Plus I don't enjoy the resources/celerity/map.php item in here b/c of D25631