Summary: Use `PhutilClassMapQuery` where appropriate.
Test Plan: Browsed around the UI to verify things seemed somewhat working.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13429
Summary: Ref T8726. The existing implementation is largely made redundant by the newer modular implementation.
Test Plan: Created a custom field rule, set custom field to various values until it matched, saw effect and proper transcripts.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13497
Summary: Return `$this` from setter methods for consistency. I started writing a [[https://secure.phabricator.com/differential/diff/32506/ | linter rule]] to detect this, but I don't think it is trivial to do this properly.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13422
Summary: Move some `PhabricatorSearchField` subclasses to be adjacent to the application to which they belong. This seems generally better to me than lumping them all together in the `src/applications/search/field/` directory. I was also wondering if it makes sense to rename these subclasses as `PhabricatorXSearchField` rather than `PhabricatorSearchXField` (as per T5655), but wasn't really sure if these objects are meant to be search-fields, or just fields belonging to the #search application.
Test Plan: N/A.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13374
Summary: Ref T8750, Adds a maxwidth class for Graphviz images.
Test Plan: Generate a Graphviz image, really big, see it scale to the viewport.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8750
Differential Revision: https://secure.phabricator.com/D13548
Summary:
Fixes T8743. Fixes T8746. When running queries with the omnipotent viewer and no explicit space constraints, don't add implicit space constraints.
This prevents us from fataling when running older pre-space migrations and trying to load space-aware objects.
Test Plan: Manually ran migrations with `--trace`, verified no `WHERE spacePHID = ...`.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: eadler, epriestley
Maniphest Tasks: T8743, T8746
Differential Revision: https://secure.phabricator.com/D13542
Summary:
Ref T8731. I think the issue is that some `ProjectQuery` (without needImages()) populates the query workspace, then the "real" one hits the workspace.
Instead, only populate the workspace from ObjectQuery, so we know that objects in the workspace always have whatever ObjectQuery attaches to them.
Test Plan: Verified this didn't destroy the cache hitrate, but I can't repro the original issue locally per se.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T8731
Differential Revision: https://secure.phabricator.com/D13516
Summary: Ref T8099. Better alignment with TODO and Ghost Icons.
Test Plan: Test more states of inline comments, ghosts, todos, unsubmitted, etc.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13515
Summary:
Ref T8631. The query plan for feed stories is really bad right now, because we miss caches we should be hitting:
- The workspace cache is stored at each query, so adjacent queries can't benefit from the cache (only subqueries). Feed has primarily sibling queries.
- There is no technical reason to do this. Store the workspace cache on the root query, so sibling queries can hit it.
- In `ObjectQuery`, we check the workspace once, then load all the PHIDs. When the PHIDs are a mixture of transactions and objects, we always miss the workspace and load the objects twice.
- Instead, check the workspace after loading each type of object.
- `HandleQuery` does not set itself as the parent query for `ObjectQuery`, so handles never hit the workspace cache.
- Pass it, so they can hit the workspace cache.
- Feed's weird `PhabricatorFeedStory::loadAllFromRows()` method does not specify a parent query on its object/handle queries.
- Just declare the object query to be the "root" query until this eventually gets cleaned up.
Test Plan: Saw queries for each object drop from 4-6x to 1x in `/feed/`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8631
Differential Revision: https://secure.phabricator.com/D13479
Summary: Fixes T8723. We override timeline titles for these fields to shorten them, but the parent class shows full values for feed edits, which can lead to screen-sized notifications.
Test Plan: Edited a project, saw a reasonably-sized notification for it.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8723
Differential Revision: https://secure.phabricator.com/D13485
Summary: Ref T8099, Simplifies the button bar with a `borderless` option and implements in Differential Inline Commenting.
Test Plan:
Review new and old comments, submitted, unsubmitted, ghosts, done.
{F562765}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13475
Summary:
Ref T8637. This does nothing interesting, just has empty scaffolding for a bulk job queue.
Basic idea is that when you do something like a batch edit in Maniphest, we:
- Create a BulkJob with all the details.
- Queue a worker to start the job.
- Send you to a progress bar page for the job.
In the background:
- The "start job" worker creates a ton of Task objects, then queues worker tasks to do the work.
In the foreground:
- Fancy ajax animates the progress bar and it goes wooosh.
In general:
- Big jobs actually work.
- Jobs get logged.
- You can monitor jobs.
- Terrible junk like T8637 should be much harder to write and much easier to catch and diagnose.
Test Plan:
No interesting code/beahavior yet. Clean `storage adjust`.
{F526411}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8637
Differential Revision: https://secure.phabricator.com/D13392
Summary:
Ref T8096. Various tweaks here:
- Sort result lists by importance (even lint -- "errors first" seems better than "alphabetical by file", I think?).
- Do sane stuff with display limits.
- Add a "view all" view.
- Don't show a huge table of passing tests in Differential.
- Link to full results.
Test Plan: See screenshots.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13407
Summary: Fixes T8599. I'm not sure how to reproduce the original issue, but I'm fairly confident that the issue is that the issue is that `execute()` is not called on the query object.
Test Plan: Created a Harbormaster build plan with a single "Lease Host" step. Ran `./bin/harbormaster build --plan 1 D1` from the command line and hit the exception as described in T8599. Applied patch and hit a different exception (which I think is just because I don't know how to use #drydock and #harbormaster).
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: hach-que, epriestley, Korvin
Maniphest Tasks: T8599
Differential Revision: https://secure.phabricator.com/D13335
Summary:
Ref T8575. Because orderable columns need to build custom fields, they are relatively expensive to build. Use the request cache.
(The request cache is technically more correct than `static`, because configuration may change between requests and we may eventually reuse interpreters.)
Test Plan: Saw home page time drop 39% (from 462ms to 283ms).
Reviewers: btrahan, avivey
Reviewed By: avivey
Subscribers: avivey, epriestley
Maniphest Tasks: T8575
Differential Revision: https://secure.phabricator.com/D13322
Summary: Ref T8575. We check if users can see applications frequently, and caching on the Query isn't especially effective. Use the new Request cache instead.
Test Plan:
- Saw `/feed/` drop 7% (from ~830ms to ~770ms) on profiles.
Reviewers: btrahan, avivey
Reviewed By: avivey
Subscribers: avivey, epriestley
Maniphest Tasks: T8575
Differential Revision: https://secure.phabricator.com/D13321
Summary:
Ref T4558. This diff modernizes the #diviner application. Basically:
- Add an edit controller, accessible at `/book/$BOOK/edit/`.
- Add edit/view policies.
- Added an action menu to the `DivinerBookController` to expose the edit interface.
- Allows projects to be associated with books.
- Implement edges and transactions.
- Implemented `PhabricatorApplicationTransactionInterface` in `DivinerLiveBook`.
Test Plan:
- Generated a Diviner book with `./bin/diviner generate`.
- Added projects to a book and ensured that they persisted.
- Changed the view policy on a book and made sure it was effective.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4558
Differential Revision: https://secure.phabricator.com/D13091
Summary: All classes should extend from some other class. See D13275 for some explanation.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13283
Summary:
Ref T4345. This error is per object-type in the query implementations, not a mail/permissions issue.
Without `didRejectResult()`, we can't distinguish between "restricted" and "unknown" for objects filtered by `willFilterPage()`.
- Call `didRejectResult()` on commits.
- Make `didRejectResult()` handle both existing policy exceptions and filtering.
- Recover from partial objects (like commits) which are missing attached data required to figure out policies.
Test Plan: Saw "Restricted Diffusion Commit" instead of "Unknown Object (Diffusion Commit)" when viewing nonvisible commit handle in Maniphest.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4345
Differential Revision: https://secure.phabricator.com/D13289
Summary:
Ref T8377. This adds a standard disable/enable feature to Spaces, with a couple of twists:
- You can't create new stuff in an archived space, and you can't move stuff into an archived space.
- We don't show results from an archived space by default in ApplicationSearch queries. You can still find these objects if you explicitly search for "Spaces: <the archived space>".
So this is a "put it in a box in the attic" sort of operation, but that seems fairly nice/reasonable.
Test Plan:
- Archived and activated spaces.
- Used ApplicationSearch, which omitted archived objects by default but allowed searches for them, specifically, to succeed.
- Tried to create objects into an archived space (this is not allowed).
- Edited objects in an archived space (this is OK).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8377
Differential Revision: https://secure.phabricator.com/D13238
Summary: Ref T8441. Ref T7715. Ref T7909. Clean up all the ordering and grouping hacks in Maniphest so we can drive it through normal infrastructure, move it to SearchField, introduce Spaces, and eventually modernize the Conduit API.
Test Plan:
- Executed all grouping/ordering queries, including custom queries.
- Forced execution with old aliases; got modern results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7909, T7715, T8441
Differential Revision: https://secure.phabricator.com/D13197
Summary:
Ref T8441. Ref T7715. Automatically generate a modern "Order" control in ApplicationSearch for engines which fully support SearchField.
Notably, this allows the standard "Order" control to automatically support custom field orders. We do this in Maniphest today, but in an ad-hoc way.
Test Plan: Performed order-by queries in Almanac (Services), Pholio, Files, People, Projects, and Paste.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7715, T8441
Differential Revision: https://secure.phabricator.com/D13193
Summary:
Ref T8441. Ref T7715. For modern Query classes, automatically make subscriber queries and SearchField integrations work.
In particular, we can just drive this query with EdgeLogic and don't need to do anything specific on these Query classes beyond making sure they're implemented in a way that picks up all of the EdgeLogic clauses.
Test Plan:
- Searched for subscribers in Pholio, Files, Paste, and Projects.
- Searched for all other fields in Projects to check that Query changes are OK.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7715, T8441
Differential Revision: https://secure.phabricator.com/D13191
Summary:
Ref T8441. Ref T7715.
- These are obsolete after the Viewer/HandlePool changes.
- These are unused after the typeahead parameterization changes.
Test Plan: `grep`, poked around.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7715, T8441
Differential Revision: https://secure.phabricator.com/D13176
Summary: I saw these appear untranslated in the feed on https://secure.phabricator.com.
Test Plan: Created a dependency and checked the story text in `/feed/`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13149
Summary:
Ref T8424. No UI or interesting behavior yet, but integrates Spaces checks:
- `PolicyFilter` now checks Spaces.
- `PolicyAwareQuery` now automatically adds Spaces constraints.
There's one interesting design decision here: **spaces are stronger than automatic capabilities**. That means that you can't see a task in a space you don't have permission to access, //even if you are the owner//.
I //think// this is desirable. Particularly, we need to do this in order to exclude objects at the query level, which potentially makes policy filtering for spaces hugely more efficient. I also like Spaces being very strong, conceptually.
It's possible that we might want to change this; this would reduce our access to optimizations but might be a little friendlier or make more sense to users later on.
For now, at least, I'm pursuing the more aggressive line. If we stick with this, we probably need to make some additional UI affordances (e.g., show when an owner can't see a task).
This also means that you get a hard 404 instead of a policy exception when you try to access something in a space you can't see. I'd slightly prefer to show you a policy exception instead, but think this is generally a reasonable tradeoff to get the high-performance filtering at the Query layer.
Test Plan:
- Added and executed unit tests.
- Put objects in spaces and viewed them with multiple users.
- Made the default space visible/invisible, viewed objects.
- Checked the services panel and saw `spacePHID` constraints.
- Verified that this adds only one query to each page.
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T8424
Differential Revision: https://secure.phabricator.com/D13156
Summary:
Ref T8424. I'm using Paste as a testbed application because Spaces make some degree of sense for it but it's also flat/simple.
This doesn't do anything interesting or useful and mostly just making the next (more interesting) diff smaller.
Test Plan:
- Ran `bin/storage upgrade -f`.
- Browsed pastes.
- Created a paste.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8424
Differential Revision: https://secure.phabricator.com/D13154
Summary:
Ref T8424. This adds a standard KeyValueCache to serve as a request cache.
In particular, I need to cache Spaces (they are frequently accessed, sometimes by multiple viewers) but not have them survive longer than the scope of one request.
This request cache is explicitly destroyed by each web request and each daemon request.
In the very long term, building this kind of construct supports reusing PHP interpreters to run web requests (see some discussion in T2312).
Test Plan:
- Added and executed unit tests.
- Ran every daemon.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8424
Differential Revision: https://secure.phabricator.com/D13153
