Paths

Table of Contentst

Differential D25518

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

Authored by aklapper on Jan 17 2024, 14:46.

Details

Reviewers

speck

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15676: Empty "Page Size" field in Maniphest Advanced Search should display "100" by default

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

Test Failures

Build Status

Buildable 1362
Build 1362: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	94 ms	PhabricatorCelerityTestCase::testCelerityMaps Assertion failed, expected 'true' (at PhabricatorCelerityTestCase.php:27): When this test fails, it means the Celerity resource map is out of date. Run `bin/celerity map` to rebuild it. ACTUAL VALUE
	14 ms	PhabricatorConduitTestCase::testConduitMethods 1 assertion passed.
	0 ms	PhabricatorInfrastructureTestCase::testApplicationsInstalled 1 assertion passed.
	0 ms	PhabricatorInfrastructureTestCase::testRejectMySQLNonUTF8Queries 1 assertion passed.
	1,334 ms	PhabricatorLibraryTestCase::testEverythingImplemented 1 assertion passed.
		View Full Test Results (1 Failed · 6 Passed)

Event Timeline

aklapper created this revision.Jan 17 2024, 14:46

Owners added a reviewer: O1: Blessed Committers.Jan 17 2024, 14:46

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptJan 17 2024, 14:46

aklapper requested review of this revision.Jan 17 2024, 14:46

Harbormaster completed remote builds in B1024: Diff 1641.Jan 17 2024, 14:47

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

T15700: An "Integer" search field could be addressed (patch available for review),
the type of the "Page Size" field in ManiphestTaskSearchEngine.php could be switched from PhabricatorSearchTextField to PhabricatorIntTextField - see T15723: Convert "Page Size" field in ManiphestTaskSearchEngine.php to PhabricatorSearchIntField,
an AphrontFormIntControl class could be introduced to set type="number" for the control in the HTML,
we could introduce an overwrite validateControlValue() function in PhabricatorSearchTextField.php as in theory, future code could accidentally pass invalid data types to setDefaultValue() - see T15714: Validate PhabricatorSearchTextField value

aklapper mentioned this in T15714: Validate PhabricatorSearchTextField value.Jan 17 2024, 15:03

aklapper mentioned this in D25519: Validate PhabricatorSearchTextField value not to be an array.Jan 17 2024, 15:06

speck subscribed.Jan 17 2024, 17:37

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?

aklapper added inline comments.Jan 17 2024, 18:46

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.

speck added inline comments.Jan 18 2024, 01:24

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?

aklapper added inline comments.Jan 23 2024, 14:03

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 subscribed.Jan 23 2024, 22:23

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.

aklapper added inline comments.Jan 24 2024, 09:08

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.

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

aklapper mentioned this in rPe3714c86a7d5: Validate PhabricatorSearchTextField value not to be an array.Feb 5 2024, 21:42

seems good

This revision is now accepted and ready to land.Jun 22 2024, 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

Harbormaster completed remote builds in B1362: Diff 2137.Jun 22 2024, 21:07

Make Celerity happy (I hope)

Harbormaster completed remote builds in B1363: Diff 2138.Jun 22 2024, 21:08

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