Summary: Ref T13513. All queries now go through a reasonably minimal set of pathways and should have consistent behavior.
Test Plan:
- Loaded a revision with inlines.
- Created a new empty inline, reloaded page, saw it vanish.
- Created a new empty inline, typed draft text, did not save, reloaded page, saw draft present.
- Created a new empty inline, typed draft text. Submitted feedback, got prompt, answered "Y", saw draft text submit.
- Created a new empty inline, typed draft text, scrolled down to bottom of page, typed non-draft text, saw preview include draft text.
- Marked and submitted "Done".
- Used hide/show on inlines, verified state persisted.
- Did much of the same stuff in Diffusion, where it all works the same way (except: there's no prompt when submitting draft is-editing inlines).
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21234
Summary: Ref T13513. Replaces "DifferentialInlineCommentQuery" with the similar but more modern "DifferentialDiffInlineCommentQuery".
Test Plan: Viewed comments in timeline, changesets. Created, edited, and submitted comments. Hid and un-hid comments, reloading (saw state preserved).
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21233
Summary: Ref T13513. Continue removing usage sites for the obsolete "DifferentialInlineCommentQuery".
Test Plan: Viewed the inline list in Differential, saw sensible inlines.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21232
Summary: Ref T13513. Move querying to "DiffInlineCommentQuery" classes and lift them into the base Controller.
Test Plan: In Differential and Diffusion, created, edited, and submitted inline comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21231
Summary: Ref T13513. Continue marching toward coherent query pathways for all access to inline comments.
Test Plan:
- Viewed a commit and a path within that commit, as a user with unpublished inlines and a different user.
- Saw appropriate inlines in all cases (published inlines, plus undeleted unpublished inlines authored by the current viewer).
- Grepped for "loadDraftAndPublishedComments()".
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21228
Summary:
Ref T13513. Improve consistency and robustness of the "InlineComment" queries.
The only real change here is that these queries now implicitly add a clause for selecting inlines ("pathID IS NULL" or "changesetID IS NULL").
Test Plan: Browed, created, edited, and submitted inlines.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21227
Summary:
Ref T13513. Currently, inline storage objects ("TransactionComment") can't directly generate a runtime object ("InlineComment").
Allow this transformation to be performed in a genric way so clunky code which does it per-object-type can be removed, lifted, or simplified.
Simplify an especially gross callsite in preview code.
Test Plan: Previewed inline comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21226
Summary: Ref T13513. This controller was obsoleted by EditEngine and appears unreachable without explicitly typing the URL.
Test Plan:
- Grepped for the route, didn't find any hits.
- Deleted the controller, successfully previewed comments in Diffusion.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21224
Summary:
Ref T13519. This is a little fuzzy, but I think the workflow here is:
- View an intradiff, generating an ephemeral comparison changeset with no changeset ID. This produces a state key of "*".
- Apply "hidden" state changes to the changeset.
- View some other intradiff and/or diff view.
- The code attempts to use "*" as a changset ID?
I'm not entirely sure this is accurate; this was observed in production and I couldn't get a clean reproduction case locally.
Optimistically, try making changeset IDs explicit rather than relying on state keys to be "usually changeset-ID-like".
Test Plan: Used "hidden" locally across multiple intradiffs, but I wasn't cleanly able to reproduce the initial issue.
Maniphest Tasks: T13519
Differential Revision: https://secure.phabricator.com/D21223
Summary:
Ref T13513. Overloading "original text" to get "edit-on-load" comments into the right state has some undesirable side effects.
Instead, provide the text when the editor opens. This fixes a cancel interaction.
Test Plan:
- Create an inline, type text, don't save.
- Reload page.
- Cancel.
- Before: cancelled into empty state.
- After: cancelled into deleted+undo state.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21219
Summary:
Ref T13513. As users type text into inline comments, save the comment state as a draft on the server.
This has some rough edges, particularly around previews, but mostly works. See T13513 for notes.
Test Plan: Started an inline, typed some text, waited a second, reloaded the page, saw an editing inline with the saved text.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21216
Summary: Ref T13513. Refine some inline behaviors, see test plan.
Test Plan:
- Edit a comment ("A"), type text ("AB"), cancel, edit.
- Old behavior: edit and undo states (wrong, and undo does not function).
- New behavior: edit state only.
- Edit a comment ("A"), type text ("AB"), cancel. Undo ("AB"), cancel. Edit.
- Old behavior: "AB" (wrong: you never submitted this text).
- New behavior: "A".
- Create a comment, type text, cancel.
- Old behavior: counter appears in filetree (wrong, comment is undo-able but should not be counted).
- New behavior: no counter.
- Cancel editing an empty comment with no text.
- Old behavior: Something buggy -- undo, I think?
- New behavior: it just vanishes (correct behavior).
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21212
Summary: Ref T13513. This slightly expands the existing-but-hacky "warning" workflow to cover both "mentions on draft" and "submitting inlines being edited".
Test Plan:
- Submitted changes to a revision with mentions on a draft, inlines being edited, both, and neither.
- Got sensible warnings in the cases where warnings were appropriate.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21191
Summary:
Ref T13513. Now that the "currently being edited" state of inlines is saved on the server side, clear the flag when the user clicks "Cancel" to leave the "editing" state on the client.
This also serves to delete empty comments.
Test Plan:
- Clicked a line number to create a new comment. Then:
- Clicked "Cancel". Reloaded page, saw no more comment.
- Typed text, saved. Reloaded page, saw non-editing draft. Clicked "Edit", reloaded page, saw editing draft. Clicked "Cancel", reloaded page, saw non-editing draft.
- Typed text, saved. Clicked "Edit", deleted text, saved. Reloaded page, saw no more comment.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21187
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
Summary: Fixes T13530. The block parser could match too many lines in an unterminated "%%%" literal block. Adjust the logic to stop doing this (and hopefully be a little easier to read).
Test Plan: Added a failing test, made it pass.
Maniphest Tasks: T13530
Differential Revision: https://secure.phabricator.com/D21208
Summary:
Ref T13520. Generally, make the table of contents look and more like the paths panel:
- Show a hierarchy, with compression for single-sibling children.
- Use the same icons, instead of "M D" and "(img)" stuff.
- Use EditDistanceMatrix to do a piece-by-piece diff of paths changes.
- Show path changes within the path list.
I'm not entirely sold on this, but it was complicated to write and I've never heard the term "sunk cost fallacy". I think this is mostly a net improvement, but may need some adjustments and followup.
Test Plan: Viewed various changes in Differential and Diffusion, saw a more usable table of contents.
Maniphest Tasks: T13520
Differential Revision: https://secure.phabricator.com/D21183
Summary:
Fixes T13525. Since D21044, the intermittent GC list warnings are treated more severely and can become user-visible errors.
Silence them, since this seems to be the only realistic response in most versions of APC/APCu.
Test Plan: Will deploy.
Maniphest Tasks: T13525
Differential Revision: https://secure.phabricator.com/D21179
Summary:
Ref T13455. Make "hidden" a changeset property similar to other changeset properties.
We don't need to render this on the server, so we make a request (to update the setting) and just discard the response.
Test Plan: {F7375468}
Maniphest Tasks: T13455
Differential Revision: https://secure.phabricator.com/D21158
Summary:
Ref T13515. This restores the "Open in Editor" behavior to Diffusion, and makes "\" work there.
The URI pattern is now sent as a structured template to the client, so the code will work properly if a file path contains "%l".
Test Plan:
- Clicked "Open in Editor" and pressed "\" in Diffusion when viewing a file.
- Clicked a line, hit "\", got the file opened to that line.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21149
Summary: Ref T13515. External editor URIs currently depend on repositories having callsigns, but callsigns are no longer required. Add some variables to support configuring this feature for repositories that do not have callsigns.
Test Plan: Changed settings to use new variables, saw links generate appropriately.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21147
Summary:
Ref T13515.
- Previously valid editor URIs may become invalid without being changed (if an administrator removes a protocol from the list, for example), but this isn't explained very well. Show an error on the settings page if the current value isn't usable.
- Generate a list of functions from an authority in the parser.
- Generate a list of protocols from configuration.
Test Plan: {F7370872}
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21146
Summary:
Ref T13515. Currently, opening a file to a particular line in an external editor relies on replacing "%l" with "%l" (which is escaped as "%25l") on the server, and then replacing "%25l" with the line number on the client. This will fail if the file path (or any other variable) contains "%l" in its unencoded form.
The parser also can't identify invalid variables.
Pull the parser out, formalize it, and make it generate an intermediate representation which can be sent to the client and reconstituted.
(This temporarily breaks Diffusion and permanently removes the weird, ancient integration in Dark Console.)
Test Plan:
- Added a bunch of tests for the actual parser.
- Used "Open in Editor" in Differential.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21143
Summary:
Ref T13455. Update the other "view state" properties to work like "highlight" now works.
Some complexity here arises from these concerns:
- In "View Standalone", we render the changeset inline. This is useful for debugging/development, and desirable to retain.
- In all other cases, we render the changeset with AJAX.
So the client needs to be able to learn about the "state" properties of the changeset on two different flows. Prior to this change, each pathway had a fair amount of unique code.
Then, some bookkeeping issues:
- At inital rendering time, we may not know which renderer will be selected: it may be based on the client viewport dimensions.
- Prior to this change, the client didn't separate "value of the property for the changeset as rendered" and "desired value of the property".
Test Plan:
- Viewed changes in Differential, Diffusion, and in standalone mode.
- Toggled renderer, character sets, and document engine (this one isn't terribly useful). Reloaded, saw them stick.
- Started typing a comment, cancelled it, hit the undo UI.
Maniphest Tasks: T13455
Differential Revision: https://secure.phabricator.com/D21138
Summary:
Ref T13455. Add container-level storage for persistent view state, and persist "Highlight As..." inside it.
The storage generates a "PhabricatorChangesetViewState" configuration object as an output.
When preferences are expressed on a diff and that diff is later attached to a revision, we attempt to copy the preferences.
The internal storage tracks per-changeset settings, but currently always uses "last update wins" to apply the settings in the UI.
Test Plan:
- Viewed revisions, changed highlighting, reloaded. Saw highlighting stick in revision view and standalone view.
- Viewed commits, changed highlighting, reloaded. Saw highlighting stick.
- Created a diff, changed highlighting, turned it into a revision, saw highlighting persist.
Subscribers: jmeador, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13455
Differential Revision: https://secure.phabricator.com/D21137
Summary: Ref T13511. Currently, Ferret fulltext field functions (like "title:") are hard-coded. Modularize them so extensions may define new ones.
Test Plan: Added a new custom field which emits data for the indexer, searched for "animal-noises:moo", "animal-noises:-", etc., in global search and application search.
Maniphest Tasks: T13511
Differential Revision: https://secure.phabricator.com/D21131
Summary:
Ref T13501. Depends on D21127. With the "prefix" behavior removed in D21127, we now have two virtually identical copies of the same code.
The newer one in Ferret is better: it slices utf8 correctly and is slightly more efficient on large inputs. Pull it out and make all callers call into it.
Test Plan:
- Grepped for all affected symbols.
- Ran `bin/search index --force ...` to reindex various objects (tasks, files).
- Searched for things in the UI.
Maniphest Tasks: T13501
Differential Revision: https://secure.phabricator.com/D21128
Summary:
Ref T13501. The older ngram code has some "prefix" behavior that tries to handle cases where a user issues a very short (one or two character) query.
This code doesn't work, presumably never worked, and can not be made to work (or, at least, I don't see a way, and am fairly sure one does not exist).
If the user searches for "xy", we can find trigrams in the form "xy*" using the index, but not in the form "*xy". The code makes a misguided effort to look for " xy", but this will only find "xy" in words that begin with "xy", like "xylophone".
For example, searching Files for "om" does not currently find "random.txt".
Remove this behavior. Without engaging the trigram index, these queries fall back to an unidexed "LIKE" table scan, but that's about the best we can do.
Test Plan: Searched for "om", hit "random.txt".
Maniphest Tasks: T13501
Differential Revision: https://secure.phabricator.com/D21127
Summary:
Ref T13509. Now that the compiler can parse these queries, actually implement them.
These are fairly easy to implement:
- For present, just "JOIN". If it works, the field is present.
- For absent, we "LEFT JOIN" and then "WHERE any_column IS NULL".
Test Plan: Searched for various documents with and without fields present, got sensible results in Maniphest. For example, "body:-" finds tasks with no body, "body:- duck" finds tasks with no body and "duck" elsewhere in the content, and so on.
Maniphest Tasks: T13509
Differential Revision: https://secure.phabricator.com/D21110
Summary:
Ref T13509. Currently, functions are "sticky", but this stickness is in the query execution layer.
Instead:
- move stickiness to the query compiler; and
- make it so that functions are not sticky if their arguments are quoted.
For example:
- `title:x y` previously meant `title:x title:y` (and still does). The "title:" is sticky.
- `title:"x" y` previously meant `title:x title:y`. It now means `title:x all:y`. The "title:" is not sticky because the argument is quoted.
Test Plan: Added unit tests, ran unit tests.
Maniphest Tasks: T13509
Differential Revision: https://secure.phabricator.com/D21108
Summary:
Depends on D21053. Ref T11968. Three things have changed:
- Overseers can no longer use FutureIterator to continue execution of an arbitrary list of futures from any state. Use FuturePool instead.
- Same with repository daemons.
- Probably (?) fix an API change in the Harbormaster exec future.
Test Plan:
- Ran "bin/phd debug task" and "bin/phd debug pull", no longer saw Future-management related errors.
- The Harbormaster future is easiest to test by just seeing if production works once this change is deployed there.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21054
Summary: Depends on D21014. Ref T13493. Make these objects all use destructible interfaces and destroy sub-objects appropriately.
Test Plan:
- Used `bin/remove destroy --trace ...` to destroy a provider, a user, and an external account.
- Observed destruction of sub-objects, including external account identifiers.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21015
Summary: Ref T13395. Moves a small amount of remaining "libphutil/" code into "phabricator/" and stops us from loading "libphutil/".
Test Plan: Browsed around; there are likely remaining issues.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20981
Summary: Ref T13395. Move cache classes, syntax highlighters, other markup classes, and sprite sheets to Phabricator.
Test Plan: Attempted to find any callers for any of this stuff in libphutil or Arcanist and couldn't.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20977
Summary:
Fixes T5427. See PHI1630. See also T13160 and D20568.
In the full HTML table syntax with "<table>", respect linebreaks as literals inside "<td>" cells.
Test Plan: Previewed some full-HTML tables with and without linebreaks, saw what seemed like sensible rendering behavior.
Maniphest Tasks: T5427
Differential Revision: https://secure.phabricator.com/D20971
Summary: Fixes T13487. In PHI1628, an install has a 4MB remarkup corpus which takes a long time to render. This is broadly expected, but a few reasonable improvements fell out of running it through the profiler.
Test Plan:
- Saw local cold-cache end-to-end rendering time drop from 12s to 4s for the highly secret input corpus.
- Verified output has the same hashes before/after.
- Ran all remarkup unit tests.
Maniphest Tasks: T13487
Differential Revision: https://secure.phabricator.com/D20968
Summary:
Depends on D20966. Ref T13486. Curtains currently render subscribers in a plain text list, but the new ref list element is a good fit for this.
Also, improve the sorting and ordering behavior.
This makes the subscriber list take up a bit more space, but it should make it a lot easier to read at a glance.
Test Plan: Viewed object subscriber lists at varying limits and subscriber counts, saw sensible subscriber lists.
Maniphest Tasks: T13486
Differential Revision: https://secure.phabricator.com/D20967
Summary: Fixes T13335. When processing quoted blocks, we remove leading empty lines. This logic incorrectly continued after encountering a nonempty line.
Test Plan: Added a test, made it pass. Previewed blocks in web UI.
Maniphest Tasks: T13335
Differential Revision: https://secure.phabricator.com/D20965
Summary:
Ref T13484. If you load a subproject S which has a mangled/invalid `parentPath`, the query currently tries to execute an empty edge query and fatals.
Instead, we want to deny-by-default in the policy layer but not fail the query. The subproject should become restricted but not fatal anything related to it.
See T13484 for a future refinement where we could identify "broken / data integrity issue" objects explicilty.
Test Plan:
- Modified the `projectPath` of some subproject in the database to `QQQQ...`.
- Loaded that project page.
- Before patch: fatal after issuing bad edge query.
- After patch: "functionally correct" policy layer failure, although an explicit "data integrity issue" failure would be better.
Maniphest Tasks: T13484
Differential Revision: https://secure.phabricator.com/D20963
Summary:
See <https://hackerone.com/reports/758002>. The link rules don't test that their parameters are flat text before using them in unsafe contexts.
Since almost all rules are lower-priority than these link rules, this behavior isn't obvious. However, two rules have broadly higher priority (monospaced text, and one variation of link rules has higher priority than the other), and the latter can be used to perform an XSS attack with input in the general form `()[ [[ ... | ... ]] ]` so that the inner link rule is evaluated first, then the outer link rule uses non-flat text in an unsafe way.
Test Plan:
Tested examples in HackerOne report. A simple example of broken (but not unsafe) behavior is:
```
[[ `x` | `y` ]]
```
Differential Revision: https://secure.phabricator.com/D20937
Summary:
Fixes T13457. Ref T13444. When we iterate over commits in a particular repository, the default iteration strategy can't effectively use the keys on the table.
Tweak the ordering so the "<repositoryID, epoch, [id]>" key can be used.
Test Plan:
- Ran `bin/audit delete --repository X` and `bin/repository rebuild-identities --repository X` before and after changes.
- With just the key changes, performance was slightly better. My local data isn't large enough to really emphasize the key changes.
- With the page size changes, performance was a bit better (~30%, but on 1-3 second run durations).
- Used `--trace` and ran `EXPLAIN ...` on the new queries, saw them select the "<repositoryID, epoch, [id]>" key and report a bare "Using index condition" in the "Extra" column.
Maniphest Tasks: T13457, T13444
Differential Revision: https://secure.phabricator.com/D20921
Summary:
Ref T13444. Repository identities have, at a minimum, some bugs where they do not update relationships properly after many types of email address changes.
It is currently very difficult to fix this once the damage is done since there's no good way to inspect or rebuild them.
Take some steps toward improving observability and providing repair tools: allow `bin/repository rebuild-identities` to effect more repairs and operate on identities more surgically.
Test Plan: Ran `bin/repository rebuild-identities` with all new flags, saw what looked like reasonable rebuilds occur.
Maniphest Tasks: T13444
Differential Revision: https://secure.phabricator.com/D20911
Summary: Ref T13444. This is an ancient event and part of the old event system. It is not likely to be in use anymore, and repository identities should generally replace it nowadays anyway.
Test Plan: Grepped for constant and related methods, no longer found any hits.
Maniphest Tasks: T13444
Differential Revision: https://secure.phabricator.com/D20909
Summary: See PHI1466. When an install defines task subtypes, show them on the task graph.
Test Plan:
- On desktop with subtypes defined, column is visible.
- On desktop with subtypes not defined, column is hidden.
- On mobile, column is hidden.
{F6896845}
Differential Revision: https://secure.phabricator.com/D20842
Summary: Depends on D20838. Fixes T13414. Instead of doing coarse diffing with "PhutilEditDistanceMatrix", use hash-and-diff with "DocumentEngine".
Test Plan:
- On a large document (~3K top level blocks), saw a more sensible diff, instead of the whole thing falling back to "everything changed" mode.
- On a small document, still saw a sensible granular diff.
{F6888249}
Maniphest Tasks: T13414
Differential Revision: https://secure.phabricator.com/D20839
Summary: Depends on D20836. Ref T13414. Ref T13425. Ref T13395. Move these to "phabricator/" before trying to improve the high-level diff engine in prose diffs.
Test Plan: Ran "arc liberate", looked at a prose diff (no behavioral change).
Maniphest Tasks: T13425, T13414, T13395
Differential Revision: https://secure.phabricator.com/D20838
Summary: Ref T13410. Fixes T4280. Allows you to put a named anchor into a document explicitly.
Test Plan: Used `{anchor ...}` in Remarkup, used location bar to jump to anchors.
Maniphest Tasks: T13410, T4280
Differential Revision: https://secure.phabricator.com/D20825
Summary:
Depends on D20820. Ref T13410. We currently cut anchor names in the middle, don't support emoji in anchors, and generate relatively short anchors.
Generate slightly longer anchors, allow more unicode, and try not to cut things in the middle.
Test Plan: Created a document with a variety of different anchors and saw them generate more usable names.
Maniphest Tasks: T13410
Differential Revision: https://secure.phabricator.com/D20821
Summary:
Fixes T13392. If you have 17 load balancers in sequence, Phabricator will receive requests with at least 17 "X-Forwarded-For" components in the header.
We want to select the 17th-from-last element, since prior elements are not trustworthy.
This currently isn't very easy/obvious, and you have to add a kind of sketchy piece of custom code to `preamble.php` to do any "X-Forwarded-For" parsing. Make handling this correctly easier.
Test Plan:
- Ran unit tests.
- Configured my local `preamble.php` to call `preamble_trust_x_forwarded_for_header(4)`, then made `/debug/` dump the header and the final value of `REMOTE_ADDR`.
```
$ curl http://local.phacility.com/debug/
<pre>
HTTP_X_FORWARDED_FOR =
FINAL REMOTE_ADDR = 127.0.0.1
</pre>
```
```
$ curl -H 'X-Forwarded-For: 1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4, 5.5.5.5, 6.6.6.6' http://local.phacility.com/debug/
<pre>
HTTP_X_FORWARDED_FOR = 1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4, 5.5.5.5, 6.6.6.6
FINAL REMOTE_ADDR = 3.3.3.3
</pre>
```
```
$ curl -H 'X-Forwarded-For: 5.5.5.5, 6.6.6.6' http://local.phacility.com/debug/
<pre>
HTTP_X_FORWARDED_FOR = 5.5.5.5, 6.6.6.6
FINAL REMOTE_ADDR = 5.5.5.5
</pre>
```
Maniphest Tasks: T13392
Differential Revision: https://secure.phabricator.com/D20785
Summary:
Ref T13336. Currently, "bin/storage destroy" destroys every master. This is wonderfully destructive, but if replication fails it's useful to be able to destroy only a replica.
Operate on a single host, and require "--host" to target the operation in cluster mode, so `bin/storage destroy --host dbreplica001` is a useful operation.
Test Plan: Ran `bin/storage destroy` with various flags locally. Will destroy `secure002` and refresh replication.
Maniphest Tasks: T13336
Differential Revision: https://secure.phabricator.com/D20784
Summary:
Depends on D20780. Ref T13403. During initial setup, it's routine to run "bin/config" with a bad database config. We start the stack in "config optional" mode to anticipate this.
However, even in this mode, we may emit warnings if the connection fails in certain ways. These warnings aren't useful; suppress them with "@".
(Possibly this message should move from "phlog()" to "--trace" at some point, but it has a certain amount of context/history around it.)
Test Plan:
- Configured MySQL to fail with a retryable error, e.g. good host but bad port.
- Ran `bin/config set ...`.
- Before: saw retry warnings on stderr.
- After: no retry warnings on stderr.
- (Turned off suppression code artificially and verified warnings still appear under normal startup.)
Maniphest Tasks: T13403
Differential Revision: https://secure.phabricator.com/D20781
Summary: Depends on D20779. Ref T13403. Bad parameters may cause this call to fail without setting an error code; if it does, catch the issue and go down the normal connection error pathway.
Test Plan:
- With "mysql.port" set to "quack", ran `bin/storage probe`.
- Before: wild mess of warnings as the code continued below and failed when trying to interact with the connection.
- After: clean connection failure with a useful error message.
Maniphest Tasks: T13403
Differential Revision: https://secure.phabricator.com/D20780
Summary:
Fixes T13336.
- Prevent `--no-indexes` from being combined with `--for-replica`, since combining these options can only lead to heartbreak.
- In `--for-replica` mode, dump caches too. See discussion in T13336. It is probably "safe" to not dump these today, but fragile and not correct.
- Mark the "MarkupCache" table as having "Cache" persistence, not "Data" persistence (no need to back it up, since it can be fully regenerated from other datasources).
Test Plan: Ran `bin/storage dump` with various combinations of flags.
Maniphest Tasks: T13336
Differential Revision: https://secure.phabricator.com/D20743
Summary:
Fixes T13390. We have some old code which doesn't dynamically select between "utf8mb4" and "utf8". This can lead to dumping utf8mb4 data over a utf8 connection in `bin/storage dump`, which possibly corrupts some emoji/whales.
Instead, prefer "utf8mb4" if it's available.
Test Plan: Ran `bin/storage dump` and `bin/storage shell`, saw sub-commands select utf8mb4 as the client charset.
Maniphest Tasks: T13390
Differential Revision: https://secure.phabricator.com/D20742
Summary:
Ref T13386. If you issue `differential.query` with a large offset (like 3000), it can overheat regardless of policy filtering and fail with a nonsensical error message.
This is because the overheating limit is based only on the query limit, not on the offset.
For example, querying for "limit = 100" will never examine more than 1,100 rows, so a query with "limit = 100, offset = 3000" will always fail (provided there are at least that many revisions).
Not all numbers work like you might expect them to becuase there's also a 1024-row fetch window, but basically small limits plus big offsets always fail.
Test Plan: Artificially reduced the internal window size from 1024 to 5, then ran `differential.query` with `offset=50` and `limit=3`. Before: overheated with weird error message. After: clean result.
Maniphest Tasks: T13386
Differential Revision: https://secure.phabricator.com/D20728
Summary:
Fixes T13384. Currently, the subtype "disabled" configuration is not respected when selecting fields for `ROLE_EDIT`.
The only meaningful caller for `ROLE_EDIT` is transaction validation, but transaction validation should respect fields being disabled by subtype configuration.
Test Plan:
- Added a "required" Maniphest custom field "F", then "disabled" it in a subtype "S".
- Created a task of subtype "S".
- Before: Form submission fails with error "F is required", even though the field is not actually visible on the form and can not be set.
- After: Form submits cleanly and creates the task.
Maniphest Tasks: T13384
Differential Revision: https://secure.phabricator.com/D20726
Summary:
Ref T13373. When you "bin/config set x ..." a value, the success message ("Set x ...") is somewhat ambiguous and can be interpreted as "First, you need to set x..." rather than "Success, wrote x...".
Make the messaging more explicit. Also make this string more translatable.
Test Plan: Ran `bin/config set ...` with various combinations of flags, saw more clear messaging.
Maniphest Tasks: T13373
Differential Revision: https://secure.phabricator.com/D20711
Summary: Fixes T13355. This didn't appear to be a ton of extra work, we just didn't get it for free in the original implementation in D14635.
Test Plan:
- Saw "date" custom fields appear in Conduit API documentation for "maniphest.edit".
- Set custom "date" field to null and non-null values via the API.
{F6666582}
Maniphest Tasks: T13355
Differential Revision: https://secure.phabricator.com/D20690
Summary:
See D20650. Long ago, this got added as "pastebin", but that's the name of another product/company, not a generic term for paste storage.
Rename the database to `phabricator_paste`.
(An alternate version of this patch would rename `phabricator_search` to `phabricator_bing`, `phabricator_countdown` to `phabricator_spacex`, `phabricator_pholio` to `phabricator_adobe_photoshop`, etc.)
Test Plan:
- Grepped for `pastebin`, now only found references in old patches.
- Applied patches.
- Browsed around Paste in the UI without encountering issues.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20661
Summary:
Fixes T13345. See D20650. Currently, `PhabricatorCursorPagedPolicyAwareQuery` does a JOIN against the "title" field so it can apply additional ranking/ordering conditions to the query.
This means that documents with no title (which don't have this field) are always excluded from the result set.
We'd prefer to include them, just not give them any bonus ranking/relevance boost. Use a LEFT JOIN so they get included.
Test Plan:
- Applied D20650 (diff 1), made it use raw `getTitle()` as the document title, indexed a paste with no title.
- Searched for a term in the paste body.
- Before change: no results.
- After change: found result.
{F6601159}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13345
Differential Revision: https://secure.phabricator.com/D20660
Summary:
Fixes T13342. This does a few different things, although all of them seem small enough that I didn't bother splitting it up:
- Support export of "remarkup" custom fields as text. There's some argument here to export them in some kind of structure if the target is JSON, but it's hard for me to really imagine we'll live in a world some day where we really regret just exporting them as text.
- Support export of "date" custom fields as dates. This is easy except that I added `null` support.
- If you built PHP from source without "--enable-zip", as I did, you can hit the TODO in Excel exports about "ZipArchive". Since I had a reproduction case, test for "ZipArchive" and give the user a better error if it's missing.
- Add a setup check for the "zip" extension to try to avoid getting there in the first place. This is normally part of PHP so I believe users generally won't hit it, I just hit it because I built from source. See also T13232.
Test Plan:
- Added a custom "date" field. On tasks A and B, set it to null and some non-null value. Exported both tasks to Excel/JSON/text, saw null and a date, respectively.
- Added a custom "remarkup" field, exported some values, saw the values in Excel.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13342
Differential Revision: https://secure.phabricator.com/D20658
Summary: See PHI1319. Ref T13291. Bump the remarkup cache version, since the old JIRA / Asana rules may exist in the partial cached representation of remarkup blocks from older versions.
Test Plan: Typed some comments with various formatting, saw remarkup work fine.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13291
Differential Revision: https://secure.phabricator.com/D20619
Summary:
Ref T13328. Currently, we read from `mysqldump` something like this:
```
until (done) {
for (100 ms) {
mysqldump > in-memory-buffer;
}
in-memory-buffer > disk;
}
```
This general structure isn't great. In this use case, where we're streaming a large amount of data from a source to a sink, we'd prefer to have a "select()"-like way to interact with futures, so our code is called after every read (or maybe once some small buffer fills up, if we want to do the writes in larger chunks).
We don't currently have this (`FutureIterator` can wake up every X milliseconds, or on future exit, but, today, can not wake for readable futures), so we may buffer an arbitrary amount of data into memory (however much data `mysqldump` can write in 100ms).
Reduce the update frequency from 100ms to 10ms, and limit the buffer size to 32MB. This effectively imposes an artificial 3,200MB/sec limit on throughput, but hopefully that's fast enough that we'll have a "wake on readable" mechanism by the time it's a problem.
Test Plan:
- Replaced `mysqldump` with `cat /dev/zero` as the source command, to get fast input.
- Ran `bin/storage dump` with `var_dump()` on the buffer size.
- Before change: saw arbitrarily large buffers (300MB+).
- After change: saw consistent maximum buffer size of 32MB.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13328
Differential Revision: https://secure.phabricator.com/D20617
Summary: Ref T13321. The daemons no longer write PID files, so we no longer need to pass any of this stuff to them.
Test Plan: Grepped for affected symbols.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13321
Differential Revision: https://secure.phabricator.com/D20608
Summary:
Fixes T13304. Shell pipes and redirects do not have robust behavior when errors occur. We provide "--compress" and "--output" flags as robust alternatives, but do not currently recommend their use.
- Recommend their use, since their error handling behavior is more robust in the face of issues like full disks.
- If "--compress" is provided but won't work because the "zlib" extension is missing, raise an explicit error. I believe this extension is very common and this error should be rare. If that turns out to be untrue, we could take another look at this.
- Also, verify some flag usage sooner so we can exit with an error faster if you mistype a "bin/storage dump" command.
Test Plan: Read documentation, hit affected error cases, did a dump and spot-checked that it came out sane looking.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13304
Differential Revision: https://secure.phabricator.com/D20572
Summary:
See downstream <https://phabricator.wikimedia.org/T902>. Currently, timezones are rendered with their raw internal names (like `America/Los_Angeles`) which include underscores.
Replacing underscores with spaces is a more human-readable (and perhaps meaningfully better for things like screen readers, although this is pure speculation).
There's some vague argument against this, like "administrators may need to set a raw internal value in `phabricator.timezone` and this could mislead them", but we already give a pretty good error message if you do this and could improve hinting if necessary.
Test Plan: Viewed timezone list in {nav Settings} and the timezone "reconcile" dialog, saw a more-readable "Los Angeles".
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20559
Summary:
Ref T13294. An install is interested in a way to easily answer audit-focused questions like "what edits were made to any Herald rule in Q1 2019?".
We can answer this kind of question with a more granular version of feed that focuses on being exhaustive rather than being human-readable.
This starts a rough version of it and deals with the two major tricky pieces: transactions are in a lot of different tables; and paging across them is not trivial.
To solve "lots of tables", we just query every table. There's a little bit of sleight-of-hand to get this working, but nothing too awful.
To solve "paging is hard", we order by "<dateCreated, phid>". The "phid" part of this order doesn't have much meaning, but it lets us put every transaction in a single, stable, global order and identify a place in that ordering given only one transaction PHID.
Test Plan: {F6463076}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13294
Differential Revision: https://secure.phabricator.com/D20531
Summary:
See PHI985. I think we pretty much need to start applying language-specific rules, but we can apply at least one more relatively language-agnostic rule: don't match lines which are indented 3+ levels.
In C++, we may have symbols like this:
```
class X {
public:
int m() { ... }
}
```
..but I believe no mainstream language puts symbol definitions 3+ levels deep.
Also clean up some of the tab handling very slightly.
Test Plan: Tests pass, looked at some C++ code and got slightly better (but still not great) matches.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20479
Summary:
Depends on D20380. Ref T8093. When prototypes are enabled, inject a (hopefully?) no-op proxy into the Git wire protocol.
This proxy decodes "git upload-pack" and allows the list of references to be rewritten, in a similar way to how we already proxy the Subversion protocol to rewrite URIs and proxy the Mercurial protocol to distinguish between read and write operations.
The piece we care about comes at the beginning, and looks like this:
```
<frame-length><ref-hash> <ref-name>\0<server-capabilities>\n
<frame-length><ref-hash> <ref-name>\n
<frame-length><ref-hash> <ref-name>\n
...
<0000>
```
We can add, remove, or modify this section to make it appear that the server has different refs than the refs that exist on disk.
Things I have tried:
- `git ls-remote`
- `git ls-remote` where the server hides some refs.
- `git fetch` where the fetch is a no-op.
Things I have not tried:
- `git fetch` where the fetch is not a no-op.
- Tricking things into doing protocol v2. Or: I tried this, I wasn't successful. In v2, additional "\0" tricks are used to hide data in the capabilities, I think?
- `git ls-remote` where we rewrite/hide the first ref in the list, and need to move the capabilities frame elsewhere.
- `git ls-remote` where the server has no refs at all, or we remove every ref.
So the "interesting" piece of this works, but it almost certainly needs some cleanup to survive interaction with the real world.
Test Plan: See above.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T8093
Differential Revision: https://secure.phabricator.com/D20381
Summary:
Ref T8093. Support dumping the protocol bytes to a side channel logfile, as a precursor to parsing the protocol and rewriting protocol frames to virtualize refs.
The protocol itself is mostly ASCII text so the raw protocol bytes are pretty comprehensible.
Test Plan:
{F6363221}
{F6363222}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T8093
Differential Revision: https://secure.phabricator.com/D20380
Summary:
Depends on D20411. Ref T13272. Dashboards and panels have new indexes (Ferret and usage edges) that need a rebuild.
For large datasets like commits we have the "activity" flow in T11932, but realistically these rebuilds won't take more than a few minutes on any realistic install so we should be able to just queue them up as migrations.
Let migrations insert a job to basically run `bin/search index --type SomeObjectType`, then do that for dashboards and panels.
(I'll do Herald rules in a followup too, but I want to tweak one indexing thing there.)
Test Plan: Ran the migration, ran `bin/phd debug task`, saw everything get indexed with no manual intervention.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20412
Summary:
See PHI1182. Ref T13266. The recent fixes didn't quite cover the case where you have a query, but order by something other than relevance, and page forward.
Refine the tests around building/selecting these columns and paging values a little bit to be more specific about what they care about.
Test Plan:
Executed queries, then went to "Next Page", for:
- query text, non-relevance order.
- query text, relevance order.
- no query text, non-relevance order.
- no query text, relevance order.
Also, made an API call similar to the one in PHI1182.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13266
Differential Revision: https://secure.phabricator.com/D20354
Summary: See PHI1180. Currently, when we failover to a replica, we may not log the failure. Failovers are serious business and bad news, so emit a log even if we are able to connect to the replica.
Test Plan:
Configured a bogus master and a good replica:
```
$ ./bin/mail list-outbound
[2019-03-29 16:26:09] PHLOG: 'Retrying (attempt 1) after connection failure ("AphrontConnectionQueryException", #2002): Attempt to connect to root@127.0.0.2 failed with error #2002: Operation timed out.' at [/Users/epriestley/dev/core/lib/libphutil/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:124]
[2019-03-29 16:26:19] PHLOG: 'Retrying (attempt 2) after connection failure ("AphrontConnectionQueryException", #2002): Attempt to connect to root@127.0.0.2 failed with error #2002: Operation timed out.' at [/Users/epriestley/dev/core/lib/libphutil/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:124]
[2019-03-29 16:26:29] EXCEPTION: (PhutilProxyException) Failed to connect to master database ("local_config"), failing over into read-only mode. {>} (AphrontConnectionQueryException) Attempt to connect to root@127.0.0.2 failed with error #2002: Operation timed out. at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:362]
<...snip backtrace...>
3945 Voided email rP04f9e72cbd10: Don't subscribe bots implicitly when they act on objects, or when they are…
3946 Voided email rPdf53d72e794c: Allow "Move Tasks to Column..." to prompt for MFA
3947 Voided email rP492b03628f19: Fix a typo in Drydock "Land" operations
3948 Voided email rPb469a5134ddd: Allow "SMTP" and "Sendmail" mailers to have "Message-ID" behavior configured in…
3949 Voided email rPa6fd8f04792d: When performing complex edits, pause sub-editors before they publish to…
...
```
Configured a bogus master and a bogus replica:
```
$ ./bin/mail list-outbound
[2019-03-29 16:26:57] PHLOG: 'Retrying (attempt 1) after connection failure ("AphrontConnectionQueryException", #2002): Attempt to connect to root@127.0.0.2 failed with error #2002: Operation timed out.' at [/Users/epriestley/dev/core/lib/libphutil/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:124]
[2019-03-29 16:27:07] PHLOG: 'Retrying (attempt 2) after connection failure ("AphrontConnectionQueryException", #2002): Attempt to connect to root@127.0.0.2 failed with error #2002: Operation timed out.' at [/Users/epriestley/dev/core/lib/libphutil/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:124]
[2019-03-29 16:27:27] PHLOG: 'Retrying (attempt 1) after connection failure ("AphrontConnectionQueryException", #2002): Attempt to connect to root@127.0.0.3 failed with error #2002: Operation timed out.' at [/Users/epriestley/dev/core/lib/libphutil/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:124]
[2019-03-29 16:27:37] PHLOG: 'Retrying (attempt 2) after connection failure ("AphrontConnectionQueryException", #2002): Attempt to connect to root@127.0.0.3 failed with error #2002: Operation timed out.' at [/Users/epriestley/dev/core/lib/libphutil/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:124]
[2019-03-29 16:27:47] EXCEPTION: (PhabricatorClusterStrandedException) Unable to establish a connection to any database host (while trying "local_config"). All masters and replicas are completely unreachable.
AphrontConnectionQueryException: Attempt to connect to root@127.0.0.2 failed with error #2002: Operation timed out. at [<phabricator>/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php:177]
<...snip backtrace...>
```
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20351
Summary:
Ref T13091. The Ferret "rank" column is a function of the query text and looks something like `SELECT ..., 2 + 2 AS rank, ...`.
You can't apply conditions to this kind of dynamic column with a WHERE clause: you get a slightly unhelpful error like "column rank unknown in where clause". You must use HAVING:
```
mysql> SELECT 2 + 2 AS x WHERE x = 4;
ERROR 1054 (42S22): Unknown column 'x' in 'where clause'
mysql> SELECT 2 + 2 AS x HAVING x = 4;
+---+
| x |
+---+
| 4 |
+---+
1 row in set (0.00 sec)
```
Add a flag to paging column definitions to let them specify that they must be applied with HAVING, then apply the whole paging clause with HAVING if any column requires HAVING.
Test Plan:
- In Maniphest, ran a fulltext search matching more than 100 results, ordered by "Relevance", then clicked "Next Page".
- Before patch: query with `... WHERE rank > 123 OR ...` caused MySQL error because `rank` is not a WHERE-able column.
- After patch: query builds as `... HAVING rank > 123 OR ...`, pages properly, no MySQL error.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13091
Differential Revision: https://secure.phabricator.com/D20298
Summary:
See downstream <https://phabricator.wikimedia.org/T210482>.
On mobile, the task graph can take up most of the screen. Hide it on devices. Keep it on the standalone view if you're really dedicated and willing to rotate your phone or whatever to see the lines.
Test Plan: Dragged window real narrow, saw graph hide.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20313
Ref T13266. We never page these queries, and previously never reached the
"nextPage()" method. The call order changed recently and this method is now
reachable. For now, just no-op it rather than throwing.
Summary:
Ref T13091. In Differential, if you provide a query and "Sort by: Relevance", we build a query like this:
```
((SELECT revision.* FROM ... ORDER BY rank) UNION ALL (SELECT revision.* FROM ... ORDER BY rank)) ORDER BY rank
```
The internal "ORDER BY rank" is technically redundant (probably?), but doesn't hurt anything, and makes construction easier.
The problem is that the outer "ORDER BY rank" at the end, which attempts to order the results of the two parts of the UNION, can't actually order them, since `rank` wasn't selected.
(The column isn't actually "rank", which //is// selected -- it's the document modified/created subcolumns, which are not.)
To fix this, actually select the fulltext columns into the result set.
Test Plan:
- Ran a non-empty fulltext query in Differential with "Bucket: Required Action" selected so the UNION construction fired.
- Ran normal queries in Maniphest and global search.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13091
Differential Revision: https://secure.phabricator.com/D20297
Summary:
Ref T13091. If you "Order By: Relevance" but don't actually specify a query, we currently raise a bare exception.
This operation is sort of silly/pointless, but it seems like it's probably best to just return the results for the other constraints in the fallback order (usually, by ID). Alternatively, we could raise a non-bare exception here ("You need to provide a fulltext query to order by relevance.")
Test Plan: Queried tasks by relevance with no actual query text.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13091
Differential Revision: https://secure.phabricator.com/D20296
Summary:
Ref T13259. Currently, visiting a page that executes a query with an invalid cursor raises a bare exception that escapes to top level.
Catch this a little sooner and tailor the page a bit.
Test Plan: Visited `/maniphest/?after=335234234223`, saw a nicer exception page.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20295
Summary:
Ref T13259. Currently, queries set a flag and return a partial result set when they overheat. This is mostly okay:
- It's very unusual for queries to overheat if they don't have a real viewer.
- Overheating is rare in general.
- In most cases where queries can overheat, the context is a SearchEngine UI, which handles this properly.
In T13259, we hit a case where a query with an omnipotent viewer can overheat: if you have more than 1,000 consecutive commits in the database with invalid `repositoryID` values, we'll overheat and bail out. This is pretty bad, since we don't process everything.
Change this beahvior:
- Throw by default, so this stuff doesn't slip through the cracks.
- Handle the SearchEngine case explicitly ("it's okay to overheat, we'll handle it").
- Make `QueryIterator` disable overheating behavior: if we're iterating over all objects, we want to hit the whole table even if most of it is garbage.
There are some cases where this might cause new exception behavior that we don't necessarily want. For example, in Owners, each package shows "recent commits in this package". If you can't see the first 1,000 recent commits, you'd previously get a slow page with no results. Now you'll probably get an exception.
If these crop up, I think the best approach for now is to deal with them on a case-by-case basis and see how far we get. In the "Owners" case, it might be good to query by repositories you can see first, then query by commits in the package in those repositories. That should give us a better outcome than any generic behavior we could implement.
Test Plan:
- Added 100000 to all repositoryID values for commits on my local install.
- Before making changes, ran `bin/repository rebuild-identities --all --trace`. Saw the script process 1,000 rows and exit silently.
- Applied the first part ("throw by default") and ran `bin/repository rebuild-identities`. Saw the script process 1,000 rows, then raise an exception.
- Applied the second part ("disable for queryiterator") and ran the script again. Saw the script process all 15,000 rows without issues (although none are valid and none actually load).
- Viewed Diffusion, saw appropriate NUX / "overheated" UIs.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20294
Summary:
Depends on D20292. Ref T13259. This converts the rest of the `getPagingValueMap()` callsites to operate on internal cursors instead.
These are pretty one-off for the most part, so I'll annotate them inline.
Test Plan:
- Grouped tasks by project, sorted by title, paged through them, saw consistent outcomes.
- Queried edges with "edge.search", paged through them using the "after" cursor.
- Poked around the other stuff without catching any brokenness.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20293
Summary:
Depends on D20291. Ref T13259. Move all the simple cases (where paging depends only on the partial object and does not depend on keys) to a simple wrapper.
This leaves a smaller set of more complex cases where we care about external data or which keys were requested that I'll convert in followups.
Test Plan: Poked at things, but a lot of stuff is still broken until everything is converted.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20292
Summary:
Ref T13259.
(NOTE) This is "infrastructure/guts" only and breaks some stuff in Query subclasses. I'll fix that stuff in a followup, it's just going to be a larger diff that's mostly mechanical.
When a user clicks "Next Page" on a tasks view and gets `?after=100`, we want to show them the next 100 //visible// tasks. It's possible that tasks 1-100 are visible, but tasks 101-788 are not, and the next visible task is 789.
We load task ID `100` first, to make sure they can actually see it: you aren't allowed to page based on objects you can't see. If we let you, you could use "order=title&after=100", plus creative retitling of tasks, to discover the title of task 100: create tasks named "A", "B", etc., and see which one is returned first "after" task 100. If it's "D", you know task 100 must start with "C".
Assume the user can see task 100. We run a query like `id > 100` to get the next 100 tasks.
However, it's possible that few (or none) of these tasks can be seen. If the next visible task is 789, none of the tasks in the next page of results will survive policy filtering.
So, for queries after the initial query, we need to be able to page based on tasks that the user can not see: we want to be able to issue `id > 100`, then `id > 200`, and so on, until we overheat or find a page of results (if 789-889 are visible, we'll make it there before overheating).
Currently, we do this in a not-so-great way:
- We pass the external cursor (`100`) directly to the subquery.
- We query for that object using `getPagingViewer()`, which is a piece of magic that returns the real viewer on the first page and the omnipotent viewer on the 2nd..nth page. This is very sketchy.
- The subquery builds paging values based on that object (`array('id' => 100)`).
- We turn the last result from the subquery back into an external cursor (`200`) and save it for the next time.
Note that the last step happens BEFORE policy (and other) filtering.
The problems with this are:
- The phantom-schrodinger's-omnipotent-viewer thing isn't explicity bad, but it's sketchy and generally not good. It feels like it could easily lead to a mistake or bug eventually.
- We issue an extra query each time we page results, to convert the external cursor back into a map (`100`, `200`, `300`, etc).
- In T13259, there's a new problem: this only works if the object is filtered out for policy reasons and the omnipotent viewer can still see it. It doesn't work if the object is filtered for some other reason.
To expand on the third point: in T13259, we hit a case where 100+ consecutive objects are broken (they point to a nonexistent `repositoryID`). These objects get filtered unconditionally. It doesn't matter if the viewer is omnipotent or not.
In that case: we set the next external cursor from the raw results (e.g., `200`). Then we try to load it (using the omnipotent viewer) to turn it into a map of values for paging. This fails because the object isn't loadable, even as the omnipotent viewer.
---
To fix this stuff, the new approach steps back a little bit. Primarily, I'm separating "external cursors" from "internal cursors".
An "External Cursor" is a string that we can pass in `?after=X` URIs. It generally identifies an object which the user can see.
An "Internal Cursor" is a raw result from `loadPage()`, i.e. before policy filtering. Usually, (but not always) this is a `LiskDAO` object that doesn't have anything attached yet and hasn't been policy filtered.
We now do this, broadly:
- Convert the external cursor to an internal cursor.
- Execute the query using internal cursors.
- If necessary, convert the last visible result back into an external cursor at the very end.
This fixes all the problems:
- Sketchy Omnipotent Viewer: We no longer ever use an omnipotent viewer. (We pick cursors out of the result set earlier, instead.)
- Too Many Queries: We only issue one query at the beginning, when going from "external" to "internal". This query is generally unavoidable since we need to make sure the viewer can see the object and that it's a real / legitimate object. We no longer have to query an extra time for each page.
- Total Failure on Invalid Objects: we now page directly with objects out of `loadPage()`, before any filtering, so we can page over invisible or invalid objects without issues.
This change switches us over to internal/external cursors, and makes simple cases (ID-based ordering) work correctly. It doesn't work for complex cases yet since subclasses don't know how to get paging values out of an internal cursor yet. I'll update those in a followup.
Test Plan: For now, poked around a bit. Some stuff is broken, but normal ID-based lists load correctly and page properly. See next diff for a more detailed test plan.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20291
Summary:
See PHI1063. See PHI1114. Ref T13253. Currently, you can't `bin/worker execute` an archived task and can't `bin/worker retry` a successful task.
Although it's good not to do these things by default (particularly, retrying a successful task will double its effects), there are plenty of cases where you want to re-run something for testing/development/debugging and don't care that the effect will repeat (you're in a dev environment, the effect doesn't matter, etc).
Test Plan: Ran `bin/worker execute/retry` against archived/successful tasks. Got prompted to add more flags, then got re-execution.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13253
Differential Revision: https://secure.phabricator.com/D20246
Summary:
See <https://discourse.phabricator-community.org/t/unhandled-exception-muting-completed-bulk-jobs/2449>. Bulk Jobs have an "edge" table but currently do not support edge transactions. Add support.
This stops "Mute Notifications" from fataling.
The action probably doesn't do what the reporting user expects (it stops edits to the job object from sending notifications; it does not stop the edits the job performs from sending notifications) but I think this change puts us in a better place no matter what, even if we eventually clarify or remove this behavior.
Test Plan: Clicked "Mute Notifications" on a bulk job, got an effect instead of a fatal.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20226
Summary:
See <https://discourse.phabricator-community.org/t/traceback-rendering-task-query-in-dashboard/2450/>.
It looks like this blames to D19126, which added some more complex constraint logic but overlooked "range" constraints, which are handled separately.
Test Plan:
- Added a custom "date" field to Maniphest with `"search": true`.
- Executed a range query against the field.
Then:
- Before: Warnings about undefined indexes in the log.
- After: No such warnings.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jbrownEP
Differential Revision: https://secure.phabricator.com/D20225
Summary:
Ref T5401. Depends on D20201. Add timestamps to worker tasks to track task creation, and pass that through to archive tasks. This lets us measure the total time the task spent in the queue, not just the duration it was actually running.
Also displays this information in the daemon status console; see screenshot: {F6225726}
Test Plan:
Stopped daemons, ran `bin/search index --all --background` to create lots of tasks, restarted daemons, observed expected values for `dateCreated` and `epochArchived` in the archive worker table.
Also tested the changes to `unarchiveTask` by forcing a search task to permanently fail and then `bin/worker retry`ing it.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T5401
Differential Revision: https://secure.phabricator.com/D20200
Summary:
Depends on D20196. See PHI985. When empty, the "moved/copied" gutter currently renders with the same background color as the rest of the line. This can be misleading because it makes code look more indented than it is, especially if you're unfamiliar with the tool:
{F6225179}
If we remove this misleading coloration, we get a white gap. This is more clear, but looks a little odd:
{F6225181}
Instead, give this gutter a subtle background fill in all casses, to make it more clear that it's a separate gutter region, not a part of the text diff:
{F6225183}
Test Plan: See screenshots. Copied text from a diff, added/removed inlines, etc.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20197
Summary:
Ref T13161. Ref T12822. See PHI870. Long ago, the web was simple. You could leave your doors unlocked, you knew all your neighbors, crime hadn't been invented yet, and `<th>3</th>` was a perfectly fine way to render a line number cell containing the number "3".
But times have changed!
- In PHI870, this isn't good for screenreaders. We can't do much about this, so switch to `<td>`.
- In D19349 / T13105 and elsewhere, this `::after { content: attr(data-n); }` approach seems like the least bad general-purpose approach for preventing line numbers from being copied. Although Differential needs even more magic beyond this in the two-up view, this is likely good enough for the one-up view, and is consistent with other views (paste, harbormaster logs, general source display) where this technique is sufficient on its own.
The chance this breaks //something// is pretty much 100%, but we've got a week to figure out what it breaks. I couldn't find any issues immediately.
Test Plan:
- Created, edited, deleted inlines in 1-up and 2-up views.
- Replied, keyboard-navigated, keyboard-replied, drag-selected, poked and prodded everything.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T12822
Differential Revision: https://secure.phabricator.com/D20188
Summary:
Depends on D20185. Ref T13161. Fixes T6791.
See some discusison in T13161. I want to move to a world where:
- whitespace changes are always shown, so users writing YAML and Python are happy without adjusting settings;
- the visual impact of indentation-only whitespace changes is significanlty reduced, so indentation changes are easy to read and users writing Javascript or other flavors of Javascript are happy.
D20181 needs a little more work, but generally tackles these visual changes and lets us always show whitespace changes, but show them in a very low-impact way when they're relatively unimportant.
However, a second aspect to this is how the diff is "aligned". If this file:
```
A
```
..is changed to this file:
```
X
A
Y
Z
```
...diff tools will generally produce this diff:
```
+ X
A
+ Y
+ Z
```
This is good, and easy to read, and what humans expect, and it will "align" in two-up like this:
```
1 X
1 A 2 A
3 Y
4 Z
```
However, if the new file looks like this instead:
```
X
A'
Y
Z
```
...we get a diff like this:
```
- A
+ X
+ A'
+ Y
+ Z
```
This one aligns like this:
```
1 A
1 X
2 A'
3 Y
4 Z
```
This is correct if `A` and `A'` are totally different lines. However, if `A'` is pretty much the same as `A` and it just had a whitespace change, human viewers would prefer this alignment:
```
1 X
1 A 2 A'
3 Y
4 Z
```
Note that `A` and `A'` are different, but we've aligned them on the same line. `diff`, `git diff`, etc., won't do this automatically, and a `.diff` doesn't have a way to say "these lines are more or less the same even though they're different", although some other visual diff tools will do this.
Although `diff` can't do this for us, we can do it ourselves, and already have the code to do it, because we already nearly did this in the changes removed in D20185: in "Ignore All" or "Ignore Most" mode, we pretty much did this already.
This mostly just restores a bit of the code from D20185, with some adjustments/simplifications. Here's how it works:
- Rebuild the text of the old and new files from the diff we got out of `arc`, `git diff`, etc.
- Normalize the files (for example, by removing whitespace from each line).
- Diff the normalized files to produce a second diff.
- Parse that diff.
- Take the "alignment" from the normalized diff (whitespace removed) and the actual text from the original diff (whitespace preserved) to build a new diff with the correct text, but also better diff alignment.
Originally, we normalized with `diff -bw`. I've replaced that with `preg_replace()` here mostly just so that we have more control over things. I believe the two behaviors are pretty much identical, but this way lets us see more of the pipeline and possibly add more behaviors in the future to improve diff quality (e.g., normalize case? normalize text encoding?).
Test Plan:
{F6217133}
(Also, fix a unit test.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T6791
Differential Revision: https://secure.phabricator.com/D20187
Summary:
Depends on D20181. Depends on D20182. Fixes T3498. Ref T13161. My claim, at least, is that D20181 can be tweaked to be good enough to throw away this "feature" completely.
I think this feature was sort of a mistake, where the ease of access to `diff -bw` shaped behavior a very long time ago and then the train just ran a long way down the tracks in the same direction.
Test Plan: Grepped for `whitespace`, deleted almost everything. Poked around the UI a bit. I'm expecting the whitespace changes to get some more iteration this week so I not being hugely pedantic about testing this stuff exhaustively.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T3498
Differential Revision: https://secure.phabricator.com/D20185
Summary:
Ref T13249. Ref T11738. See PHI985. Currently, we have a crude heuristic for guessing what line in a source file provides the best context.
We get it wrong in a lot of cases, sometimes selecting very silly lines like "{". Although we can't always pick the same line a human would pick, we //can// pile on heuristics until this is less frequently completely wrong and perhaps eventually get it to work fairly well most of the time.
Pull the logic for this into a separate standalone class and make it testable to prepare for adding heuristics.
Test Plan: Ran unit tests, browsed various files in the web UI and saw as-good-or-better context selection.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249, T11738
Differential Revision: https://secure.phabricator.com/D20171
Summary:
Ref T13248. This will probably need quite a bit of refinement, but we can reasonably allow subtype definitions to adjust custom field behavior.
Some places where we use fields are global, and always need to show all the fields. For example, on `/maniphest/`, where you can search across all tasks, you need to be able to search across all fields that are present on any task.
Likewise, if you "export" a bunch of tasks into a spreadsheet, we need to have columns for every field.
However, when you're clearly in the scope of a particular task (like viewing or editing `T123`), there's no reason we can't hide fields based on the task subtype.
To start with, allow subtypes to override "disabled" and "name" for custom fields.
Test Plan:
- Defined several custom fields and several subtypes.
- Disabled/renamed some fields for some subtypes.
- Viewed/edited tasks of different subtypes, got desired field behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13248
Differential Revision: https://secure.phabricator.com/D20161
Summary:
Ref T13253. Fixes T6615. See that task for discussion.
- Remove three keys which serve no real purpose: `dataID` doesn't do anything for us, and the two `leaseOwner` keys are unused.
- Rename `leaseOwner_2` to `key_owner`.
- Fix an issue where `dataID` was nullable in the active table and non-nullable in the archive table.
In practice, //all// workers have data, so all workers have a `dataID`: if they didn't, we'd already fatal when trying to move tasks to the archive table. Just clean this up for consistency, and remove the ancient codepath which imagined tasks with no data.
Test Plan:
- Ran `bin/storage upgrade`, inspected tables.
- Ran `bin/phd debug taskmaster`, worked through a bunch of tasks with no problems.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13253, T6615
Differential Revision: https://secure.phabricator.com/D20175
Summary:
Depends on D20175. Ref T12425. Ref T13253. Currently, importing commits can stall search index rebuilds, since index rebuilds use an older priority from before T11677 and weren't really updated for D16585.
In general, we'd like to complete all indexing tasks before continuing repository imports. A possible exception is if you rebuild an entire index with `bin/search index --rebuild-the-world`, but we could queue those at a separate lower priority if issues arise.
Test Plan: Ran some search indexing through the queue.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13253, T12425
Differential Revision: https://secure.phabricator.com/D20177
Summary:
Ref T6703. When we import external data from a third-party install to a Phacility instance, we must link instance accounts to central accounts: either existing central accounts, or newly created central accounts that we send invites for.
During this import, or when users register and claim those new accounts, we do a write from `admin.phacility.com` directly into the instance database to link the accounts.
This is pretty sketchy, and should almost certainly just be an internal API instead, particularly now that it's relatively stable.
However, it's what we use for now. The process has had some issues since the introduction of `%R` (combined database name and table refrence in queries), and now needs to be updated for the new `providerConfigPHID` column in `ExternalAccount`.
The problem is that `%R` isn't doing the right thing. We have code like this:
```
$conn = new_connection_to_instance('turtle');
queryf($conn, 'INSERT INTO %R ...', $table);
```
However, the `$table` resolves `%R` using the currently-executing-process information, not anything specific to `$conn`, so it prints `admin_user.user_externalaccount` (the name of the table on `admin.phacility.com`, where the code is running).
We want it to print `turtle_user.user_externalaccount` instead: the name of the table on `turtle.phacility.com`, where we're actually writing.
To force this to happen, let callers override the namespace part of the database name.
Long term: I'd plan to rip this out and replace it with an API call. This "connect directly to the database" stuff is nice for iterating on (only `admin` needs hotfixes) but very very sketchy for maintaining.
Test Plan: See next diff.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20167
Summary: Ref T13250. See D20149. Mostly: clarify semantics. Partly: remove magic "null" behavior.
Test Plan: Poked around, but mostly just inspection since these are pretty much one-for-one.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20154
Summary:
Ref T13250. Ref T13249. Some remarkup rules, including `{image ...}` and `{meme ...}`, may cache URIs as objects because the remarkup cache is `serialize()`-based.
URI objects with `query` cached as a key-value map are no longer valid and can raise `__toString()` fatals.
Bump the cache version to purge them out of the cache.
Test Plan: See PHI1074.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13250, T13249
Differential Revision: https://secure.phabricator.com/D20160
Summary: See D20136. This method is sort of inherently bad because it is destructive for some inputs (`x=1&x=2`) and had "PHP-flavored" behavior for other inputs (`x[]=1&x[]=2`). Move to explicit `...AsMap` and `...AsPairList` methods.
Test Plan: Bit of an adventure, see inlines in a minute.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20141
Summary:
Depends on D20106. Ref T6703. Since I plan to change the `ExternalAccount` table, these migrations (which rely on `save()`) will stop working.
They could be rewritten to use raw queries, but I suspect few or no installs are affected. At least for now, just make them safe: if they would affect data, fatal and tell the user to perform a more gradual upgrade.
Also remove an `ALTER IGNORE TABLE` (this syntax was removed at some point) and fix a `%Q` when adjusting certain types of primary keys.
Test Plan: Ran `bin/storage upgrade --no-quickstart --force --namespace test1234` to get a complete migration since the beginning of time.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20107
Summary:
Ref T13244. See D20080. Rather than randomly jittering service calls, we can give each host a "metronome" that ticks every 60 seconds to get load to spread out after one cycle.
For example, web001 ticks (and makes a service call) when the second hand points at 0:17, web002 at 0:43, web003 at 0:04, etc.
For now I'm just planning to seed the metronomes randomly based on hostname, but we could conceivably give each host an assigned offset some day if we want perfectly smooth service call rates.
Test Plan: Ran unit tests.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20087
Summary:
See T13240. Ref T13242. When we're issuing a query that will raise policy exceptions (i.e., give the user a "You Shall Not Pass" dialog if they can not see objects it loads), don't do space filtering in MySQL: when objects are filtered out in MySQL, we can't distinguish between "bad/invalid ID/object" and "policy filter", so we can't raise a policy exception.
This leads to cases where viewing an object shows "You Shall Not Pass" if you can't see it for any non-Spaces reason, but "404" if the reason is Spaces.
There's no product reason for this, it's just that `spacePHID IN (...)` is important for non-policy-raising queries (like a list of tasks) to reduce how much application filtering we need to do.
Test Plan:
Before:
```
$ git pull
phabricator-ssh-exec: No repository "spellbook" exists!
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
```
After:
```
$ git pull
phabricator-ssh-exec: [You Shall Not Pass: Unknown Object (Repository)] This object is in a space you do not have permission to access.
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
```
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13242
Differential Revision: https://secure.phabricator.com/D20042
Summary:
Ref T920. Ref T13235. This adds a `Future`, similar to `TwilioFuture`, for interacting with Amazon's SNS service.
Also updates the documentation.
Also makes the code consistent with the documentation by accepting a `media` argument.
Test Plan: Clicked the "send test message" button from the Settings UI.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T13235, T920
Differential Revision: https://secure.phabricator.com/D19982
Summary:
Depends on D19955. Ref T920. Ref T5969. Update Postmark to accept new Message objects. Also:
- Update the inbound whitelist.
- Add a little support for `media` configuration.
- Add a service call timeout (see T5969).
- Drop the needless word "Implementation" from the Adapter class tree. I could call these "Mailers" instead of "Adapters", but then we get "PhabricatorMailMailer" which feels questionable.
Test Plan: Used `bin/mail send-test` to send mail via Postmark with various options (mulitple recipients, text vs html, attachments).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5969, T920
Differential Revision: https://secure.phabricator.com/D19956
Summary:
Ref T920. Over time, mail has become much more complex and I think considering "mail", "sms", "postcards", "whatsapp", etc., to be mostly-the-same is now a more promising avenue than building separate stacks for each one.
Throw away all the standalone SMS code, including the Twilio config options. I have a separate diff that adds Twilio as a mail adapter and functions correctly, but it needs some more work to bring upstream.
This permanently destroys the `sms` table, which no real reachable code ever wrote to. I'll call this out in the changelog.
Test Plan:
- Grepped for `SMS` and `Twilio`.
- Ran storage upgrade.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19939
Summary:
Depends on D19906. Ref T13222. This isn't going to win any design awards, but make the "wait" and "answered" elements a little more clear.
Ideally, the icon parts could be animated Google Authenticator-style timers (but I think we'd need to draw them in a `<canvas />` unless there's some clever trick that I don't know) or maybe we could just have the background be like a "water level" that empties out. Not sure I'm going to actually write the JS for either of those, but the UI at least looks a little more intentional.
Test Plan:
{F6070914}
{F6070915}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19908
Summary:
Depends on D19903. Ref T13222. This was a Facebook-specific thing from D6202 that I believe no other install ever used, and I'm generally trying to move away from the old "event" system (the more modern modular/engine patterns generally replace it).
Just drop support for this. Since the constant is being removed, anything that's actually using it should break in an obvious way, and I'll note this in the changelog.
There's no explicit replacement but I don't think this hook is useful for anything except "being Facebook in 2013".
Test Plan:
- Grepped for `TYPE_AUTH_WILLLOGIN`.
- Logged in.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19904
Summary:
Depends on D19919. Ref T11351. This method appeared in D8802 (note that "get...Object" was renamed to "get...Transaction" there, so this method was actually "new" even though a method of the same name had existed before).
The goal at the time was to let Harbormaster post build results to Diffs and have them end up on Revisions, but this eventually got a better implementation (see below) where the Harbormaster-specific code can just specify a "publishable object" where build results should go.
The new `get...Object` semantics ultimately broke some stuff, and the actual implementation in Differential was removed in D10911, so this method hasn't really served a purpose since December 2014. I think that broke the Harbormaster thing by accident and we just lived with it for a bit, then Harbormaster got some more work and D17139 introduced "publishable" objects which was a better approach. This was later refined by D19281.
So: the original problem (sending build results to the right place) has a good solution now, this method hasn't done anything for 4 years, and it was probably a bad idea in the first place since it's pretty weird/surprising/fragile.
Note that `Comment` objects still have an unrelated method with the same name. In that case, the method ties the `Comment` storage object to the related `Transaction` storage object.
Test Plan: Grepped for `getApplicationTransactionObject`, verified that all remaining callsites are related to `Comment` objects.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19920
Summary:
Depends on D19918. Ref T11351. In D19918, I removed all calls to this method. Now, remove all implementations.
All of these implementations just `return $timeline`, only the three sites in D19918 did anything interesting.
Test Plan: Used `grep willRenderTimeline` to find callsites, found none.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19919
Summary:
Ref T13222. Ref T13225. We store a digest of the session key in the session table (not the session key itself) so that users with access to this table can't easily steal sessions by just setting their cookies to values from the table.
Users with access to the database can //probably// do plenty of other bad stuff (e.g., T13134 mentions digesting Conduit tokens) but there's very little cost to storing digests instead of live tokens.
We currently digest session keys with HMAC-SHA1. This is fine, but HMAC-SHA256 is better. Upgrade:
- Always write new digests.
- We still match sessions with either digest.
- When we read a session with an old digest, upgrade it to a new digest.
In a few months we can throw away the old code. When we do, installs that skip upgrades for a long time may suffer a one-time logout, but I'll note this in the changelog.
We could avoid this by storing `hmac256(hmac1(key))` instead and re-hashing in a migration, but I think the cost of a one-time logout for some tiny subset of users is very low, and worth keeping things simpler in the long run.
Test Plan:
- Hit a page with an old session, got a session upgrade.
- Reviewed sessions in Settings.
- Reviewed user logs.
- Logged out.
- Logged in.
- Terminated other sessions individually.
- Terminated all other sessions.
- Spot checked session table for general sanity.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13225, T13222
Differential Revision: https://secure.phabricator.com/D19883
Summary: Fixes T13218. We have no more callers to any of this and can get rid of it forever.
Test Plan: Grepped for all four API methods, `LiskDAOSet`, and `inSet`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13218
Differential Revision: https://secure.phabricator.com/D19879
Summary: Ref T13218. This is the last public-facing API call for `loadRelatives/loadOneRelative`. This just "primed" objects to make the other calls work and had no direct effects.
Test Plan:
- Ran `bin/fact analyze`.
- Used `bin/storage upgrade -f --apply` to apply `20181031.board.01.queryreset.php`, which uses `LiskMigrationIterator`.
- Browsed user list.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13218
Differential Revision: https://secure.phabricator.com/D19878
Summary:
Ref T13222. See PHI683. Currently, you can "Change subtype..." via Conduit and the bulk editor, but not via the comment action stack or edit forms.
In PHI683 an install is doing this often enough that they'd like it to become a first-class action. I've generally been cautious about pushing this action to become a first-class action (there are some inevitable rough edges and I don't want to add too much complexity if there isn't a use case for it) but since we have evidence that users would find it useful and nothing has exploded yet, I'm comfortable taking another step forward.
Currently, `EditEngine` has this sort of weird `setIsConduitOnly()` method. This actually means more like "this doesn't show up on forms". Make it better align with that. In particular, a "conduit only" field can already show up in the bulk editor, which is goofy. Change this to `setIsFormField()` and convert/simplify existing callsites.
Test Plan:
There are a lot of ways to reach EditEngine so this probably isn't entirely exhaustive, but I think I got pretty much anything which is likely to break:
- Searched for `setIsConduitOnly()` and `getIsConduitOnly()`, converted all callsites to `setIsFormField()`.
- Searched for `setIsLockable()`, `setIsReorderable()` and `setIsDefaultable()` and aligned these calls to intent where applicable.
- Created an Almanac binding.
- Edited an Almanac binding.
- Created an Almanac service.
- Edited an Almanac service.
- Edited a binding property.
- Deleted a binding property.
- Created and edited a badge.
- Awarded and revoked a badge.
- Created and edited an event.
- Made an event recurring.
- Created and edited a Conpherence thread.
- Edited and updated the diff for a revision.
- Created and edited a repository.
- Created and disabled repository URIs.
- Created and edited a blueprint.
- Created and edited tasks.
- Created a paste, edited/archived a paste.
- Created/edited/archived a package.
- Created/edited a project.
- Made comments.
- Moved tasks on workboards via comment action stack.
- Changed task subtype via comment action stack.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19842
Summary:
Ref T13216. See PHI916. Harbormaster builds may be long-running, particularly if they effectively wrap `ssh ... ./run-huge-build.sh`. If we spend more than a few seconds waiting for futures to resolve, close idle database connections.
The general goal here is to reduce the held connection load for installs with a very large number of test runners.
Test Plan: Added debugging code to `phlog()` closures, saw connections closed while running builds.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19824
Summary: Depends on D19810. Ref T13217. Ref T13216. I mostly used `grep implode | grep OR` and `grep implode | grep AND` to find these -- not totally exhaustive but should be a big chunk of the callsites that are missing `%LO` / `%LA`.
Test Plan:
These are tricky to test exhaustively, but I made an attempt to hit most of them:
- Browsed Almanac interfaces.
- Created/browsed Calendar events.
- Enabled/disabled/showed the lock log.
- Browsed repositories.
- Loaded Facts UI.
- Poked at Multimeter.
- Used typeahead for users and projects.
- Browsed Phriction.
- Ran various fulltext searches.
Not sure these are reachable:
- All the lint stuff might be dead/unreachable/nonfunctional?
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13217, T13216
Differential Revision: https://secure.phabricator.com/D19814
Summary:
Ref T13216. See PHI970. Ref T13054. See some discussion in T13216.
When a Harbormaster Buildable object is first created for a Diff, it has no `containerPHID` since the revision has not yet been created.
We later (after creating a revision) send the Buildable a message telling it that we've added a container and it should re-link the container object.
Currently, we send this message in `applyExternalEffects()`, which runs inside the Differential transaction. If Harbormaster races quickly enough, it can read the `Diff` object before the transaction commits, and not see the container update.
Add a `didCommitTransaction()` callback after the transactions commit, then move the message code there instead.
Test Plan:
- See T13216 for substantial evidence that this change is on the right track.
- Before change: added `sleep(15)`, reproduced the issue reliably.
- After change: unable to reproduce issue even with `sleep(15)` (the `containerPHID` always populates correctly).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216, T13054
Differential Revision: https://secure.phabricator.com/D19807
Summary:
Ref T13216. Ref T13217. Depends on D19800. This fixes all of the remaining query warnings that pop up when you run "arc unit --everything".
There's likely still quite a bit of stuff lurking around, but hopefully this covers a big set of the most common queries.
Test Plan: Ran `arc unit --everything`. Before change: lots of query warnings. After change: no query warnings.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217, T13216
Differential Revision: https://secure.phabricator.com/D19801
Summary: Depends on D19789. Ref T13217. Continue updating things to use the new %Q-flavored conversions instead of smushing a bunch of strings together.
Test Plan: Browsed around, far fewer errors. These changes are largely mechanical in nature.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19790
Summary: Depends on D19785. Ref T13217. This converts many of the most common clause construction pathways to the new %Q / %LQ / %LO / %LA / %LJ semantics.
Test Plan: Browsed around a bunch, saw fewer warnings and no obvious behavioral errors. The transformations here are generally mechanical (although I did them by hand).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: hach-que
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19789
Summary: Depends on D19784. Ref T13217. Reduce uses of unsafe `%Q` in SELECT construction.
Test Plan: This reduces the number of safety warnings when loading Phabricator home from ~900 to ~800.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19785
Summary:
Ref T13217. This method is slightly tricky:
- We can't safely return a string: return an array instead.
- It no longer makes sense to accept glue. All callers use `', '` as glue anyway, so hard-code that.
Then convert all callsites.
Test Plan: Browsed around, saw fewer "unsafe" errors in error log.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19784
Summary: Depends on D19796. Simplify some timing code by using phutil_microseconds_since() instead of duplicate casting and arithmetic.
Test Plan: Grepped for `1000000` to find these. Pulled, pushed, made a conduit call. This isn't exhaustive but it should be hard for these to break in a bad way since they're all just diagnostic.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19797
Summary:
Ref T13210. Ref T11908. Add some basic test coverage for the new "%R" introduced in D19764, then convert LiskDAO to implement the "Database + Table Ref" interface.
To move forward, we need to convert all of these (where `%T` is not a table alias):
```counterexample
qsprintf($conn, '... %T ...', $thing->getTableName());
```
...to these:
```
qsprintf($conn, '... %R ...', $thing);
```
The new code is a little simpler (no `->getTableName()` call) which is sort of nice. But we also have a //lot// of `%T` so this is probably going to take a while.
(I'll hold this until after the release cut.)
Test Plan:
- Ran unit tests.
- Browsed around and edited some objects without issues. This change causes a reasonably large percentage of our total queries to use the new code since the LiskDAO builtin queries are some of the most commonly-constructed queries, although there are still ~700 callsites which need to be examined for possible conversion.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210, T11908
Differential Revision: https://secure.phabricator.com/D19765
Summary:
Ref T13210. See PHI930. This translation is wrong: the parameter is a comma-separated list as a string, but the USEnglish translation provides alternatives. We can't select among alternatives based on a random string (it isn't a plurality value to let us select "chair" vs "chairs", and isn't a gender value to let us select "his profile" vs "her profile") so we get an error.
But the string itself is also misleading, since "bin/phd log --id A --id B --id C" will say "none of these are valid" if //any// of them are invalid.
Instead, just tell the user explicitly about the first problem.
Test Plan:
- Ran `bin/phd log --id` with good (got logs) and bad IDs (got sensible error).
- Ran `bin/phd log` with any logs (got logs) and (simluated) without any logs (got error).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19755
Summary:
Ref T13210. Minor usability improvements to "bin/bulk export":
- Allow `--class task` to work (previously, only `--class ManiphestTaskSearchEngine` worked).
- If you run `--query jXIlzQyOYHPU`, don't require `--class`, since the query identifies the class on its own.
- Allow users to call `--query A --query B --query C` and get a union of all results.
Test Plan:
- Ran `--class task`, `--query A --query B`, `--query X` (with no `--class`), got good results.
- Ran various flavors of bad combinations (queries from different engines, invalid engines, query and class differing, ambiguous/invalid `--class` name) and got sensible errors.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19738
Summary:
Ref T13202. See PHI889. If the lock log is enabled, we can try to offer more details about lock holders.
When we fail to acquire a lock:
- check for recent acquisitions and suggest that this is a bottleneck issue;
- if there are no recent acquisitions, check for the last acquisition and print details about it (what process, how long ago, whether or not we believe it was released).
Test Plan:
- Enabled the lock log.
- Changed the lock wait time to 1 second.
- Added a `sleep(10)` after grabbing the lock.
- In one window, ran a Conduit call or a `git fetch`.
- In another window, ran another operation.
- Got useful/sensible errors for both ssh and web lock holders, for example:
> PhutilProxyException: Failed to acquire read lock after waiting 1 second(s). You may be able to retry later. (This lock was most recently acquired by a process (pid=12609, host=orbital-3.local, sapi=apache2handler, controller=PhabricatorConduitAPIController, method=diffusion.rawdiffquery) 3 second(s) ago. There is no record of this lock being released.)
> PhutilProxyException: Failed to acquire read lock after waiting 1 second(s). You may be able to retry later. (This lock was most recently acquired by a process (pid=65251, host=orbital-3.local, sapi=cli, argv=/Users/epriestley/dev/core/lib/phabricator/bin/ssh-exec --phabricator-ssh-device local.phacility.net --phabricator-ssh-key 2) 2 second(s) ago. There is no record of this lock being released.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19702
Summary:
Ref T13195. Fixes T8573. When you're adding inlines to your own stuff, mark them "Done" by default. You can unmark them as "Done" if you're legitimately leaving TODOs for yourself, although I think this is unusual.
(If this turns out to be less unusual than I think, we could consider an alternate rule: mark replies by the author as "Done" by default.)
Test Plan: Added some inlines as an author and a non-author. Saw my author inlines marked as "Done" by default. Submitted them; unmarked and submittted them.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195, T8573
Differential Revision: https://secure.phabricator.com/D19635
Summary:
Ref T13195. Ref T8573. This allows reviewers to mark their own inline comments as "Done" before they submit them.
If you're leaving a non-actionable comment like "this is good", you can pre-check "Done" to give the author a hint that you don't expect any response.
Test Plan: On revisions and commits, added inlines as the author and a reviewer/auditor. Marked them done/not-done before submitting. As author, marked the not-done ones done after submitting. Checked preivews, toggled done/not done states.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195, T8573
Differential Revision: https://secure.phabricator.com/D19634
Summary:
Ref T13195. Ref T8573. The inline comment controllers currently use outdated `$user = $this->getRequest()->getUser()` calls.
Instead, use `$viewer = $this->getViewer()`.
This is just a small consistency update with no behavioral changes.
Test Plan: Viewed and added inlines in Differential and Diffusion.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195, T8573
Differential Revision: https://secure.phabricator.com/D19633
Summary:
Ref T13164. See PHI823. (See that issue for some more details and discussion.)
Add aural labels to various buttons which were missing reasonable aural labels.
The "Search" button (magnifying glass in the global search input) had an entire menu thing inside it. I moved that one level up and it doesn't look like it broke anything (?). All the other changes are pretty straightforward.
Test Plan:
{F5806497}
{F5806498}
- Will follow up on the issue to make sure things are in better shape for the reporting user.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19594
Summary:
Ref T13164. PHI805 incidentally includes some `bin/storage probe` output for 100GB+ tables which renders wrong.
We have the tools to render it properly, so stop doing this manually and let ConsoleTable figure out the alignment.
Test Plan:
Faked very large table sizes, ran `bin/storage probe`:
{F5785946}
(Then, un-faked the very large table sizes and ran it again, got sensible output.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19567
Summary:
See PHI785. Ref T13164. In this case, an install wants to receive mail via Mailgun, but not configure it (DKIM + SPF) for outbound mail.
Allow individual mailers to be marked as not supporting inbound or outbound mail.
Test Plan:
- Added and ran unit tests.
- Went through some mail pathways locally, but I don't have every inbound/outbound configured so this isn't totally conclusive.
- Hit `bin/mail send-test` with a no-outbound mailer.
- I'll hold this until after the release cut so it can soak on `secure` for a bit.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19546
Summary: I've pulled up this code probably three different times to make sure that the big scary warning does, in fact, still get printed even when passing `--unitest-fixtures` to `bin/storage destroy`. Make the warning message less scary if only removing test data.
Test Plan: Ran with and without `--unitest-fixtures` and saw expected warnings. After agreeing to warnings, test data was deleted as expected. Did not test `bin/storage destroy` without `--unittest-fixtures`.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19535
Summary:
Ref PHI778. In D18492, I added support for parsing this operator, but did not actually implement it in the query engine.
Implementation is fairly straightforward. This supports querying for objects by exact title with `title:="exact title"`. This is probably a bad idea, but sometimes maybe useful anyway.
Test Plan: Queried for `title:="xxx"`, found only exact matches.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: ahoffer2
Differential Revision: https://secure.phabricator.com/D19529
Summary:
Ref T13168. I'm not sure how this worked before, but I ran into this issue on my new laptop.
SiteSource accesses `PhabrictatorEnv::getEnvConfig('phabricator.base-uri')` when local, which may poison the cache and lock the value since we don't later discard the cache.
Specifically, when I access `http://locala.phacility.com`, I was getting an error like "You made a request for locala.phacility.com, but no configured site can serve this request.". This was because the base-uri was being incorrectly frozen as "local.phacility.com". The expectation is that it will match, so the standard PlatformSite will serve the request.
Test Plan:
- Before change: "no configured site" error.
- After change: local instance works properly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13168
Differential Revision: https://secure.phabricator.com/D19526
Summary:
See PHI746. See also T11833, perhaps. Ref T13151.
Long ago, parent revisions were called "dependent revisions". This was changed to "parent revisions" in the action UI to improve clarity, but not changed in the timeline stories.
Update the timeline stories to use the same language the actions in the UI use.
Test Plan:
{F5732876}
{F5732877}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19514
Summary: Ref T13152. The pager does a bit of magic here and doesn't populate `nextPageID` when it knows it got an exact final page. The logic misfired in this case and sent us back to the start.
Test Plan:
- Set page size to 1 to guarantee rows were an exact multiple of page size.
- Ran `rebuild-identities` (I no-op'd the actual logic to make it faster).
- Before: looped forever.
- After: clean exit after processing everything.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13152
Differential Revision: https://secure.phabricator.com/D19479
Summary:
Ref T13151. See PHI654. Depends on D19477. If you have long package names, the table of contents (e.g., in Differential) can end up expanding to be gigantic.
Getting tables to behave nicely is hard (or, at least, I can't figure it out after spending a decent amount of time on it; see also `AphrontTableView::renderSingleDisplayLine()`). I tried a bunch of things and Googled for a bit but didn't make any progress on finding a CSS solution. Just truncate the package names to get reasonable behavior without falling down any kind of CSS rabbit hole.
Test Plan:
- Created a package named "Very long package name...".
- Created a package named "MMMMMMMMMMMMMMMMMMMMMM...".
- Had them own a file in a Differential revision, viewed that revision.
- Before: table is pushed out to several times the browser window width and everything is kind of a mess.
- After: package names get truncated to something reasonable.
{F5652953}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19478
Summary:
Ref T13141. Currently, during first-time setup we don't surface all the details about connection exceptions that we could: the underlying exception is discarded inside cluster connection management.
This isn't a huge issue since the reason for connection problems is usually fairly obvious, but in at least one case (see T13141) we hit a less-than-obvious exception.
Instead, store the original exception and propagate the message up the stack so users have more information about the problem.
Test Plan:
- Configured an intentionally bad MySQL username.
- Restarted Apache and loaded Phabricator.
- Got a more helpful exception with a specific authentication error message.
{F5622361}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13141
Differential Revision: https://secure.phabricator.com/D19454
Summary:
See D19446. This should make it easier to process larger, more complex result sets in constant memory.
Today, `LiskMigrationIterator` takes constant memory but can't apply `needX()` reqeusts or `withY(...)` constraints.
Using a raw `Query` can handle this stuff, but requires memory proportional to the size of the result set.
Offer the best of both worlds: constant memory and full access to the power of `Query` classes.
Test Plan:
Used this script to iterate over every commit, saw sensible behavior:
```name=list-commits.php
<?php
require_once 'scripts/init/init-script.php';
$viewer = PhabricatorUser::getOmnipotentUser();
$query = id(new DiffusionCommitQuery())
->setViewer($viewer);
$iterator = new PhabricatorQueryIterator($query);
foreach ($iterator as $commit) {
echo $commit->getID()."\n";
}
```
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19450
Summary:
See <https://discourse.phabricator-community.org/t/maniphest-non-integer-point-values-in-csv-export/1443>.
We currently export the Maniphest "points" field as an integer, but allow it to accept decimal values (e.g. "6.25").
Also fix a bug where we wouldn't roll over from "..., X, Y, Z, AA, AB, ..." correctly for Excel column names if sheet had more than 26 columns.
Test Plan:
- Set a task point value to 6.25.
- Exported to text, JSON, XLS.
- Saw 6.25 represented accurately in exports.
- Exported an excel sheet with 27+ columns.
- Manually printed the first 200 column names to check that the algorithm looks correct.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19434
Summary:
Ref T13130. See PHI483. Currently, "Plan Changes + Draft" uses rules like "Plan Changes", not rules like "Draft", and allows "Accept".
This isn't consistent with how "Draft" and "Accept" work in other cases. Make "Plan Changes + Draft" more like "Draft" for consistency.
Also fix a string that didn't have a natural English version.
Test Plan:
- Added a failing build plan.
- Created a revision.
- Loaded the revision before builds completed, saw a nicer piece of text about "waiting for builds" instead of "waiting for 2 build(s)".
- Builds failed, which automatically demoted the reivsion to "Changes Planned + Draft".
- As the author and as a reviewer, verified all the actions available to me made sense (particularly, no "Accept").
- Abandoned the revision to test "Abandoned + Draft".
- As the author and as a reviewer, verified all the actions available to me made sense.
- Reclaimed the revision, then used "Request Review" to send it to "Needs Review". Verified that actions made sense and, e.g., reviewers could now "Accept" normally.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19398
Summary:
Ref T13124. Ref T13131. Fixes T8953. See PHI512.
When you receieve a notification about an object and then someone hides that object from you (or deletes it), you get a phantom notification which is very difficult to clear.
For now, test that notifications are visible when you open the menu and clear any that are not.
This could be a little more elegant than it is, but the current behavior is very clearly broken. This unbreaks it, at least.
Test Plan:
- As Alice, configured task stuff to notify me (instead of sending email).
- As Bailey, added Alice as a subscriber to a task, then commented on it.
- As Alice, loaded home and saw a notification count. Didn't click it yet.
- As Bailey, set the task to private.
- As Alice, clicked the notification bell menu icon.
- Before change: no unread notifications, bell menu is semi-stuck in a phantom state which you can't clear.
- After change: bad notifications automatically cleared.
{F5530005}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13131, T13124, T8953
Differential Revision: https://secure.phabricator.com/D19384
Summary: Ref T13116. See PHI526. Currently, the YouTube remarkup rule writes an `<iframe ...>` but does not adjust the Content-Security-Policy appropriately.
Test Plan: Pasted a YouTube link; viewed it in Safari, Chrome and Firefox.
Maniphest Tasks: T13116
Differential Revision: https://secure.phabricator.com/D19277
