Summary: Ref T13065. Ref T13641. Migrate "mailKey" and drop the column.
Test Plan: Ran "bin/storage upgrade", got a clean report and saw binding mail keys in the mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641, T13065
Differential Revision: https://secure.phabricator.com/D21630
Summary: Ref T13641. Ref T13065. Migrate and drop the onboard "mailKey" column for Almanac Services.
Test Plan: Ran storage migration, got clean report, saw migrated value in mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641, T13065
Differential Revision: https://secure.phabricator.com/D21629
Summary: Ref T13641. Make "active bindings" a real query and make callers that only care about active bindings only query for active bindings.
Test Plan:
- Queried for "bindings" and "activeBindings" via Conduit.
- Disabled/enabled devices, saw binding status update in UI.
- Loaded Diffusion cluster layout.
- Grepped for `needBindings()`, `getActiveBindings()`, etc.
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21628
Summary:
Ref T13641. Add a "status" property with most of the relevant support code.
This currently has no impact on use of the device or bindings by Diffusion or Drydock: they ignore the status of devices bound to services.
Test Plan:
- Created a new device.
- Changed the status of a device via web and API.
- Queried for devices via API.
- Searched for active and disabled devices.
- Viewed UI in list view, detail view.
- Used typeahead to add a new binding to an interface on a disabled device, got disabled hint in typeahead UI.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21627
Summary: Ref T13641. Provide minor modernizations before adding a "Disabled" state.
Test Plan: Browsed devices, created a new device.
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21626
Summary: Ref T13065. See similar changes attached to that task.
Test Plan: Ran migration, got a clean database state, saw mail keys populate in mail property table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13065, T13641
Differential Revision: https://secure.phabricator.com/D21625
Summary:
Ref T13395. "libphutil/" was stripped for parts, but some documentation still references it. This is mostly minor corrections, but:
- Removes "Javelin at Facebook", long obsolete.
- Removes "php FPM warmup", which was always a prototype and is obsoleted by PHP preloading in recent PHP.
Test Plan: `grep` / reading
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D21624
Summary: Ref T13639. Move Diffusion to use the new API and get rid of the old API now that it no longer has any callers.
Test Plan:
Grepped for remaining callers.
{F8539335}
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21620
Summary:
Ref T13639. There's currently a hard-to-hit bug where editing the "Repository" of a revision doesn't update this index.
Instead: update the index on repository change, not just diff update.
Test Plan:
- Updated a revision, used debug view to see index update.
- Changed repository on a revision, used debug view to see index update.
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21618
Summary:
Ref T13639. Make schema changes:
- Make repositoryID nullable, for revisions with no repository.
- Remove "epoch", which has no readers and no clear use.
- Change the ordering of the key, since "pathID" has more unique values and no queries ever issue without it.
Test Plan:
- Ran `bin/storage upgrade`, got a clean schema.
- Reindexed all revisions with an external script.
- Reviewed index via debug UI, saw appropriate index for non-repositoy revisions.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21617
Summary:
Ref T13639. Move operations related to updating the "AffectedPath" index to a dedicated class.
This change has no functional effect and only moves code.
Test Plan: Used an external script to rebuild every revision index; destroyed a revision with `bin/remove destroy`.
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21616
Summary:
Ref T13639. The "Affected Path" table is currently hard to inspect: there's no UI, and using MySQL just gives you a bunch of IDs.
Add a simple UI and a debug-mode link to it.
Test Plan:
{F8539098}
{F8539099}
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21615
Summary:
Ref T13639. Updating the affected path table has a peculiar side effect from D19426, which is a simplification of a peculiar side effect from earlier.
Don't condition Owners behavior on path index behavior.
Test Plan: Created a revision.
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21614
Summary: See T13639. This change simplifies providing a more modern approach to querying this data via "differential.revision.search".
Test Plan: Called "differential.query" with paths (got an error) and without paths (got a valid query result).
Differential Revision: https://secure.phabricator.com/D21613
Summary:
Ref T13639. In D17754, this:
> OPEN REVISIONS
> Recently updated open revisions affecting this file.
...was simplified into:
> RECENTLY OPEN REVISIONS
This is a bit misleading, since the panel doesn't contain "recently open" results. Use "Recent Open" instead, which is a bit more consistent with other product text. This is still slightly misleading, but probably close enough.
Test Plan: Read text.
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21612
Summary:
Ref T13631. This option has a behavior other than the behavior implied by the name and documented.
Document the correct behavior, at least. This can likely be removed after T10574.
Test Plan: Read config option help in Config.
Maniphest Tasks: T13631
Differential Revision: https://secure.phabricator.com/D21610
Summary: Ref T13623. In D21603, I made the "partial object" this query returns a raw row, which paging keys can no longer be extracted from properly.
Test Plan: Paged notifications to page 2, no longer saw an error.
Maniphest Tasks: T13623
Differential Revision: https://secure.phabricator.com/D21609
Summary:
Fixes T13622. Figuring out what permissions we have seems difficult, so address this a bit more narrowly:
- Make the "access denied" error message a bit more helpful.
- Tailor error handling for the "CREATE TEMPORARY TABLE" statement.
Test Plan:
- Created a new user, granted them "SELECT ON *.*" but not "CREATE TEMPORARY TABLE", ran `bin/storage upgrade --force --apply phabricator:20210215.changeset.02.phid-populate.php`.
- Before: fairly opaque error.
- After: fairly useful error.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13622
Differential Revision: https://secure.phabricator.com/D21608
Summary: Ref T13636. Add routing for "/robots.txt", "favicon.ico", and "/status/" on the ShortSite and BlogSite.
Test Plan: Visted all resources (and 404 pages) on Short and Blog sites, and Platform site.
Maniphest Tasks: T13636
Differential Revision: https://secure.phabricator.com/D21607
Summary:
Fixes T12919. Fixes T13636. Prior to this change, some well-known resource paths don't route on sites like ResourceSite.
- `/robots.txt`: Make it route on ResourceSite and just deny the whole site.
- `/favicon.ico`: Make it route on ResourceSite.
- `/status/`: Make it route on ResourceSite.
- 404: Make it render a 404 on ResourceSite.
Test Plan:
- Visited all URIs on ResourceSite, got sensible responses.
- Visited all URIs on main site.
- Visited 404 while logged out, got login page.
Maniphest Tasks: T13636, T12919
Differential Revision: https://secure.phabricator.com/D21606
Summary:
Ref T13635. Currently, the JSON DocumentEngine uses "phutil_json_decode()", but this can confuse "{}" and "[]".
Be more careful about how the JSON value is decoded, to preserve the distinction.
Test Plan: {F8520479}
Maniphest Tasks: T13635
Differential Revision: https://secure.phabricator.com/D21605
Summary:
Ref T13623. The change in D21577 could lead to a case where we try to access stories the user can't see.
Move the story-loading piece to "willFilterPage()" to make our way thorugh this.
Test Plan:
- Made FeedStory return nothing to simulate invisible notifications, loaded page.
- Before: index access fatal.
- After: clean "no notifications".
- Loaded notifications normally, saw normal notifications.
Maniphest Tasks: T13623
Differential Revision: https://secure.phabricator.com/D21603
Summary: Ref T13632. Users searching for `__FILE__`, etc., almost certainly mean to perform a substring search.
Test Plan: Added tests and made them pass. Searched for various tokens, saw compiler interpretation in UI.
Maniphest Tasks: T13632
Differential Revision: https://secure.phabricator.com/D21602
Summary:
Ref T13631. This supports a more robust version of "poll for updates by using dateModified window queries" that uses transactions as a logical clock.
This is particularly relevant for commits, since they don't have a "dateModified" at time of writing.
Test Plan:
- Queried for transactions by type and object.
- Issued various invalid transaction queries, got appropriate errors.
Maniphest Tasks: T13631
Differential Revision: https://secure.phabricator.com/D21601
Summary: Ref T13631. These strings were a little inconsistent; make them more consistent.
Test Plan: Called `diffusion.commit.search` with the appropriate attachment, saw slightly more consistent statuses.
Maniphest Tasks: T13631
Differential Revision: https://secure.phabricator.com/D21600
Summary:
Ref T13631. Move "PhabricatorAuditStatusConstants" to a more modern object ("PhabricatorAuditRequestStatus").
Expose the status value via Conduit.
Test Plan:
- Ran `bin/audit delete`.
- Viewed a commit with auditors in the web UI.
- Grepped for affected symbols.
- Called Conduit with the "auditors" attachment, saw auditor statuses.
Maniphest Tasks: T13631
Differential Revision: https://secure.phabricator.com/D21599
Summary:
Ref T13631. See that task for discussion.
- "NONE": Probably never used?
- "CC": Obsoleted by subscribers.
- "AUDIT_NOT_REQUIRED": For Owners packages, obsoleted by edges.
- "CLOSED": For "Close Audit", obsoleted by "Request Verification".
Test Plan:
- Grepped for constants, browsed Diffusion.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13631
Differential Revision: https://secure.phabricator.com/D21598
Summary: Ref T13631. For now, this only shows the auditor PHID. The current status constants could use some cleanup before they're exposed.
Test Plan: Queried with "auditors" attachment, saw basic auditor information.
Maniphest Tasks: T13631
Differential Revision: https://secure.phabricator.com/D21597
Summary:
Ref T13629.
- Allow files to have custom alt text.
- If a file doesn't have alt text, try to generate a plausible default alt text with the information we have.
Test Plan:
- Viewed image files in DocumentEngine diffs, files, `{Fxxx}` embeds, and lightboxes.
- Saw default alt text in all cases, or custom alt text if provided.
- Set, modified, and removed file alt text. Viewed timeline and feed.
- Pulled alt text with "conduit.search".
Maniphest Tasks: T13629
Differential Revision: https://secure.phabricator.com/D21596
Summary:
Ref T13628. When creating a revision, hide the recently introduced "Author" field.
It seems exceedingly unlikely that anyone would ever want to adjust this field when creating a revision, and this workflow already has a lot of fields and complexity, so this likely adds more noise than signal on the balance.
Test Plan:
- Created revisions via copy/paste on the web workflow, no longer saw "Author".
- Used "Edit Revision" to adjust the author normally.
Maniphest Tasks: T13628
Differential Revision: https://secure.phabricator.com/D21595
Summary:
See PHI2015. Diffusion attempts to prevent a commit's author from being made an auditor, but currently uses an out-of-date method for identifying the author.
Use the modern ("Repository Identity" aware) method instead.
Test Plan:
- Authored a commit as user "X", mapped to my account.
- Pushed/imported/discovered it.
- Changed the identity mapping for "X" from my account to a different account.
- Tried to add myself as an auditor.
- Before: error, "author can't be an auditor".
- After: succeeds.
- Tried to add the newly mapped user as an auditor. This correctly fails with the "author can't be an auditor" error.
It's possible to put commits into a wonky state by remapping the author identity to a user who is already an auditor, but I think that isn't important and we can't do much about it, realistically.
Differential Revision: https://secure.phabricator.com/D21594
Summary:
See PHI2015. Currently, "resigned" reviewers/auditors get different icons in Differential and Diffusion.
The Diffusion icon is exceptionally poor and confusing: it does not communicate "resigned" and it is similar to other icons.
For clarity and consistency, use the Differential icon (a grey "X") in both applications.
Test Plan: {F8492303}
Differential Revision: https://secure.phabricator.com/D21593
Summary:
Ref T13628. If you "Foist Upon", then delete the value in the tokenizer, then scroll down to the preview, you currently get an ugly "Unknown Object (???)" rendering.
This is technically correct in some vague sense and the transaction won't apply, but provide a more aesthetic rendering instead.
Test Plan: {F8487050}
Maniphest Tasks: T13628
Differential Revision: https://secure.phabricator.com/D21592
Summary:
Ref T13628. Currently, Differential has a "Commandeer" action, but no way to edit the author otherwise.
This is largely archaic: there is no reason to prevent editing the author, and this makes it difficult to undo mistakes if you commandeer by accident.
Instead, provide a normal "Author" field and a "Foist Upon" action, similar to the "Owner" and "Claim/Assign" fields in Maniphest.
Test Plan:
- Applied author transactions as the old author ("foisted"), the new author ("commandeerd"), and an arbitrary third party ("changed author").
- Tried to unassign author, etc.
- Viewed stories in feed and transaction timeline.
- Saw sensible automatic reviewer changes.
- Used existing "Commandeer" action, which is unchanged.
- Called "transaction.search" and saw reasonable transaction values.
Maniphest Tasks: T13628
Differential Revision: https://secure.phabricator.com/D21591
Summary: T13578
Test Plan:
This method uses the existing transaction. As such, most of the testing focused on the integration between the workflow and transaction. The only change made to the transaction was to allow an omnipotent user to make the change in addition to an admin.
Other than that, I removed the "approved" flag from the user, then ran the command-line utilty until the user was successfully approved.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T13578
Differential Revision: https://secure.phabricator.com/D21587
Summary:
Ref T13627. If a lock fails, the connection may be returned to the pool, even if the connection is an external connection. Under old versions of MySQL, connection reuse can release other locks on the same connection.
Don't return external connections to the pool.
This issue was introduced in D21369.
Test Plan: Added a failing test and made it pass.
Maniphest Tasks: T13627
Differential Revision: https://secure.phabricator.com/D21585
Summary:
Ref T13627. MySQL versions older than 5.7 release held locks when a new lock is acquired.
Prevent acquisition of a second lock to prevent this.
Test Plan: Added a failing unit test, made it pass.
Maniphest Tasks: T13627
Differential Revision: https://secure.phabricator.com/D21586
Summary: Ref T13627. This makes the API harder to misuse: setting an external connection on a held lock isn't a meaningful operation. Prevent it.
Test Plan: Added a failing test, made it pass.
Maniphest Tasks: T13627
Differential Revision: https://secure.phabricator.com/D21584
Summary:
Ref T13627. Currently, global locks always return connections (even external connections) to the connection pool when unlocked.
This code is obviously buggy: `isExternalConnection` is set to false immediately before it is tested. This bug has existed since this code was introduced, in D15792.
- Instead of storing a flag, store the actual connection.
- Don't clear it when unlocking.
- Don't return external connections to the pool.
Test Plan:
- Added a failing test, made it pass.
Maniphest Tasks: T13627
Differential Revision: https://secure.phabricator.com/D21583
Summary:
Ref T13625. See that task for discussion.
Currently, the Viewer when performing revision updates in response to commits may be an arbitrary low-privilege user (an Application, a disabled User, a bot, a mailing list, etc).
Today, this leads to an exception when trying to make API calls.
Ideally, we probably would not perform the update in these cases. However, performing the update isn't a policy violation and is generally less surprising than not performing it, so continue performing it for now: just use the omnipotent user to interact with the API.
Test Plan:
- Authored a commit as a bot user without permission to view the repository or revision.
- Commented out a couple of caches, and used `bin/repository reparse --publish ...` to republish the commit.
- Before: exception when trying to interact with the API.
- After: clean publish.
Maniphest Tasks: T13625
Differential Revision: https://secure.phabricator.com/D21582
Summary:
Ref T13623. When paginating notifications, we may currently construct a query which:
- loads from non-unique rows; and
- returns multiple results.
In particular, `chronologicalKey` isn't unique across the whole table (only for a given viewer). We can get away with this because no user-facing view of notifications is truly "every notification for every viewer" today.
One fix would be to implicitly force the paging query to include `withUserPHIDs(viewerPHID)`, but puruse a slightly more general fix:
- Load only unique stories.
- Explictly limit the pagination subquery to one result.
Test Plan:
- Set page size to 1, inserted duplicate notifications of all stories for another user, clicked "Next", got the GROUP BY error.
- Applied the "only load unique stories" part of the change, got a "expected one row" error instead.
- Applied the "limit 1" part of the change, got a second page of notifications.
Maniphest Tasks: T13623
Differential Revision: https://secure.phabricator.com/D21577
Summary: Ref T13624. Depends on D21578. In "sshd" subprocess contexts, use "PhutilErrorLog" to direct errors to both stderr and, if configured, a logfile on disk.
Test Plan:
- Confiugured an error log.
- Forced `ssh-auth` to fatal.
- Saw errors on stderr and in log.
Maniphest Tasks: T13624
Differential Revision: https://secure.phabricator.com/D21579
Summary: Ref T13611. This property worked correctly when implemented in D19357. The behavior was broken by D20775, which tested node-level routing but did not specifically re-test the "writable" property. This was difficult to spot because ref query outcomes weren't observable in the UI, and the ref itself had the correct property value.
Test Plan:
See D21575. After this change, the UI shows the correct state, rather than showing a read-only service ref as writable:
{F8465865}
Maniphest Tasks: T13611
Differential Revision: https://secure.phabricator.com/D21576
Summary: Ref T13611. Currently, the "writable" property on service bindings has no effect because of a trivial bug. Provide more information in the UI to make this kind of problem observable.
Test Plan:
Viewed "Storage" section of management UI, saw a more-obvious problem with ref management (a non-writable ref is listed as writable).
{F8465851}
Maniphest Tasks: T13611
Differential Revision: https://secure.phabricator.com/D21575
Summary:
Ref T13620.
- Make generic edge stories render links with hovercards. Other story types (like subscriptions) already do this so I'm fairly certain this is just old code from before hovercards.
- Include a longer commit message snippet in hovercards.
Test Plan: {F8465645}
Maniphest Tasks: T13620
Differential Revision: https://secure.phabricator.com/D21574
Summary:
Ref T13617. When an inline comment is added inside a block of added lines, it currently ends up off-by-one when porting forward.
This is a disagreement between the mapping engine and the display engine about what "offset" means. Choose the simpler of the two interpretations.
Test Plan:
- Created a revision with the diff in T13617.
- Added an inline in the middle of the added block.
- Updated the revision with the same diff.
- Before: inline incorrectly moves up by one line.
- After: inline maps correctly.
Maniphest Tasks: T13617
Differential Revision: https://secure.phabricator.com/D21572
Summary: Ref T13586. In the footsteps of D21563, make Herald rule results more formal and structured to support meaningful exception reporting.
Test Plan:
Ran various Herald rules and viewed transcripts, including rules with recursive dependencies and condition exceptions.
{F8447894}
Maniphest Tasks: T13586
Differential Revision: https://secure.phabricator.com/D21565
