Paths

Table of Contentst

Differential D25500

Add getQueryApplicationClass() to *TransactionQuery.php classes
ClosedPublic
Actions

Authored by aklapper on Dec 25 2023, 20:29.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15642: Feed Transaction Logs: Exception: Query overheated: examined more than 1,010 raw rows without finding 101 visible objects.

Commits

rP064424c1b755: Add getQueryApplicationClass() to *TransactionQuery.php classes

Summary

Add getQueryApplicationClass() to all *TransactionQuery.php classes similar to other *Query.php classes having the same function, and make the parent function in PhabricatorApplicationTransactionQuery.php abstract.

In the future, this will enable excluding transaction query results based on their underlying application (for example if an application has been uninstalled) to mitigate the problem of overheated search results. See https://we.phorge.it/T15642 for context.

The only callers of getQueryApplicationClass() are in src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php and src/applications/policy/__tests__/PhabricatorPolicyTestCase.php.

See T15642

Test Plan

Patch changes only one existing code place, thus check if related pages still work as expected:

Go to http://phorge.localhost/feed/
Go to http://phorge.localhost/feed/transactions/
On http://phorge.localhost/feed/transactions/ , click Edit Query and set Object Types to Application etc.

Diff Detail

Repository

rP Phorge

Branch

T15642transactionQueryClassesAppName (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 1041
Build 1041: arc lint + arc unit

Event Timeline

aklapper created this revision.Dec 25 2023, 20:29

Owners added a reviewer: O1: Blessed Committers.Dec 25 2023, 20:29

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptDec 25 2023, 20:29

aklapper requested review of this revision.Dec 25 2023, 20:29

Harbormaster completed remote builds in B994: Diff 1601.Dec 25 2023, 20:29

aklapper added inline comments.Dec 25 2023, 20:31

src/applications/meta/query/PhabricatorApplicationApplicationTransactionQuery.php
11	Must return `null` otherwise pages like http://phorge.localhost/applications/view/PhabricatorSomethingApplication/ will raise an exception `No application 'Foo'!`.

Document abstract function that it can return string|null

Harbormaster completed remote builds in B995: Diff 1602.Dec 25 2023, 21:08

aklapper mentioned this in D25501: Feed: Do not query and display data of uninstalled applications.Dec 25 2023, 21:20

Thanks! some quick tips maybe relevant:

Maybe we can return ClassName::class - that is supported since PHP 5.5
- among other things, the ::class also better integrates with an IDE to find class usages
- I see that ::class is already in-use in Phorge here and there
I've seen the comment https://we.phorge.it/D25500#inline-3509. If I understand correctly, we changed PhabricatorApplicationTransactionQuery#getQueryApplicationClass() so, the IDE helped to find classes to touch. Since we already done that, maybe better to restore, so we can omit methods that just return null

src/infrastructure/daemon/workers/query/PhabricatorWorkerBulkJobTransactionQuery.php
11	Example change

lgtm

Thanks. Don't take my comments seriously. Probably they have no sense.

This revision is now accepted and ready to land.Jan 8 2024, 13:24

Revert turning getQueryApplicationClass() abstract in the parent class src/applications/transactions/query/PhabricatorApplicationTransactionQuery.php and make it return null again (and thus remove the only newly added call which returned null in src/applications/meta/query/PhabricatorApplicationApplicationTransactionQuery.php); also return ::class as proposed by Valerio

Harbormaster completed remote builds in B1039: Diff 1659.Jan 22 2024, 23:13

Maybe we can return ClassName::class - that is supported since PHP 5.5

Note to myself: Makes sense; there's a lot more code to update in random *Query.php classes defining getQueryApplicationClass() functions, in a separate future patch

double-accept

aklapper mentioned this in D25524: Make misc get*ApplicationClass[Name]() calls return ::class constant instead of string.Jan 23 2024, 13:36

In D25500#15027, @aklapper wrote:

Maybe we can return ClassName::class - that is supported since PHP 5.5

there's a lot more code to update in random *Query.php classes defining getQueryApplicationClass() functions, in a separate future patch

Done in D25524

In D25500#15024, @aklapper wrote:

and thus remove the only newly added call which returned null in src/applications/meta/query/PhabricatorApplicationApplicationTransactionQuery.php)

Put that one file back in, as it's clearer to explicitly make that one case return null. See https://we.phorge.it/source/phorge/browse/master/src/applications/meta/query/PhabricatorApplicationQuery.php$165 whose explanation I copied.

Harbormaster completed remote builds in B1041: Diff 1661.Jan 23 2024, 13:42

Closed by commit rP064424c1b755: Add getQueryApplicationClass() to *TransactionQuery.php classes. · Explain WhyJan 28 2024, 16:10

This revision was automatically updated to reflect the committed changes.

aklapper added a commit: rP064424c1b755: Add getQueryApplicationClass() to *TransactionQuery.php classes.

aklapper mentioned this in rP25aebab655fd: Make misc get*ApplicationClass[Name]() calls return ::class constant instead of….Feb 5 2024, 13:11