Summary:
Ref T13591. Some storage patches queue worker tasks, currently always to rebuild search indexes.
These patches can not execute in creation order if a later patch modifies the worker task table, since they'll try to perform a modern INSERT against an out-of-date table schema. Such a modification is desirable in the context of T13591, but making it causes these patches to fail.
Patches have an existing "after" mechanism which allows them to have explicit dependencies. This mechanism could be used to resolve this issue, but all patches with a dependency like this would need to be updated every time the queue table changes.
Instead, introduce "phases" to provide broader ordering rules. There are now two phases: "default" and "worker". Patches in the "worker" phase execute after patches in the "default" phase.
Phases may eventually be further separated, but
Test Plan:
- Ran `bin/storage status`, saw patches annotated with phases.
- Will apply `containerPHID` changes on top of this.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21529
Summary: Ref T13591. This is mostly a workaround for Big Sur not having pcntl/posix installed by default and the mess with M1 / Homebrew / SIP / Code Signing (see T13232) so I can't easily run actual daemons and need to fake them with `bin/worker execute --active`, but it's a reasonable flag on its own.
Test Plan:
- Ran `bin/worker execute --active` and `bin/worker cancel --active`.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21517
Summary: These characters are missing support in `{key ...}` but are reasonable to include.
Test Plan: {F8302969}
Differential Revision: https://secure.phabricator.com/D21508
Summary:
See <https://discourse.phabricator-community.org/t/mail-details-view-broken/4315>. The change in D21400 detects a missing "GROUP BY" in some variations of this query.
Specifically, we may join multiple recipient rows (since mail may have multiple recipients) and then fail to group the results.
Fix this by adding the "GROUP BY". Additionally, remove the special-cased behavior when no authors or recipients are specified -- it's complicated and not entirely correct (e.g., may produce a "no object" instead of a policy error when querying by ID), and likely predates overheating.
Test Plan:
- Disabled `metamta.one-mail-per-recipient` in Config.
- Generated a message to 2+ recipients.
- Viewed the message detail; queried for the message by specifying 2+ recipients.
- Viewed the unfiltered list of messages, saw the query overheat.
Differential Revision: https://secure.phabricator.com/D21486
Summary:
See PHI1876. Normally, deleted inlines are undeleted with an "undelete" operation, which clears the "isDeleted" flag.
However, when an inline is deleted implicitly by using "Cancel" without first saving it, the flag currently isn't cleared properly. This can lead to cases where inlines seem to vanish (they are shown to the user in the UI, but treated as deleted on submission).
Test Plan:
There are two affected sequences here:
- Create a new inline, type text, cancel, undo.
- Create a new inline, type text, cancel, undo, save.
The former sequence triggers an "edit" operation. The subsequent "Save" in the second sequence triggers a "save" operation.
It's normally impossible in the UI to execute a "save" without executing an "edit" first, but "save" clearly should undelete the comment if you get there somehow, so this change clears the deleted flag in both cases for completeness.
- Executed both sequences, saw comment persist in preview, on reload, and after submission.
Differential Revision: https://secure.phabricator.com/D21483
Summary:
See PHI1900. Recent changes to how commit graphs are drawn made the height automatic in most cases, but it fails in Differential because the element isn't initially visible so the computed height is 0.
Just give them an explicit height so they show up again.
Test Plan: Viewed graphs in Maniphest, Differential, and Diffusion; saw them all render properly.
Differential Revision: https://secure.phabricator.com/D21481
Summary:
See PHI1896. If you do this:
- Create an inline comment over a wide range of lines.
- Suggest an edit.
- Make a change near the beginning of the block.
- Make a change near the end of the block.
- Save the inline.
...you get a rendering which includes a "Show More Context" fold in the middle.
Currently, this element renders in a visually broken way and consumes too many columns.
However, this element isn't ever desirable inside inline comment suggestions. Stop it from rendering entirely.
Test Plan:
- Made an inline comment suggestion across lines 1-50 with edits at the beginning and end, saw a contiguous diff.
- Made smaller inline comment suggestions (one line, a few lines).
Differential Revision: https://secure.phabricator.com/D21476
Summary:
Ref T13552. Fixes T13569. Currently, if a process uses in-process tasks (usually, a debugging/diagnostic workflow) and those tasks (or tasks those tasks queue) fail permanently, the exception escapes to top level and the process exits.
This isn't desirable; catch the exception and fail them locally instead.
Test Plan:
With a failing Asana integration and misconfigured Webhook, ran `bin/repository reparse --publish ...`.
- Before: fatals on each substep.
- After: warnings emitted for failed substep, but process completes.
Maniphest Tasks: T13569, T13552
Differential Revision: https://secure.phabricator.com/D21459
Summary: Ref T13577. After the fix in D21453, lint identifies additional static errors in Phabricator; fix them.
Test Plan: Ran `arc lint`; these messages are essentially all very obscure.
Subscribers: hach-que, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13577
Differential Revision: https://secure.phabricator.com/D21457
Summary: Ref T13579. This property was removed in D21425, but I missed this usage site. Remove the assignment; this class no longer tracks the subprocess PID directly.
Test Plan: Searched for "->pid", no further hits.
Maniphest Tasks: T13579
Differential Revision: https://secure.phabricator.com/D21452
Summary:
Ref T13573. Using the browser "Print" feature on pages produces "Thu, Aug 4, 12:22" timestamps which require context to interpret precisely (they don't have a year and don't have a timezone).
Instead, retain these timestamps in "screen" contexts but use "YYYY-MM-DD HH:MM:SS (UTC+X)" timestamps when printing.
Test Plan: Printed Maniphest tasks and other pages in Safari and Chrome using "?__print__=1" and "Print to PDF", saw absolute timestamps after this chagne in the printed documents.
Maniphest Tasks: T13573
Differential Revision: https://secure.phabricator.com/D21451
Summary:
See PHI1809, which identified a bug in Project search where queries with a large number of slugs could paginate improperly.
This change detects problems in this category: cases where multiple rows with the same ID are passed to "loadAllFromArray()". It's likely that all cases it detects are cases where a GROUP BY is missing.
Since this might have some false positives or detect some things which aren't fundamentally problematic, I'm planning to hold it until the next release.
Test Plan:
- Reverted D21399, then created a project with multiple slugs and queried for one of them via "project.search". Hit this new exeception.
- Browsed around a bit, didn't immediately catch any collateral damage.
Differential Revision: https://secure.phabricator.com/D21400
Summary: Ref T13552. In the new combined "table/list" graph view, tidy up the graph rendering.
Test Plan: {F7633504}
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21411
Summary:
See PHI1834. It's not obvious why this "+1" is present in the code, but it causes inlines to be adjusted incorrectly when a file is not modified across changes. See D21435.
Remove it, which appears to produce accurate adjustment behavior.
Test Plan:
- See D21435 for instructions to build a change, where a file with lines "A-Z" is unmodified across Diff 1 and Diff 2.
- Left inlines on lines 14, 17-19, and 16-26 (end of the file) on Diff 1.
- Before: saw inlines incorrectly adjusted to lines 15, 18, and 17 on Diff 2. Before D21435, the last inline was culled by the rendering engine.
- After: saw inlines correctly adjusted to lines 14, 17, and 16 (the same lines as the original), render properly, and highlight the correct lines when hovered.
Differential Revision: https://secure.phabricator.com/D21436
Summary:
Ref T13556. These options are very old and effectively obsoleted by "bin/phd debug [--trace]". I haven't used either option diagnostically in many years, and they aren't mentioned in the documentation.
Remove them to simplify configuration, and because "phd.trace" doesn't work anyway and likely hasn't for a long time -- it has specific issues with TTY detection (see T13556).
Test Plan: Grepped for "phd.trace" and "phd.verbose". Ran "bin/phd debug [--trace]" and saw verbose/trace output.
Maniphest Tasks: T13556
Differential Revision: https://secure.phabricator.com/D21426
Summary:
Ref T13555. Currently, the daemon future may resolve into a failure state immediately inside "start()", and not have a valid PID when we read it.
Instead, read PIDs from the current active future in all cases, using "hasPID()" to test for the presence of a valid PID.
Since we don't query the PID immediately, we no longer need to explicitly start the future.
Also fix an issue where the same future could be added to the overseer pool more than once if it threw on "resolve()". In general:
- Before we "resolve()" a future, detach it from the DaemonHandle: we're always done with it.
- Catch exceptions on resolution and treat them the same way as subprocess resolution errors. These aren't common, but are possible in the general case.
- Have DaemonHandle add futures to the future pool directly when they're created.
Test Plan:
- Ran daemons with intentional subprocess creation failures, saw clean recovery.
- Ran daemons with intentional resolution exceptions, saw clean recovery.
Maniphest Tasks: T13555
Differential Revision: https://secure.phabricator.com/D21425
Summary:
Ref T13555. Although these callsites may not actually impact anything, it's possible for an active handle to have no PID (e.g., if the subprocess failed to start).
Handle these cases more carefully.
Test Plan: Started daemons, saw them run fine. See also next change.
Maniphest Tasks: T13555
Differential Revision: https://secure.phabricator.com/D21424
Summary:
Fixes T13554. For certain prose diff inputs and PCRE backtracking limits, this regular expression may back track too often and fail.
A characteristic input is "x x x x ...", i.e. many sequences where `(.*?)\s*\z` looks like it may be able to match but actually can not.
I think writing an expression which has all the behavior we'd like without this backtracking issue isn't trivial (at least, I don't think I know how to do it offhand); just use a strategy based on "trim()" insetad, which avoids any PCRE complexities here.
Test Plan: Locally, this passes the "x x x ..." test which the previous code failed. I'm not including that test because it won't reproduce across values of "pcre.backtrac_limit", PCRE versions, etc.
Maniphest Tasks: T13554
Differential Revision: https://secure.phabricator.com/D21422
Summary:
See PHI1794, which describes a connection exhaustion issue with a large number of webhook tasks in queue.
The "GlobalLock" mechanism manages a separate connection pool from the main pool, and webhook workers immediately try to grab a webhook lock with a 0-second wait when they start. So far, this is fine.
Prior to this change, good connections which fail to acqiure a lock are discarded. This can lead to connection exhaustion as the worker rapidly cycles through lock attempts: the connections will remain open for at least 60 seconds (since D16389) in an effort to avoid outbound port exhaustion, but they're effectively orphaned because they aren't part of the main pool and aren't part of the lock pool. We're basically leaking a connection every time we fail to lock.
Failing to lock doesn't mean we need to discard the connection: it's a completely suitable connection for reuse. Instead of dropping it on the floor, put it into the lock pool.
Test Plan:
- Used "bin/webhook call ... --count 10000 --background" to queue a large number of webhook calls against a slow ("sleep(15);") webhook.
- Used "bin/phd launch 32 taskmaster" to start taskmasters.
- Observed MySQL connection behavior:
- Before change: 2048 configured connections immediately exhausted.
- After change: connections stable at ~160ish.
- Ran queue for a while, saw expected single-threaded calls to webhook.
Differential Revision: https://secure.phabricator.com/D21369
Summary:
See <https://discourse.phabricator-community.org/t/bad-regex-in-prose-diff-logic/3969>.
The prose splitting rules normally guarantee that newlines appear only at the beginning or end of blocks. However, if a prose sentence ends with text like "...x\n.", we can end up with a newline inside a "sentence".
If we do, the regular expression that breaks it into pieces will fail.
Arguably, this is an error in how sentences are split apart (we might prefer to split this into two sentences, "x\n" and ".", rather than a single "x\n." sentence) but in the general case it's not unreasonable for blocks to contain newlines, so a simple fix is to make the pattern more robust.
Test Plan: Added a failing test which includes this behavior, made it pass.
Differential Revision: https://secure.phabricator.com/D21295
Summary:
Ref T13513. Syntax highlighting is potentially expensive, and the changeset rendering pipeline can cache it. However, the cache is currently keyed ONLY by Differential changeset ID.
Destroy the existing cache and rebuild it with a more standard cache key so it can be used in a more ad-hoc way by inline suggestion snippets.
Test Plan: Used Darkconsole, saw cache hits and no more inline syntax highlighting for changesets with many inlines.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21280
Summary: Ref T13513. Inline comment context information is somewhat expensive to construct and can be cached. Add a readthrough cache on top of it.
Test Plan: Loaded a source code changeset with many inline comments, used Darkconsole to inspect query activity. Saw caches get populated. Updated cache key, saw caches regenerate. Browsed Diffusion, nothing looked broken.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21279
Summary: Ref T13513. For now, I'm not supporting inline edit suggestions in Diffusion, although it's likely not difficult to do so in the future. Clean up some of the code so that plain ol' inlines work the same way they did before.
Test Plan:
- Created, edited, reloaded, submitted inlines in Diffusion: familiar old behavior.
- Created, edited, reloaded, submitted inlines with suggestions in Differential: familiar new behavior.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21278
Summary: Ref T13513. When rendering an inline suggestion for display, use highlighting and diffing.
Test Plan: {F7495053}
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21277
Summary:
Ref T13513. This still has quite a few rough edges and some significant performance isssues, but appears to mostly work.
Allow reviewers to "Suggest Edit" on an inline comment and provide replacement text for the highlighted source.
Test Plan: Created, edited, reloaded, and submitted inline comments in various states with and without suggestion text.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21276
Summary: Ref T13513. Introduce a formal server-side content state object so the whole state can be saved and restored to the drafts table, read from the request, etc.
Test Plan: Created and edited inlines. Reloaded drafts with edits. Submitted normal and editing comments. Grepped for affected symbols.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21275
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
Summary: See PHI1745. Under PHP7, errors raised as Throwable miss this "generic exception" logic and don't increment their failure count. Instead, treat any "Throwable" we don't recognize like any "Exception" we don't recognize.
Test Plan:
- Under PHP7, caused a worker task to raise a Throwable (e.g., call to undefined method, see D21270).
- Ran `bin/worker execute --id ...`.
- Before: worker failed, but did not increment failure count.
- After: worker fails and increments failure count as it would for other types of unknown error.
Differential Revision: https://secure.phabricator.com/D21271
Summary:
Ref T13513. Give selected inlines a selection state and visual cues which are similar to the changeset selection state.
Also fix a couple of minor issues with select interactions and offset comments.
Test Plan: Selected inlines, saw obvious visual cues.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21256
Summary:
Ref T13513. When a user selects a text range and uses "New Inline Comment" to create a comment directly from a range, store the offset information alongside the comment.
When hovering the comment, highlight the original range.
Test Plan: {F7480926, size=full}
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21250
Summary: Ref T11401. Ref T13513. This paves the way for more comment actions, particularly an edit-after-submit action.
Test Plan: Took all actions from menus, via mouse and via keyboard (where applicable).
Maniphest Tasks: T13513, T11401
Differential Revision: https://secure.phabricator.com/D21244
Summary:
Ref T13513. Currently, viewing a Jupyter document, hidden context just gets a plain "* * *" facade with no way to expand it.
Support click-to-expand, like source changes.
Test Plan:
- Clicked to expand various Jupyter diffs.
- Clicked to expand normal source changes.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21243
Summary:
Ref T13513. As part of inline metadata, save the document engine the change is being rendered with.
This will allow other parts of the UI to detect that an inline was created on a Jupyter notebook but is being rendered on raw source, or whatever else.
The immediate goal is to fix nonsensical inline snippet rendering in email on Jupyter notebooks.
Test Plan:
- Created inlines and replies on normal soure code, saw no document engine annotated in the database.
- Created inlines and replies on a Jupyter notebook rendered in Jupyter mode, saw "jupyter" annotations in the database.
- Swapped document engines between Jupyter and Source, etc.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21240
Summary:
Ref T13513. Currently, clicking "View" from the inline comment preview (below the "add comment" area at the bottom of the page) only works if the inline isn't being edited.
Update this behavior so it works on inlines in either "Viewing" or "Editing" states.
Test Plan:
- Clicked "View" on a normal inline, got jumped/selected.
- Clicked "View" on an editing inline, got jumped/selected.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21237
Summary:
Ref T13513. Currently, if you:
- click a line to create an inline;
- type some text;
- wait a moment; and
- close the page.
...you don't get an "Unsubmitted Draft" marker in the revision list.
Lift all the draft behavior to "InlineController" and make saving a draft dirty the overall container draft state.
Test Plan:
- Took the steps described above, got a draft state marker.
- Created, edited, submitted, etc., inlines in Diffusion and Differential.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21235
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
