Summary: Decided to just remove the hover grey to white, seems fine with the new white icons.
Test Plan: use homepage + icons
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6181
Summary: Took a stab at some login icons for buttons.
Test Plan: photoshop
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6174
Summary:
Ref T1703. This sets the stage for (but does not yet implement) custom UI types for config. In particular, a draggable list for custom fields.
I might make all the builtin types go through this at some point too, but don't really want to bother for the moment. It would be very slightly cleaner but woudn't get us much of anything.
Test Plan:
UI now renders via custom code, although that code does nothing (produces an unadorned text field):
{F45693}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1703
Differential Revision: https://secure.phabricator.com/D6154
Summary:
Adds a profile edit controller (with just one field and on links to it) that uses ApplicationTransactions and CustomField.
{F45617}
My plan is to move the other profile fields to this interface and get rid of Settings -> Profile. Basically, these will be "settings":
- Sex
- Language
- Timezone
These will be "profile":
- Real Name
- Title
- Blurb
- Profile Image (but I'm going to put this on a separate UI)
- Other custom fields
Test Plan: Edited my realname using the new interface.
Reviewers: chad, seporaitis
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6152
Summary:
None of this code is reachable yet. See discussion in D6147. Ref T1703.
Provide tighter integration between ApplicationTransactions and CustomField. Basically, I'm just trying to get all the shared stuff into the base implementation.
Test Plan: Code not reachable.
Reviewers: chad, seporaitis
Reviewed By: chad
CC: aran
Maniphest Tasks: T1703
Differential Revision: https://secure.phabricator.com/D6149
Summary:
Ref T1703. We have currently have two custom field implementations (Maniphest, Differential) and are about to add a third (User, see D6122). I'd like to generalize custom fields before doing a third implementation, so we don't back ourselves into the ApplicationTransactions corner we have with Maniphest/Differential/Audit.
For the most part, the existing custom fields work well and can be directly generalized. There are three specific things I want to improve, though:
- Integration with ApplicationSearch: Custom fields aren't indexable. ApplicationSearch is now online and seems stable and good. D5278 provides a template for a backend which can integrate with ApplicationSearch, and ApplicationSearch solves many of the other UI problems implied by exposing custom fields into search (principally, giant pages full of query fields). Generally, I want to provide stronger builtin integration between custom fields and ApplicationSearch.
- Integration with ApplicationTransactions: Likewise, custom fields should support more native integrations with ApplicationTransactions, which are also online and seem stable and well designed.
- Selection and sorting: Selecting and sorting custom fields is a huge mess right now. I want to move this into config now that we have the UI to support it, and move away from requiring users to subclass a ton of stuff just to add a field.
For ApplicationSearch, I've adopted and generalized D5278.
For ApplicationTransactions, I haven't made any specific affordances yet.
For selection and sorting, I've partially implemented config-based selection and sorting. It will work like this:
- We add a new configuration value, like `differential.fields`. In the UI, this is a draggable list of supported fields. Fields can be reordered, and most fields can be disabled.
- We load every avialable field to populate this list. New fields will appear at the bottom.
- There are two downsides to this approach:
- If we add fields in the upstream at a later date, they will appear at the end of the list if an install has customized list order or disabled fields, even if we insert them elsewhere in the upstream.
- If we reorder fields in the upstream, the reordering will not be reflected in install which have customized the order/availability.
- I think these are both acceptable costs. We only incur them if an admin edits this config, which implies they'll know how to fix it if they want to.
- We can fix both of these problems with a straightforward configuration migration if we want to bother.
- There are numerous upsides to this approach:
- We can delete a bunch of code and replace it with simple configuration.
- In general, we don't need the "selector" classes anymore.
- Users can enable available-but-disabled fields with one click.
- Users can add fields by putting their implementations in `src/extensions/` with zero subclassing or libphutil stuff.
- Generally, it's super easy for users to understand.
This doesn't actually do anything yet and will probably see some adjustments before anything starts running it.
Test Plan: Static checks only, this code isn't reachable yet.
Reviewers: chad, seporaitis
Reviewed By: chad
CC: aran
Maniphest Tasks: T1703
Differential Revision: https://secure.phabricator.com/D6147
Summary: Gets TOC populated for articles, at least, and fixes a few other things.
Test Plan: {F45474}
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6144
Summary: Needed to be more restrictive
Test Plan: Test maniphest and list examples.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6139
Summary: This adds examples and abstracts out CSS for common nav re-use.
Test Plan: Tested DocumentExample and ListExample
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6138
Summary:
This diff covers a bit of ground.
- PHUIDocumentExample has been added
- PHUIDocument has been extended with new features
- PhabricatorMenuView is now PHUIListView
- PhabricatorMenuItemView is now PHUIItemListView
Overall - I think I've gotten all the edges covered here. There is some derpi-ness that we can talk about, comments in the code. Responsive design is missing from the new features on PHUIDocument, will follow up later.
Test Plan: Tested mobile and desktop menus, old phriction layout, new document views, new lists, and object lists.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6130
Summary:
Applications come with builtin queries, but users might want to get rid of them. Allow users to disable named queries if they prefer.
This has one funky behavior, which is that the first time you disable a named query it goes to the top of your list. That will be fixed in the next diff, which will make them reorderable.
Test Plan: Added/edited/removed named queries, disabled/enabled builtin named queries.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6128
Summary: Ref T988. Mostly backend changes, with a very rough frontend on top of them. See Conpherence discussion.
Test Plan: {F45010}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D6113
Summary: See D6115.
Test Plan:
- Ran unit tests.
- Viewed reports.
- Created a countdown.
- Searched chatlog.
- Searched pastes by created date.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6116
Summary: Ref T2625. Ref T1163. A couple of small generalization nudges, but this is almost entirely straightforward.
Test Plan: Executed various File queries.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1163, T2625
Differential Revision: https://secure.phabricator.com/D6091
Summary:
Ref T2625. Ref T3273. This is mostly a UI foil for T3273. Right now, to find tasks without owners or without projects you search for the magic strings "upforgrabs" and "noproject". Unsurprisingly, no users have ever figured this out. I want to get rid of it. Instead, these interfaces will look like:
Assigned: [ Type a user name... ]
[ X ] Find unassigned tasks.
Projects: [ Type a project name... ]
[ X ] Find tasks with no projects.
Seems reasonable, I think?
Test Plan: Searched for "rainbow, js", "rainbow + no language", "no language", date ranges, etc.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T2625, T3273
Differential Revision: https://secure.phabricator.com/D6085
Summary:
Ref T1163. Ref T2625. This could probably use some tweaks, but I kept things mostly-generic.
I added a new control for freeform dates so we can have it render help or whatever later on.
Test Plan: See screenshots.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625, T1163
Differential Revision: https://secure.phabricator.com/D6084
Summary:
This prevents security by obscurity.
If I have read-only access to the database then I can pretend to be any logged-in user.
I've used `PhabricatorHash::digest()` (even though we don't need salt as the hashed string is random) to be compatible with user log.
Test Plan:
Applied patch.
Verified I'm still logged in.
Logged out.
Logged in.
$ arc tasks
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6080
Summary:
Currently, the author of an image macro is read from the attached file. This is messy and necessitates a join, and is not always correct. Instead, store the data when the macro is created.
This lays the groundwork for generalizing ApplicationSearch here. Ref T2625.
Test Plan: Migrated existing macros, created a new macro, checked web UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6071
Summary:
Kind of a quick look at an idea for T2184
Ref T2184
Test Plan: Make sure the site still loads
Reviewers: epriestley
CC: aran, Korvin, mbishopim3
Maniphest Tasks: T2184
Differential Revision: https://secure.phabricator.com/D6045
Summary: I want to use some of these for instructional text in Diffusion. Provide a concrete class to make one-offs cacheable and switch Releeph to use it.
Test Plan: {F44175}
Reviewers: btrahan, chad, edward
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6032
Summary:
Ref T2231, T603. Plan of attack here is pretty much:
- Built out a new (currently not linked in the UI) edit interface in Diffusion which is transaction-based and has a sensible layout.
- Build out a new create interface based on PagedForm which dumps into the new edit interface.
- Throw the old stuff away.
- Everyone lives happily ever after.
Test Plan:
{F44163}
{F44164}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2231
Differential Revision: https://secure.phabricator.com/D6029
Summary: Added constants to PhabricatorEventType. Modified DifferentialRevisionEditor and DifferentialCommentEditor.
Test Plan:
Created a revision. Edited and made a comment on that revision. It's updating as usual. I think nothing broke may be it's working.
Let me know if I have done it correclty.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T2899
Differential Revision: https://secure.phabricator.com/D5869
Summary: If we're unassigning an owner from a task it should set the column to `NULL` rather than an empty string. Fixes T3239
Test Plan: Assigned and Unassigned a task. Make sure the db is doing as excpected. Ran the patch, checked the db.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3239
Differential Revision: https://secure.phabricator.com/D6017
Summary:
removes the whole custom image thing, instead using a more standard application crumbs. Gives this glorious space back to the compose area which is now tens of pixels taller. Also defaults it to the people widget. Basically, fixes T3160.
For now, you **CAN NOT** edit the title of a conpherence. I didn't want to jam in too much here. Next diff will be to change the widget icons into the dropdown switcher, which will also bring back the editing of titles.
Test Plan: looked at conpherence and it was pretty. Resized it vigorously and it wasn't too bad.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T3160
Differential Revision: https://secure.phabricator.com/D5998
Summary:
Following on from D5938, also need to override the beta setting
so that testApplicationsInstalled passes
Test Plan: testApplicationsInstalled passes
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6001
Summary: My `\w` shenanigans don't work with Diffusion, which has no explicit prefix.
Test Plan: Typed `rXnnn` and got the whole thing properly marked up.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D5984
Summary:
Ref T988. This adds basics for the non-static publishing target:
- Storage (called "Live", e.g. `DivinerLiveAtom` to distinguish it from shared classes like `DivinerAtom`).
- Mostly populate the storage.
- Some minor fixes and improvements.
Test Plan: Generated docs, looked at DB, saw mostly-sensible output.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D5973
Summary: Fixes failure in "testApplicationInstalled" if this flag is off (as it is by default).
Test Plan: Ran "testApplicationsInstalled" with this flag off.
Reviewers: garoevans, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D5979
Summary: If `jsxmin` is not available, use a pure PHP implementation instead (JsShrink).
Test Plan:
- Ran `arc lint --lintall` on all JS and fixed every relevant warning.
- Forced minification on and browsed around the site using JS behaviors. Didn't hit anything problematic.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5670
Summary:
Ref T3190. #shipit
Likely future work:
- Extract project mentions from remarkup for ApplicationTransactions; this isn't relevant in any apps right now (but will be in Pholio before tooo long). Ref T3189.
- Allow projects to have alternate short names. As written, this is fine for most projects ("Differential" is `#differential`) but not so great for other projects ("Phabricator Public & Media Relations" is `#phabricator_public_media_relations`). This also breaks refs when you rename a project. Better would be letting long project names have short aliases (`#pr`) as permitted alternatives.
Since this mention uses `#` instead of a letter, I needed to do a small amount of regexp gymnastics.
Test Plan:
you only #yolo once
{F43615}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3189, T3190
Differential Revision: https://secure.phabricator.com/D5954
Summary: Debating removing the textures from the sidenavs, sending this around for comments.
Test Plan: Test homepage and various sidenavs, mobile layouts.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5948
Summary:
Normalize the unit test environment by installing all applications.
The immediate issue this fixes is that `testDropUnknownSenderMail` depends on Maniphest being installed. Some possible fixes are:
# Don't rely on the Maniphest mail receiver for the test (e.g., write a stub/dummy/mock receiver).
# Explicitly make sure Maniphest is installed before running the test.
# Normalize the test environment to install all applications.
I don't like (1) much because it turns a pretty good 10 line test into a bunch of stub classes or mock junk. I'll do it if we have more uses after a few more diffs, but so far running these tests against real code hasn't created a dependency mess and we get more coverage.
I don't like (2) much because I think requiring tests to do this will do more harm than good. The number of issues we'll hypothetically uncover by exposing unrealized application interdependencies is probably very small or maybe zero, and they're probably all trivial. But tests with an undeclared but implicit dependency on an application (e.g., Differential tests depend on Differential) are common.
So here's (3), which I think is reasonable.
I also simplified some of this code a little bit, and moved the Application object cache one level down (this was sort of a bug -- installation status is variant across requests).
Test Plan: Added unit test.
Reviewers: wez, btrahan
Reviewed By: wez
CC: aran
Differential Revision: https://secure.phabricator.com/D5938
Summary:
We/I broke a couple of things here recently (see D5911) and are doing some work here in general (see D5912, etc.).
Generally, this code is pretty oldschool and not especially well architected for modern application-oriented Phabricator. It hardcodes a lot of stuff which should be applications' responsibilites.
Take the first steps toward making it more solid to reduce the risk here. In particular:
- Factor out the "self mail" and "duplicate mail" checks and add unit tests.
- Make Message-ID hash handling automatic.
Test Plan: Ran unit tests.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D5915
Summary:
Migrate to `PhabricatorApplicationTransactions` (`ReleephRequestTransactions` applied by `ReleephRequestTransactionalEditor`, instead of `ReleephRequestEvents` created by `ReleephRequestEditor`) and migrate all the old events into transactions. Email is supported in the standard way (no more `ReleephRequestMail`) as well.
This also collapses the Releeph request create and edit controllers into one class, as well as breaking everyone's subject-based mail rules by standardising them (but which should be more easily filtered by looking at headers.)
Test Plan:
* Make requests, then pick them.
* Pick and revert the same request so that discovery happens way after `arc` has told Releeph about what's been happening.
* Try to pick something that fails to pick in a project with pick instructions (and see the instructions are in the email.)
* Load all of FB's Releeph data into my DB and run the `storage upgrade` script.
* Request a commit via the "action" in a Differential revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: epriestley, aran, Korvin, wez
Maniphest Tasks: T3092, T2720
Differential Revision: https://secure.phabricator.com/D5868
Summary: Can name saved queries.
Test Plan: Try naming some saved queries using the form.
Reviewers: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D5878
Conflicts:
src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php
Summary:
We have a few interfaces where add "Edit", "Delete" or some other action to a list. Currently, this happens via icons, but these are cumbersome and weird, are inconsistent, can't be workflow'd, are hard to hit on desktops and virtually impossible to hit on mobile, and generally just feel iffy to me. Prominent examples are Projects and Flags. I'd like to try adding an "edit" action to Maniphest (to provide quick edit from list views, basically). It looks like some of Releeph would benefit here, as well.
Instead, provide first-class actions:
{F42978}
They produce targets which my meaty ham-fists can plausibly hit on mobile, too:
{F42979}
(We could do some kind of swipe-to-expose thing eventually, but I think putting them by default is OK?)
Test Plan: Added UIExamples. Checked desktop/mobile.
Reviewers: chad, btrahan, edward
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D5890
Summary: See D5874. PhutilDeferredLog's exception on `write()`/`__destruct()` is not especially important and can come at an awkward time. Instead of throwing, just emit an error message to the log.
Test Plan: Faked failed writes and saw an error message in the log instead of many terrible things everywhere.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3144
Differential Revision: https://secure.phabricator.com/D5875
Summary: Adding mail-keys; required for `PhabricatorApplicationTransaction` support.
Test Plan: Upgrade an old database with this patch, observe the matrix: {F42620}
Reviewers: wez, epriestley
Reviewed By: epriestley
CC: epriestley, aran
Maniphest Tasks: T2720
Differential Revision: https://secure.phabricator.com/D5852
Summary:
Removing a bunch of cache-style columns from `ReleephRequest`, where it's actually much easier to just load the information at runtime.
This makes sense for migrating to `PhabricatorApplicationTransactions`, where each xaction changes one aspect of a `ReleephRequest` at a time (rather than multiple columns at once.)
Test Plan: Request something, run `arc releeph` and watch the picks, pass on some RQs, run `arc releeph` and watch the reverts.
Reviewers: wez, epriestley
Reviewed By: epriestley
CC: epriestley, aran
Maniphest Tasks: T2720
Differential Revision: https://secure.phabricator.com/D5851
Summary:
countdown_timer table named to countdown.
datepoint and related stuff renamed to epoch.
Countdowns now have phids.
Various UI items changed from timer to countdown.
Test Plan: Did run storage upgrade and added some countdowns.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T2624
Differential Revision: https://secure.phabricator.com/D5812
Summary: Enable saved query objects to actually be saved to the database.
Test Plan: Insert a call to save() and check that the query is written correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D5775
Conflicts:
src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php
Summary: this is D5750 but just the conpherence part. fixes a few random conpherence bugs / quirks as well. Also messes with ApplicationTransactionEditor to expose the xactions so Conpherence doesn't over-update participation rows. Fixes T2429.
Test Plan: set LIMIT to 3. verified I could scroll down all conpherences. next, picked a conpherence "in the middle" to load. verified I could page up and down. next, picked a conpherence in the middle then had another user update that conpherence. verified as I paged up the conpherence re-loaded properly selected
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin, vrana
Maniphest Tasks: T2429
Differential Revision: https://secure.phabricator.com/D5783
Summary:
This has no real behavioral changes (except better error handling), it just factors things out to be a bit cleaner. In particular:
- Move more shared form behaviors into the common JS form component.
- Move more error handling into shared pathways.
- Make the specialized Stripe / Balanced methods do less work.
This needs some more polish for nontrival errors (especially on the Balanced side) but none of the error behavior is worse than it was and a lot of it is much better.
Ref T2787.
Test Plan: Hit all invalid form errors, added valid payment methods with Stripe and Balacned.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D5771
Summary: Sql Patch to rename the externalaccount table to user_externalaccount and to add dateCreated, dateModified fields to the updated table.
Test Plan: {F41442}
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Differential Revision: https://secure.phabricator.com/D5770
Summary: This is a simpler version of D5649.
Test Plan: Switched to ALL CAPS translation and clicked View Options.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D5759
Summary:
Ref T2599.
Implements an "order" mode which fullscreens the editor and reduces distractions, similar to Asana's "focus" mode and GitHub's "zen" mode. This can help users who need fewer distractions get work done.
Implements a "chaos" mode which does the opposite. This can help users who need more distractions to get work done.
Test Plan: Clicked "order" and "chaos" buttons.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2599
Differential Revision: https://secure.phabricator.com/D5735
Summary: Adds a base class for displaying images and icons.
Test Plan: Tested giving and taking tokens, viewed action headers, uiexamples for icons, workboards.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5736
Summary: Created PhabricatorExternalAccount class with only data members. Will discuss with you regarding the necessary functions to be implemented in this class. Sql Patch to create a new table for external_accounts. Will I have to write unit tests the new storage object? Sending you this diff so that you can comment on this to further improve :).
Test Plan: {F40977}
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T1536, T1205
Differential Revision: https://secure.phabricator.com/D5724
Summary: Fixes T2956. Ref T2399.
Test Plan: set message limit to 2 and verified "show older" showed up, and that clicking it again and again and again showed the right stuff, ultimately not showing a "show older" UI anymore.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2399, T2956
Differential Revision: https://secure.phabricator.com/D5721
Summary: Refreshes feed's design a bit, adds app icons, works nicer on mobile.
Test Plan: Tested many feed stories, not sure I got them all, but seems fine.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5673
Summary: Migration doesn't delete differential.revisionPHID but maybe it should?
Test Plan: Reparsed commit, ran the migration, deleted differential.revisionPHID, looked at task with attached commit with attached revision.
Reviewers: epriestley, edward
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Differential Revision: https://secure.phabricator.com/D5634
Summary: Fixes T2962. That task discusses this issue.
Test Plan: Read php-curl documentation to verify this change makes sense. Sent an email with SES.
Reviewers: btrahan, garoevans
Reviewed By: garoevans
CC: aran
Maniphest Tasks: T2962
Differential Revision: https://secure.phabricator.com/D5669
Summary: Fixes T2933
Test Plan:
As guided by Evan - by setting $tasks = array(); in PhabricatorTaskmasterDaemon.php
and running 'phd debug taskmaster' and 'show full processlist' on mysql as root. No extra connections
detected.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2933
Differential Revision: https://secure.phabricator.com/D5654
Summary: We're baking some useful things into ActionHeader, would like to consolidate it's use around the site for consistency.
Test Plan: Tested log out dialog, attach dependencies, delete document in phriction.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Differential Revision: https://secure.phabricator.com/D5635
Test Plan: Searched for `IDS_PHID` and loaded homepage.
Reviewers: epriestley, edward
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5619
Summary: Creates CSS Gradients for buttons. Also tweaked hover and active styles. "Fixed" disabled state (this may require follow up diffs to correct around site)
Test Plan: Tested Maniphest and UI Examples in Chrome and IE10/9/8
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5624
Summary: MySQL is not able to use indexes with searching for tuples.
Test Plan:
Explained the query before and after, saw `key_len` 16 instead of 8.
Also saw time 0.0 s instead of 2.9 s (but that was probably caused by warming up).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5580
Summary:
Refs T1048
Adding Differential Hovercard EventListener
Adding People Hovercard EventListener
Adding basic Diffusion hovercard
Adding Conpherence Hovercard EventListener
Test Plan:
Used in a combo with working hovercards. So beautiful.
Also visited test page. Works alright.
awesometown
Reviewers: epriestley, chad, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1048
Differential Revision: https://secure.phabricator.com/D5576
Summary: Initial pass at an action header. The idea is to support current and future planned needs in 'headers' with various colors and icons. The overall goal here is to keep markup light and allow other classes to wrap and extend with more specific features.
Test Plan: Tested UIExamples and Workboards.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Differential Revision: https://secure.phabricator.com/D5585
Summary: this just does the back-end migration. I realized that we don't need to keep track of cacheTitle and cachePhoto since those are based off recent participation handles and dynamic relative to who is viewing it. Also kept the "last seen phid" as I think that will be useful to have auto-scroll to where you last read. Ref T2867.
Test Plan: did the migration. observed sensical values in the database. created a new conpherence - again sensical values. updated a conpherence - more sensical values.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T2867
Differential Revision: https://secure.phabricator.com/D5567
Summary: First pass at 'action icons' for headers and other items. Ties into future diff.
Test Plan: photoshop, read the css.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Differential Revision: https://secure.phabricator.com/D5565
Summary:
It's sometimes necessary to specify the paths to individual binaries
explicitly, e.g. a particular build of 'javelinsymbols' or a newer
version of git than is installed on your shared system.
By adding symlinks in the .../phabricator/support/bin/ directory you
can now spell these out using the file system.
Test Plan:
Ran on local Ubuntu VM:
.. add 'TEST' repo to diffusion ..
.. visit 127.0.01/diffusion/TEST - see ok ..
$ cd /opt
$ sudo sh -c 'echo "exit 1" > badgit'
$ sudo chmod +x /opt/badgit
$ sudo mkdir goodgit
$ sudo mv /usr/bin/git /opt/goodgit/
.. unset environment.append-paths ..
.. visit 127.0.01/diffusion/TEST - see error 'git: not found' ..
.. set environment.append-paths to /opt/goodgit/ ..
.. visit 127.0.01/diffusion/TEST - see ok ..
$ sudo ln -s /opt/badgit /usr/bin/git
.. visit 127.0.01/diffusion/TEST - see error 'error #1' ..
sudo ln -s /opt/goodgit/git web/phabricator/support/bin/git
.. visit 127.0.01/diffusion/TEST - see ok ..
.. unset environment.append-paths ..
.. visit 127.0.01/diffusion/TEST - see ok ..
$ sudo rm web/phabricator/support/bin/git
.. visit 127.0.01/diffusion/TEST - see error 'error #1' ..
$ sudo rm /usr/bin/git
$ sudo mv /opt/goodgit/git /usr/bin/
.. visit 127.0.01/diffusion/TEST - see ok ..
Note that 'DIRECTORY_SEPARATOR' was not used because apparently it's
portable and ok to just use '/'.
http://alanhogan.com/tips/php/directory-separator-not-necessary
(I'm pretty new to PHP so looking for guidance :)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2378
Differential Revision: https://secure.phabricator.com/D5561
Summary: Removes the glow icons and uses a hover change. Fixes phantom anchors.
Test Plan: Review in Chrome at various sizes (phone tablet). Check that icons still work. Check that mobile menus render when clicked.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2876
Differential Revision: https://secure.phabricator.com/D5549
Summary:
We have a fair number of conditionals on the existence of the access log. Instead, always build it and just don't write it if the user doesn't want a version on disk.
Also, formalize logged-in user PHID (avoids object existence juggling) in the access log and move microseconds-since-startup to PhabricatorStartup (simplifies index.php).
Depends on D5532. Fixes T2860. Ref T2870.
Test Plan: Disabled access log, verified XHProf writes occurred correctly.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2860, T2870
Differential Revision: https://secure.phabricator.com/D5533
Summary:
adds ye olde people widget. Features include
- handle-based display, so we get status for free. (Note less pretty than M14 would have it!)
- can add a person
- can remove a person
- can see the people already in the conpherence
Test Plan: added and removed people and noted they joined / re-added as appropriate. Tried to add someone already in the conpherence and got a "transaction has no effect" message
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2399
Differential Revision: https://secure.phabricator.com/D5466
Summary: As we've moved more remarkup logic post-cache (e.g., to implement policy-aware rules), we're paying more of a price for it. Improve performance in Phabricator by sharing engine instances where possible.
Test Plan: With next diff, this drops 75% of the cost of markup handling in Conpherence.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D5510
Summary: We hit some env config fairly often (~200 calls on the home page) and it's not especially cheap right now. This saves about 10ms on home, which is ~8% of the page weight on my machine.
Test Plan: Clicked around, everything seemed fine. Unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D5512
Summary: Add pagination to leader board. Add key on token count in db.
Test Plan: Set page size to 1 and give tokens to two tasks.
Reviewers: epriestley, AnhNhan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5513
Summary:
Added the initial storage structure (DB tables and DAO classes)
for Phrequent.
Test Plan:
Apply the patch and run `bin/storage upgrade`. It should
complete successfully.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2857
Differential Revision: https://secure.phabricator.com/D5476
Summary:
Ref T2787. A product is the abstract representation of something you can buy or rent/subscribe to. Although the interface isn't locked down yet, this would ultimately be internal/administrative.
Products likely have some user-facing skin on top of them: plans would have a purchasing/comparison flow, physical goods would have a storefront, etc., so products don't have any information like descriptions or images, just the data that Phortune needs to correctly bill accounts.
Generally, this is very basic for the moment.
Test Plan:
{F37594}
{F37595}
{F37596}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D5441
Summary:
Hook @btrahan's Stripe form to the rest of Phortune.
- Users can add payment methods.
- They are saved to Stripe and associated with PhortunePaymentMethods on our side.
- Payment methods appear on account overview.
Test Plan:
{F37548}
{F37549}
{F37550}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D5438
Summary:
Ref T2787. This does very little so far, but makes inroads on accounts and billing. This is mostly just modeled on what Stripe looks like. The objects are:
- **Account**: Has one or more authorized users, who can make manage the account. An example might be "Phacility", and the three of us would be able to manage it. A user may be associated with more than one account (e.g., a corporate account and a personal account) but the UI tries to simplify the common case of a single account.
- **Payment Method**: Something we can get sweet sweet money from; for now, a credit card registered with Stripe. Payment methods are associated with an account.
- **Product**: A good (one time charge) or service (recurring charge). This might be "t-shirt" or "enterprise plan" or "hourly support" or whatever else.
- **Purchase**: Represents a user purchasing a Product for an Account, using a Payment Method. e.g., you bought a shirt, or started a plan, or purchased support.
- **Charge**: Actual charges against payment methods. A Purchase can create more than one charge if it's a plan, or if the first charge fails and we re-bill.
This doesn't fully account for stuff like coupons/discounts yet but they should fit into the model without any issues.
This only implements `Account`, and that only partially.
Test Plan: {F37531}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D5435
Summary: Introduces a new settings panel for Conpherence specific settings.
Test Plan:
started a thread with a test user, thus two participants total. Replied to conpherence, toggling notification settings in between. Verified 1 or 2 emails were sent as appropos to the current toggle.
Toggled global setting and verified setting was updated in conpherences where nothing was specified. Verified setting conpherence setting overrides global setting.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2521
Differential Revision: https://secure.phabricator.com/D5391
Summary: Add the ``PhabricatorEdgeDataEditor`` that allows you to edit the data attached to an edge and its inverse.
Test Plan: Create several edges and update the data attached. Once duplicating the data on both the edge and its inverse, and once setting different value for both.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D5447
Summary: Mostly finished, wanted to get it into your hands to play with. I need to remove some more dead CSS and figure out where we want to put profile/logout, but overall feels pretty good. Tested a bunch in iOS and other layouts.
Test Plan: Test Home, Maniphest, search bars, app menus.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T2809
Differential Revision: https://secure.phabricator.com/D5407
Summary:
Fixes T2694
added edge infrastructure for Phriction
added mail subject prefix option for Phriction
added messy mail support for subscribers
adds edges to the phriction db, along with the subscriber interface
which gives us subscriptions for free.
simple display of subscribers, adequate to the current design and
sufficient fallbacks for exceptional cases. @chad may
be mailed about that one more UI element may be added to his redesign
mail support is messy. not generic at all. only sends to subscribed non-authors.
Test Plan:
tried out all kinds of stuff. applied patch, subscribed, unsubscribed with multiple
accs. verified proper
edited documents, verified that mail was sent in MetaMTA. Verified
contents, tos and stuff by looking into the db, comparing PHIDs etc.
functional testing per serious MTA (that is, AWS SES) worked wonderfully.
Here's how the subscription list looks like:
{F36320, layout=link}
Reviewers: epriestley, chad, btrahan
Reviewed By: epriestley
CC: hfcorriez, aran, Korvin
Maniphest Tasks: T2686, T2694
Differential Revision: https://secure.phabricator.com/D5372
Conflicts:
src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php
Summary: Added a column called explicit_upload to Phabricator_File.file table
Test Plan: By chekcing locally if the the column has been added in table using mysql commands.
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5384
Summary: Fished from logs, it probably doesn't cause any real problems.
Test Plan: Looked at logs.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5350
Summary:
this diff does a few, not so exciting things
- changes "conpher" to "conpherence" where it snuck into CSS, spritemap, etc -- I believe we now consistently call it conpherence. Feel free to change it, just change it everywhere. :D
- puts the widget icons in the right order per M14
- makes the "mobile-only" widgets show the toggles only in the mobile view
- also made it so clicking them does nothing for now
- removes the tasks widget since we don't want it
...my time is getting chopped up funny (yay puppy) so this is just an attempt at something that can go into the codebase and not make it worse. Next up is making the widgets that show on desktop look right / not say "TODO"
Test Plan: played around in Conpherence
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2530
Differential Revision: https://secure.phabricator.com/D5334
Summary:
Fixes T2639 by grouping related transactions at display time, so all the inlines merge into a nice block.
(Note that this does not do anything about T2709 yet, so there's still no way to figure out where the inlines actually are.)
Test Plan: {F35262}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2639
Differential Revision: https://secure.phabricator.com/D5313
Summary: It's dumb to execute a query which we know will return an empty result.
Test Plan: Looked at comment preview with "11", didn't see "1 = 0" in DarkConsole.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5177
Test Plan:
Applied the patch.
Looked at blame and plain blame of SVN and Git file.
Ran the lint saver.
Looked at lint messages list.
/diffusion/lint/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5218
Summary: Build one fixture total, not one per test case. Depends on D5258.
Test Plan: `arc unit --everything`
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5259
Test Plan:
Store large amounts of text into a string auxiliary field. It should
be stored successfully rather than truncated.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2575
Differential Revision: https://secure.phabricator.com/D5246
Summary: Dump everything to the debug log for `phd debug phabricatorbot ...`
Test Plan: Ran `phd debug phabricatorbot ...`
Reviewers: chad, AnhNhan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5239
Summary: Fixes T2651. This could be futher generalized but it's a bit out of the way.
Test Plan: See chatlog.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2651
Differential Revision: https://secure.phabricator.com/D5236
Summary:
For a single line, I can use the right click.
But for highlighting multiple lines, I have to wait for page reload.
It would be nice to track this in history but that's more involved.
This is actually maybe better behavior.
Test Plan: Clicked on the line link, dragged and dropped on it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5172
Summary:
Provide a viewer to all remarkup engines.
This fixes commit summaries in Diffusion, which were failing to link because they didn't have a user and thus couldn't see/load `D123`, e.g.
Test Plan: Grepped for engine creation.
Reviewers: vrana
Reviewed By: vrana
CC: aran, edward
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D5152
Test Plan: Made an error, rebuilt the map, linted the file, saw the error.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5113
Summary:
Ref T2632. Fixes T1466.
Currently, we normalize slugs (and thus Phriction URIs and canonical project names) to a small number of latin characters. Instead, blacklist a few characters and permit everything else (including utf8 characters).
When generating Phriction URIs, encode any utf8 characters. This means we render URIs encoded, but browsers handle this fine and display them readably in the URI and address bar, etc.
The blacklisted characters are mostly for practical reasons: \x00-\x19 are control characters, `#%?` are meaningful in URIs, `+` is sometimes configured to be interprted as space by apache, etc., `<>\\` are just silly, `&= ` are largely cosmetic.
This allows some silly stuff, like generating URIs with zero-width spaces and RTL markers in them. Possibly we should go blacklist those characters at some point.
Depends on: D5191
Test Plan: {F34402}
Reviewers: AnhNhan, chad, vrana
Reviewed By: chad
CC: aran
Maniphest Tasks: T1466, T2632
Differential Revision: https://secure.phabricator.com/D5192
Summary: Simplify the look of the mobile menu, provide active states for the menu icons.
Test Plan: iOS Simulator and Chrome.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2426
Differential Revision: https://secure.phabricator.com/D5189
Summary:
Unmuck almost all of the we-sort-of-have-viewers-some-of-the-time mess.
There are a few notable cases here:
- I used Omnipotent users when indexing objects for search. I think this is correct; we do policy filtering when showing results.
- I cheated in a bad way in the Remarkup object rule, but fixing this requires fixing all the PhabricatorRemarkupEngine callsites (there are 85). I'll do that in the next diff.
- I cheated in a few random places, like when sending mail about package edits. These aren't a big deal.
Test Plan:
- Grepped for all PhabricatorObjectHandleData references.
- Gave them viewers.
Reviewers: vrana
Reviewed By: vrana
CC: aran, edward
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D5151
Summary:
See discussion in D5121. Fixes T2615.
This might cause us more issues if anything is loading commit handles without passing a viewer, but I think I tested all of those cases.
Test Plan: Looked at feed, audit, maniphest, diffusion, differential, owners, repositories.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2615
Differential Revision: https://secure.phabricator.com/D5139
Summary:
Move Diffusion to be hovercard-ready, and expand our ability to resolve commit references.
- Link unqualified hashes of 7 characters or more which match a commit.
- Link qualified hashes of 5 characters or more which match a commit.
- Support `{...}` syntax.
Test Plan: {F33896}
Reviewers: chad, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5121
Summary:
- Unify all the reference/embed Remarkup rules for Differential, Maniphest, Paste and Ponder.
- Add rules for Pholio.
- Does not yet unify Diffusion or Files (both are a bit more involved).
- Prepare for hovercards.
Test Plan: {F33894}
Reviewers: chad, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5120
Summary: Makes the visual size 2px larger and hit area 4px bigger on notification and message icons.
Test Plan: Review icons in sandbox, test new layout with notifications or messages.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5127
Summary:
D5120 and followups refactor and generalize object references in Remarkup -- notably, they move remarkup rules from a central location to the implementing applications.
Preserve blame by doing moves/renames only first. This change moves application remarkup rules into those applications, and renames the ones D5120 modifies.
Test Plan: Typed some preview text into a textarea, got a valid Remarkup render.
Reviewers: vrana, chad
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5123
Summary:
Fixes T2059. Ref T2517.
Currently, you can run `bin/storage upgrade` with `--user` and `--password` arguments. However, these clownishly apply only to `.sql` patches -- the `.php` migrations still use the default user and password.
This is dumb. Stop doing it. Respect `--user` and `--password` for PHP patches.
(I implemented "override", which is very similar to "repair", but kept them separate since I think they're semantically distinct enough to differentiate.)
Test Plan: Ran `./bin/storage upgrade --user x --pass y --apply phabricator:20130219.commitsummarymig.php`. Verified the correct user and password were used both for the initial connect and patch application.
Reviewers: chad, vrana
Reviewed By: chad
CC: aran
Maniphest Tasks: T2059, T2517
Differential Revision: https://secure.phabricator.com/D5115
Summary: Deleted the channel field and added a sql patch to drop the channel field.
Test Plan: I have messed up my local mysql:P, hence by storage upgrade is failing. Anyways, The chatlog_event table shouldn't contain the channel column now.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5011
Summary:
Fixes T2563. Instead of rendering "rPnnnnnn", render "rPnnnnnn: add feature X". Tweak Audit tables to accommodate.
@vrana / @nh, this migration might take a while. You could safely skip it when deploying and then run it after deployment.
I think I fixed all the other places where these render, but might have missed something.
Test Plan:
- Ran first schema migration, clicked around to make sure nothing broke.
- Ran `scripts/repository/reparse.php --message rXyyyyy`, verified summary populated.
- Ran second migration.
- Checked task/diffusion/audit/differential for weird rendering.
Reviewers: vrana
Reviewed By: vrana
CC: nh, aran, chrisbolt, allixsenos
Maniphest Tasks: T2563
Differential Revision: https://secure.phabricator.com/D5012
Summary:
Also splits blocking and active revisions.
This could display 0 with non-empty tip over it.
It's intentional meaning that 0 objects need your attention but there is still some work to do.
Test Plan: Hovered over number.
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5049
Summary: Added ttl field to files. Gabage collect files with expired ttl
Test Plan: created file with a ttl. Let garbage collector run
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4987
Summary:
PhabricatorBotMessage->getSender returns a PhabricatorBotUser object (which potentially can be null)
So check null and then use getName to get actual name of the sender
Test Plan: Run phabot and add myself to ignore list
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5036
Summary: Mostly applies a new call spacing rule; also a few things that have slipped through via pull requests and such
Test Plan: `find src/ -type f -name '*.php' | xargs -n16 arc lint --output summary --apply-patches`
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5002
Summary:
It makes sense for Phabricator customizations to use all the Phabricator linters and maybe add some more or delete some.
Doing this as proxy would be PITA as there are lots of methods that would need to be proxied.
Test Plan: Extended it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4954
Summary: Fixes T2559 with an incredibly original patch which I came up with myself.
Test Plan:
$ ./bin/storage upgrade -f
Applying patch 'phabricator:20130218.longdaemon.sql'...
Storage is up to date. Use 'storage status' for details.
Reviewers: edward
Reviewed By: edward
CC: aran
Maniphest Tasks: T2559
Differential Revision: https://secure.phabricator.com/D5006
Summary: Php schema patch to update channel id's of past events.
Test Plan: Having some proxy issues here due to which connection is timing out and bot is not able to log into IRC. Bot connects to IRC in my home though ! So I wasn't able to quite to test this by running storage upgrade.
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5000
Summary:
- Currently, the atomizers don't emit atoms with the right file in all cases. Make them always emit it correctly.
- Currently, we use absolute paths in some cases and relative paths in other cases. Use them consistently: relative when storing/presenting, absolute when accessing data.
- Don't preserve linebreaks when marking up documentation (documentation is generally wrapped at 80col, but should not be wrapped in this way when displayed).
- Markup Diviner link rules (albeit uselesly).
Test Plan:
Before:
{F33044}
After:
{F33045}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D4992
Summary: We currently garbage collect general cache entries after a set period of time (30 days by default), but the recent changes to DarkConsole have left us writing a lot of large, short-TTL data to the cache. In addition to a maximum age, GC cache entires after they TTL out.
Test Plan: Ran GC daemon, saw TTL'd entries get collected. Inserted a TTL'd entry, saw it get collected by GC. Saw non-ttl'd entries not get collected.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4990
Summary:
- Remove some redundant copies of translations after D4985.
- Make some %d more grammatical, "run this command" reads better than "run this 1 command". In context, these numbers are always very small, so counting them even in the >1 variants aren't useful.
- Fix subscriber(s).
Test Plan: Looked at an unsubscribe story, saw translation.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4989
Summary: First pass. Flowdock supports interesting message types (like replies to messages), but for now implementing a standard messaging interface.
Test Plan: Ran both a Flowdock bot and a Campfire bot. Made sure both still connected and responded properly to the Object Handler.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4983
Summary:
Remarkup rule callbacks now get SafeHTML matches instead of string matches. If they call:
$some_lisk_dao->load($matches[1]);
..as is the case with the `{F123}` rule, we reject the SafeHTML as an invalid ID and return null.
Allow load() to string convert any object (which will either succeed or fatal in an obviously-broken way).
(Long ago we threw instead of returning null here, but it meant we had to do a lot of redundant checks.)
Test Plan: `{F123}` shows an image again. `{C1}` embeds a countdown.
Reviewers: vrana, chad
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4961
Summary: Added a column channelID column to phabricator_chatlog.chatlog_event
Test Plan: Checked through mysql to see if table is updated
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4956
Summary: Added 20130214.chatlogchannel.sql in resources/sql/patches to add a new table
Test Plan: Hmmmmm .......
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4952
Summary:
Clearly silly to have a separate handler for this. I also made most of the protocol stuff direct writes so we don't need to ship them through handlers, and made the adapter ignore message it does not understand by default instead of sending them to IRC, and added PASTE "support".
We could still let handlers react to these messages by emitting them all as 'RAWIRC' or similar, but there's currently no need for that so I didn't bother.
Also fix an issue in D4924 with nickpass.
Test Plan: Had bot join IRC, talked to it.
Reviewers: indiefan
Reviewed By: indiefan
CC: aran
Differential Revision: https://secure.phabricator.com/D4925
Summary:
Make users/channels/rooms into objects, so we can later sort out stuff like Campfire user IDs, Phabricator vs chat accounts, etc.
The only change here is that I removed output buffering from the macro handler. We should move throttling/buffering to adapters instead and have it apply globally.
Test Plan: Ran IRC and Campfire bots and interacted with them.
Reviewers: indiefan
Reviewed By: indiefan
CC: aran
Differential Revision: https://secure.phabricator.com/D4924
Summary:
- Reduce visibiliy of config.
- Add a typehint.
Test Plan: Ran campfire/irc bots and chatted with them.
Reviewers: indiefan
Reviewed By: indiefan
CC: aran, amerigomasini
Differential Revision: https://secure.phabricator.com/D4923
Summary: Sgrepped for `"=~/</"` and manually changed every HTML.
Test Plan: This doesn't work yet but it is hopefully one of the last diffs before Phabricator will be undoubtedly HTML safe.
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4927
Test Plan: Ran the bot with a handler that sends sound commands.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4922
Summary:
This is pretty brutal and it adds some `phutil_safe_html()`.
But it is a big step in the right direction.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4905
Summary:
When a developer changes CSS, it is normally sufficient to reload the page to get changes to show up, because browsers revalidate resources on reload.
However, if you reload the page and then an Ajax request adds new CSS to the page, this CSS does not trigger revalidation. The developer must currently clear their cache or re-run `scripts/celerity_mapper.php webroot`, to get this request to skip cache. We rarely use CSS over Ajax right now, so this hasn't cropped up much, but Conpherence does use this and clearing the resource is a big pain.
This seems to work fine normally, but I'm worried it might break some of the extra-celerity-resources stuff Facebook is doing.
Test Plan: In development mode, changed `conpherence/message-pane.css` and saw changes reflected on reload. Verified normal page loads do not cause additional HTTP requests. This change has no effect in production mode.
Reviewers: edward, vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2428
Differential Revision: https://secure.phabricator.com/D4902
Summary: Also added sender to the campfire adapter. This isn't extremely useful as it's just a numeric id, but it allows us to add ignores (specifically having the bot ignore itself).
Test Plan: Ran the bot, ignored itself.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4893
Summary:
Done by searching for `AphrontDialogView` and then `appendChild()`.
Also added some `pht()`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4882
Summary: Route all `$_SERVER['HTTP_...']` stuff through AphrontRequest (it would be nice to make this non-static, but the stack is a bit tangled right now...)
Test Plan: Verified CSRF and cascading profiling. `var_dump()`'d User-Agent and Referer and verified they are populated and returned correct values when accessed. Restarted server to trigger setup checks.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4888
Summary: If a page is profiled, add an "X-Phabricator-Profiler" header to all Ajax requests, and profile those too.
Test Plan: Profiled a page, checked Darkconsole, saw profiles for everything.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4885
Summary:
- Use PhutilURI to correct for specifying "https://yourname.campfire.com/" instead of "https://yourname.campfire.com".
- Use HTTPSFuture to get logging via `--trace` and error detection (CA stuff should be OK since 37signals has real certs).
- On destruction, only try to leave rooms we've actually joined.
Test Plan: Setup a bot, had it join a room, talked to it.
Reviewers: indiefan
Reviewed By: indiefan
CC: aran
Differential Revision: https://secure.phabricator.com/D4849
Summary:
Decided the best approach for refactoring the message/command stuff would be to actually start implementing the campfire adapter to get a better idea of what the abstractions should look like. It feels awkward and unwieldy trying to maintain the irc command interface (notice the message instantiation in the `processReadBuffer()` method. However, i'm still not clear what the best approach is without requiring a re-write of nearly all the existing handlers and defining essentially a custom dsl on top of irc's.
I suppose given that alternative, implementing to irc's dsl doesn't sound all that bad. Just feels like poor coupling.
Also, I know that there is some http stuff in libphutil's futures library, but the https future is shit and I need to do some custom curlopt stuff I wasn't sure how to do with that. But if you think this should be refactored, let me know.
I tested this with the ObjectHandler (messages with DXXX initiate the bot to respond with the title/link just as with irc), but beyond that, I haven't tried any of the other handlers, so if there are complications you think i'm going to run into, just let me know (this is one of the reasons for requesting review early on).
Also, this diff is against my last one, even though that hasn't been merged down yet. It was starting to get large and I'd prefer to keep to two conversations separate.
Fixing some lint issues.
Test Plan: Ran the bot with the Object Handler in campfire and observed it behaving properly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2462
Differential Revision: https://secure.phabricator.com/D4830
Summary: ...i tend to forget to do this for some reason. my bad.
Test Plan: NA
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2503
Differential Revision: https://secure.phabricator.com/D4840
Summary:
I wasn't able to reproduce the "recursion detected" in real web request but I saw lots of 1073741824 refcounts in `debug_zval_dump()` of $object.
I'm not sure how that happens.
Test Plan: D4807#4
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4839
Summary: If a file isn't a viewable image, don't try to figure out metadata (size, etc.) when rendering a `{F...}` tag in Remarkup.
Test Plan: Uploaded a .rtf, added it as `{F1}` in a new Maniphest task, saw no errors in the dark console.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2479
Differential Revision: https://secure.phabricator.com/D4837
Summary:
Ugh, just wrote out a huge message, only to lose it with a fat-fingered ctrl-c. Le sigh.
First pass at decoupling the bot from the protocol. Noticeably absent is the command/message coupling. After this design pass I'll give that a go. Could use some advice, thinking that handlers should only create messages (which can be public or private) and not open ended, undefined 'commands'. The problem being that there needs to be some consistant api if we want handlers to be protocol agnostic. Perhaps that's a pipedream, what are your thoughts?
Secondly, a few notes, design review requests on the changes i did make:
# Config. For now i'm passing config through to the adapter. This was mainly to remain backwards compatible on the config. I was thinking it should probably be namespaced into it's own subobject though to distinguish the adapter config from the bot config.
# Adapter selection. This flavor is the one-bot-daemon, config specified protocol version. The upside is that in the future they won't have to run different daemons for this stuff, just have different config, and the door is open for multiple protocol adapters down the road if need be. The downside is that I had to rename the daemon (non-backwards compatible change) and there will need to be some sort of runtime evaluation for instatiation of the adapter. For now I just have a crude switch, but I was thinking of just taking the string they supply as the class name (ala `try { new $clasName(); } catch...`) so as to allow for homegrown adapters, but I wasn't sure how such runtime magic would go over. Also, an alternative would be to make the PhabricatorBot class a non-abstract non-final base class and have the adapters be accompanied by a bot class that just defines their adapter as a property. The upside of which is backwards compatibility (welcome back PhabricatorIRCBot) and perhaps a little bit clearer plugin path for homegrowners.
# Logging. You'll notice I commented out two very important logging lines in the irc adapter. This isn't intended to remain commented out, but I'm not sure what the best way is to get logging at this layer. I'm wary of just composing the daemon back down into the adapter (bi-directional object composition makes my skin crawl), but something needs to happen, obviously. Advice?
That's it. After the feedback on the above, you can either merge down, or wait until i finish the command/message refactor if you don't think the diff will grow too large. Up to you, this all functions as is.
Test Plan: Ran an irc bot, connected, read input, and wrote output including handler integration.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2462
Differential Revision: https://secure.phabricator.com/D4757
Summary: do so via event engine. note different order now...
Test Plan: toggled "show beta applications" to off and noted that Conpherence disappeared. Otherwise noted that links showed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2424
Differential Revision: https://secure.phabricator.com/D4708
Test Plan: Ran the migration on a single revision, verified DB, called `loadUnsubscribedPHIDs()`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4786
Summary: Refactor options related to verbose error reporting and forcing disk reads into a single developer option.
Test Plan: Run Phabricator with the developer-mode option set and check that errors print stack traces, static assets are always reloaded, etc.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4780
Summary: It's a little bit confusing that you couldn't use %d but kind of expected taken that the number will be formatted so it's not a number anymore.
Test Plan: /paste/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4764
Summary: Pretty straightforward.
Test Plan: Viewed inline edit on left / right and new /edit.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4724
Summary: break out the calculation of dimensions as a static method and use it
Test Plan: made a conpherence with many images and noted i auto-scrolled to the bottom correctly
Reviewers: chad, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2399
Differential Revision: https://secure.phabricator.com/D4733
Summary: Adding ':' in order to support SA-style smiley conventions (e.g: :allears:) in Phabricator.
Test Plan: Tested working on local Phabricator copy.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D4727
Summary: Converts various callsites from render_tag variants to tag variants.
Test Plan: See inlines.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4689
Summary:
If you run this code:
json_encode(array('tag' => phutil_tag('div', array())));
...you get this result, because json_encode() does not call toString() on objects:
{"tag":{}}
Instead, convert such objects to their underlying strings. Javelin has support for JX.HTML and for implicit conversion (which is kind of sketchy for other reasons) but it's sort of complicated (only happens on Ajax, not behaviors) and messy (not metadata-based), so ignore it for now.
We'll need to do something similar for serialization to the database. My plan there is just to throw on any objects. The only time we put HTML in the database is cache-related and those tiny number of callsites can manually handle it.
Test Plan: Various ajax things now receive the correct data.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4684
Summary: Let's see if I did this right. This adds on and off state icons (1 and 2x) for conpherence. I think I need to tweak and add more CSS to have the off hover state be the on icon. Will check.
Test Plan: spritegen
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2400
Differential Revision: https://secure.phabricator.com/D4709
Summary: this was done for conpherence so the auto-scroll actually works. NOTE we actually use the 220 preview UI for file attachments right now so this really only helps in the macro case. :/
Test Plan: sent some conpherences with macros and files. verified image width / height was set as expected.
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2399
Differential Revision: https://secure.phabricator.com/D4678
Summary: Adds notification icons for Conpherence and re-writes the CSS a bit for the new icons and states. I removed the background bubble here and went straight CSS. I also seem to have a JS error and the notification menu doesn't display, but I'm tired and wanted to look at this in Differential. Will update after JS fix.
Test Plan: Turned on notification numbers and conpherence numbers, turned them off. Made them big. Checked FF and IE.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Maniphest Tasks: T2415
Differential Revision: https://secure.phabricator.com/D4666
Summary: Fixes various array_combine() warnings for PHP < 5.4
Test Plan: lint/unit/grep
Reviewers: btrahan, vrana, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4660
Summary:
- Implements `javelin_tag()`, which is `javelin_render_tag()` on top of `phutil_tag()` instead of `phutil_render_tag()`.
- Manually converts all or almost all of the trivial callsites.
Test Plan:
- Site does not seem any more broken than before.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4639
Summary:
- Grepped for phutil_render_tag().
- Fixed some easy ones.
Test Plan:
- Browsed around; site didn't seem more broken than it was before.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4638
Summary:
Created with spatch:
lang=diff
- phutil_render_tag
+ phutil_tag
(X, Y, '...')
Then searched for `&` and `<` in the output and replaced them.
Test Plan: Loaded homepage.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4503
Summary: it's ugly. but it works. basically. See T2399 for a roughly prioritized list of what still needs to happen.
Test Plan:
- created a conpherence with myself from my profile
- created a conpherence with myself from "new conpherence"
- created a conphernece with another from "new conpherence"
- created a conpherence with several others
- created a conpherence with files in the initial post
- verified files via comment text ("{F232} is awesome!") and via traditional attach
- edited a conpherence image
- verified it showed up in the header and in the conpherence menu on the left
- edited a conpherence title
- verified it showed up in the header and in the conpherence menu on the right
- verified each widget showed up when clicked and displayed the proper data
- calendar being an exception since it sucks so hard right now.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, epriestley, chad, codeblock, Korvin
Maniphest Tasks: T2301
Differential Revision: https://secure.phabricator.com/D4620
Summary: Adds an ALL CAPs language, requires changes from libphutil as well.
Test Plan: Turn on AC, get caps back. Turn it back to English, stuff went away.
Reviewers: epriestley, btrahan, vrana
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4627
Summary: This needs some tweaks but I'll follow up with @DeedyDas in T2353.
Test Plan: So many memes.
Reviewers: chad, btrahan, DeedyDas
Reviewed By: chad
CC: aran
Maniphest Tasks: T2353
Differential Revision: https://secure.phabricator.com/D4616
Summary:
- PHABRICATOR_ENV is now optional. If you don't specify it, we won't load a config file.
- PhabricatorSetup is now gone.
- I removed the alternate file domain check for now, see T2380.
- `phabricator.setup` config is now gone.
- Rewrote documentation:
- No more mentions of `phabricator.setup`.
- Normal install guide no longer mentions PHABRICATOR_ENV. This is now an advanced topic.
- Clarified that you only need to set up one of apache, nginx or lighttpd.
- Tweaked a few things I've seen users have difficulty with.
This should have no effect on any existing installs, but make the process much simpler for future installs.
Closes T2221.
Closes T2223.
Closes T2228.
Test Plan:
- Removed my PHABRICATOR_ENV and went through the install process.
- Generated and read documentation.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2221, T2223, T2228
Differential Revision: https://secure.phabricator.com/D4596
Summary: Port the database checks over.
Test Plan: Triggered all the checks via intentional misconfiguration.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2228
Differential Revision: https://secure.phabricator.com/D4590
Summary:
- Allow new-style setup to raise fatal setup errors.
- Port extension checks to new-style setup as fatal errors.
- When fatal errors are raised, abort setup and show them in a chrome-free response.
Test Plan: {F29981}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2228
Differential Revision: https://secure.phabricator.com/D4587
Summary:
We no longer need to do PHP CLI checks (D4568) or run `git submodule` (D4581) so we don't need $PATH to be set to complete setup. Move it to post-install.
Drop the instructions about PHP-FPM because the Phabricator config is dramatically easier now that we have it.
Test Plan: Set environment.append-paths to various things, faked lack of $PATH, verified I got the warning when I expected to setting Phabricator config cleared it.
Reviewers: codeblock, btrahan
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2228
Differential Revision: https://secure.phabricator.com/D4585
Summary:
Fixes T2293.
We currently hard-require this in setup. We do not need to; we don't actually need it until we start running daemons. Move it to post-install and provide more guidance.
We could make this even easier in the future, but we'd need to special case it, since it's dangerous to let it be set to any value (if you set it to the wrong value, you can't log in). We could safely have a workflow which writes the current request URI into the database configuration, or a two-stage workflow where we set the URI and then verify it, but these both imply some special casing and complication. This should be a step forward from where we are today, regardless.
Test Plan:
Removed "phabricator.base-uri" from my configuration. Verified Phabricator still works.
Without "phabricator.base-uri" configured, logged in from multiple host names (127.0.0.1:8080, local.aphront.com:8080).
Configured "phabricator.base-uri". Verified my unblessed session no longer worked. Verified setup issue went away.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2293
Differential Revision: https://secure.phabricator.com/D4580
Summary:
Submoduling is slightly convenient for developers but hellishly difficult for many users. Since we make about a dozen updates to Javelin per year, just include the source directly.
Even if we run `git submodule status` more often, this creates additional problems for users with PATH misconfigured.
Fixes T2062 by nuking it from orbit.
Test Plan: Loaded site, browsed around. Grepped for references to submodules.
Reviewers: btrahan, vrana
CC: aran
Maniphest Tasks: T2062
Differential Revision: https://secure.phabricator.com/D4581
Summary:
Some time ago, we added `ORDER BY id ASC` to the worker `UPDATE ...` query, because someone reported that their MySQL read slaves were complaining about the query (I can't find the exact error message, but something to the effect of the rows the query affected not being deterministic). This seemed harmless since it should be the same as the query's implicit order (I guess?), but actually made the query dramatically slower for large numbers of rows.
On my local machine, this query takes about 2 seconds with ~1M rows. If I run `SELECT`, or run `UPDATE` without ORDER BY, the query takes < 0.01s. I don't understand exactly what's happening -- my guess is something to do with the ORDER BY implying that a lot of rows need to be locked?
In T2372, a user is seeing 20-60s rumtimes on this query.
I solved this by doing a SELECT, followed by an UPDATE. Each query runs quickly. This introduces the possibility of a race (two processes SELECT the same rows, then try to UPDATE), which we currently recover from by having the second UPDATE fail and then having that daemon try again 1 second later. This seems generally reasonable. Some alternatives I considered:
- We could SELECT ... LOCK FOR UPDATE, but failing and retrying a little later seems at least as good as blocking.
- We could select more rows than we need, and then try to lock some of them randomly. I think this would work well, but it's a bit more complex than what we're doing now so I left it until we have a clearer need.
Test Plan:
Inserted ~1M tasks into the queue. Ran `phd debug taskmaster`, saw ~2s task updates. Applied patch. Ran `phd debug taskmaster`, saw <1ms updates. Ran `phd launch 8 taskmaster`, saw rapid completion of tasks.
This stuff also has fairly thorough unit test coverage.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2372
Differential Revision: https://secure.phabricator.com/D4576
Summary: These have been marked as deprecated since May 2012. Clean them up.
Test Plan: Grepped for `repository-launch`, `phd_load_tracked_repositories`: no hits.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2372
Differential Revision: https://secure.phabricator.com/D4575
Summary: ignore - array - Array of nicks to ignore all mesages from
Test Plan: run phabot with ignore set
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4582
Summary: The easiest approach here is proably to provide a more specific rule in the sheet CSS. This saves us from having to write any JS, notably.
Test Plan: Hovered over "+" on homepage.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4579
Summary: D4453 and D4427 sailed past one another, like ships in the night.
Test Plan: Verified Differential hover and selected states.
Reviewers: asherkin, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4577
Summary:
As far as I know, we never actually need `php` to be available from the web UI. I think the history here is:
- Long ago, we checked for 'pcntl' as an extension during setup.
- Someone had an install where 'pcntl' was available from the CLI, but not the web UI. So we switched the check to use the CLI.
- Someone had an install where the CLI binary was php-fpm, which caused the 'pcntl' check to loop endlessly, so we added more checks.
But we don't actually need to do any of this -- when the user tries to run the daemons, they get an explicit message that they need to install pcntl already, and we never (as far as I know) try to run PHP scripts from the web UI other than the pcntl_available.php check (we only run `git`, `svn`, `hg`, `ssh-agent`, `diff`, `xhpast` and `pygmentize`, I think).
Test Plan: Thought carefully about places we might execute PHP scripts from the web UI. Looked through /scripts/ to try to identfiy anything we might execute.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4568
Summary: Unit test for T2345
Test Plan: Ran unit tests, checked that it passed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4552
Summary:
T2345
getConfig throws an Exception when the key does not exist.
Also removes dead code that throws an Exception.
Test Plan:
Reloaded the Phabricator home page. In the process, found
2 Exceptions thrown due to nonexistent keys. After addressing these problems,
the home page loads without Exceptions.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4541
Summary: I've stored `PhutilSafeHTML` instance to cache on devbox and then wasn't able to read it in production.
Test Plan: Displayed revision with unreadable cache, saw error in error log but not fatal.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4520
Summary: Technically we should have these for all the OAuth providers but I don't think anyone really has trouble with them and it can probably be done generically after T1536. Preserve the functionality, at least.
Test Plan: Broke my config, verified warnings appeared.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2228
Differential Revision: https://secure.phabricator.com/D4506
Summary: These are nonblocking warnings and can move to post-install.
Test Plan: Broke my environment and observed the warnings.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran, asherkin
Maniphest Tasks: T2228
Differential Revision: https://secure.phabricator.com/D4505
Summary:
Migrate to the new hotness. Also:
- Remove a string test, which is now impossible since the config will repair itself and raise a type error.
- Restore the header even in /config/ -- this check is kind of hacky and it feels a bit more natural now that it's above the menu.
Test Plan: Set my local disk path to something invalid, verified I got a setup error.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2228
Differential Revision: https://secure.phabricator.com/D4497
Summary: Fixes T2269. If the user manages to mess up both the PHP and Phabricator configurations, set the timezone to UTC. We basically never use this anyway (we always render into the user's time), PHP just gets angry at us if we don't set it. (We do use it for logged-out users, I suppose.)
Test Plan: Set PHP and Phabricator timezones to goofy nonsense, verified we recover sensibly from it.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2228, T2269
Differential Revision: https://secure.phabricator.com/D4496
Summary: Because the Default configuration provider is loaded before custom libraries, any config options specified in them don't get a default values.
Test Plan: Looked at /config/
Reviewers: epriestley, codeblock, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4532
Summary:
Follows Phabricator's feed and puts notifications into channels
that are configured.
~~notification.all - bool - 1:1 stories to messages~~
notification.types - array - Specific story types to notify for - ["differential", "maniphest"]
notification.verbosity - int - Range of 0-3 for verbosity
notification.max_pages - int - Maximum number of pages to go back per poll
notification.page_size - int - Size of pages (limit) to poll
~~notification.channels - array - Array of channels to send messages to~~
~~notification.sleep - int - Seconds to sleep between polls~~
Test Plan: Run phabot with various configuration options
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, asherkin
Differential Revision: https://secure.phabricator.com/D4418
Summary: Ports mail stuff from the existing setup process to the more modular setup checks.
Test Plan: Configured my local install to have all these errors, verified setup raised them.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2228
Differential Revision: https://secure.phabricator.com/D4494
Summary: This is gross, but fixes an issue where `bin/storage upgrade` tries to access DB config which doesn't exist yet. We need a version of this for `bin/config` anyway. I'll sort this out into a proper sequenced startup process in a followup.
Test Plan: `bin/storage upgrade` no longer fatals when upgrading across the config boundary.
Reviewers: asherkin, codeblock, btrahan, vrana
Reviewed By: codeblock
CC: aran
Differential Revision: https://secure.phabricator.com/D4512
Summary:
If your configuration overrides the connection adapter, we need to load libraries before we can setup the database config source.
Also lock this since it won't work when edited from the web anymore, and so sneaky users can't upload stuff and then edit their config to run arbitrary code.
Test Plan: See chatlog in #phabricator. This is a problem for Facebook only.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4498
Summary: We now issue only valid setup warnings, so we can let administrators know when we detect problems.
Test Plan:
Banner:
{F29568}
Created a fake issue; saw banner. No banner inside /config/. Resovled the issue, banner went away.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2221
Differential Revision: https://secure.phabricator.com/D4493
Summary:
When configuration is set incorrectly (e.g., of the wrong type), detect and repair it by setting it to the default value. A setup warning will be raised separately.
Notably, this removes the need to hard-code all the class types.
This runs separately from the "invalid config" check because we need to run it on every page, but do setup checks only once per restart (some of them are slow).
Also dirty setup when we edit configuration.
Test Plan: Set config incorrectly on purpose, saw Phabricator correct it on restart and on every subsequent page load until it was fixed.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2292
Differential Revision: https://secure.phabricator.com/D4492
Summary:
Read configuration from the new database source.
This adds an extra MySQL connect + query to every page. They're very cheap so I think we can suffer them for now, but I'd like to put cache in front of this at some point. The difficulties are:
- If we use APC, multi-frontend installs (Facebook) can't dirty it (major problem), and the CLI can't dirty it (fine for now, maybe a major problem later).
- If we use Memcache, we need to add config stuff.
- We could use APC in all non-Facebook installs if we can make it dirtyable from the CLI, but I don't see a reasonable way to do that.
- We don't have any other caches which are faster than the database.
So I'll probably implement Memcache support at some point, although this is a lame excuse for it.
Test Plan: Added some config values via web UI, saw them active on the install.
Reviewers: btrahan, codeblock, vrana
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2221
Differential Revision: https://secure.phabricator.com/D4296
Summary: If `unset($env)` throws then we pop some other environment instead which is impossible to pop later.
Test Plan:
$ arc unit src/infrastructure/env/__tests__ src/applications/calendar/storage/__tests__
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4488
Summary: Connection takes .3s from dev server to master.
Test Plan:
$ bin/storage --trace upgrade --namespace x
$ bin/storage --trace destroy --namespace x
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4480
Summary: These can all fit into the gradient sprite.
Test Plan: Looked at menu with selected item, hovered over menu.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4453
Summary: Allows to easily disable responding to "where is..."
Test Plan: Run ircbot with and without the handler
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4444
Summary:
Fixes T2273. We currently discard logs, service calls, etc., for daemons, but not for other scripts. However, other scripts may be long-running or issue a large body of service calls (e.g., `bin/search index --all`). We never retrieve this information from scripts (it is used to build darkconsole; in scripts, we echo it immediately under --trace), so discard it immediately to prevent these scripts from requiring a large amount of memory.
(When the daemons load `__init_script__.php` they end up calling this code, so this doesn't change anything for them. They hit another ServiceProfiler discard along the daemon pathways in libphutil, but the call is idempotent.)
Test Plan: Ran `bin/search index --all` and saw increasing memory usage before this patch, but steady memory usage after this patch.
Reviewers: btrahan, vrana, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2273
Differential Revision: https://secure.phabricator.com/D4364
Summary: Also provide a way to update old files metadata.
Test Plan: Create a revision which includes a image file. Check whether the widht, height metadata exists. Run `scripts/files/manage_files.php metadata --all` to update previously uploaded files.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2101
Differential Revision: https://secure.phabricator.com/D4347
Summary: this can happen if you have Phabricator and email lists co-mingling such that Phabricator receives an email multiple times. we can prevent this from then spamming everyone or otherwise taking the action multiple times by storing a message id hash and dropping the message if we have more than one message that matches.
Test Plan: simulated sending the same email multiple times on the command line. noted only the first one made it through.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1726
Differential Revision: https://secure.phabricator.com/D4328
Summary:
This replaces D4175 and makes it so phabot doesn't message anyone.
The reasons for this are twofold:
- It was possible to get information from the bot, by private messaging it, even
if the bot was only in a +i channel (on a public network) -- meaning that if
someone knew the nickname of the bot, they could obtain e.g. ticket names
or diff titles.
- The other time it messaged people was when you typed e.g. "somenick: T123".
Most times when this is triggered, it's done so on accident.
See discussion on the old revision (D4175).
Test Plan:
15:29:33 ::: Irssi: Starting query in quartz with cb-phabot
15:29:38 <relrod> T2
(nothing back)
and
15:29:21 <@relrod> rublets: T1
15:29:21 < cb-phabot> T1: asdfasdf (Priority: Needs Triage) - http://local.elrod.me/T1
Reviewers: epriestley, btrahan, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4339
Summary:
- Ports MySQL settings to PHP.
- Removes "mysql.retries" -- this existed only because Magic Numbers Are Bad, but there is no concievable reason it should ever be set to anything other than 3.
- Introduced "Hidden" config, which isn't visible from the web (for SaaS, we'll just mark anything with secret keys as "hidden").
- Introduced "Masked" config, which will be masked in darkconsole once that gets updated.
- "Hidden" implies "Masked" and "Locked".
- Moved "storage.default-namespace" here -- it probably makes more sense than core; this was my bad in T2255.
- Put cancel button back for hidden/locked config.
- Introduce 'class' config type.
Test Plan: Viewed MySQL options. None are editable.
Reviewers: codeblock, btrahan
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4326
Summary: these existed once, are no more, and don't get cleaned up in the current code path
Test Plan: storage destroy --dryrun -- noted the correct database names
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2237
Differential Revision: https://secure.phabricator.com/D4329
Summary: We interpret "size" as "size => true", and "true == 'full'", so we hit the wrong branch in the switch(). String cast explicitly.
Test Plan: Typed `{Fnnn, size}`; saw it render as a thumb instead of full.
Reviewers: btrahan, codeblock
Reviewed By: codeblock
CC: frozendevil, aran
Differential Revision: https://secure.phabricator.com/D4323
Summary:
- Move GC options into PHP.
- Remove the "run at" and "run for" options. The GC daemon doesn't actually do any table scans, is very gentle, and runs for like 3 seconds per day in any normal install. Just limit it to running once every 4 hours when it's caught up and call it a day.
Test Plan: Edited GC options.
Reviewers: btrahan, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4321
Summary: Use ApplicationTransactions in Config to create an edit history. Resolves T2256.
Test Plan: {F28477}
Reviewers: btrahan, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2256
Differential Revision: https://secure.phabricator.com/D4314
Summary: Show the value for all loaded configuration sources.
Test Plan:
{F28469}
{F28470}
{F28471}
Reviewers: btrahan, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4312
Summary:
See discussion in T2221. Before we can move configuration to the database, we have a bootstrapping problem: we need database credentials to live //somewhere// if we can't guess them (and we can only really guess localhost / root / no password).
Some options for this are:
- Have them live in ENV variables.
- These are often somewhat unfamiliar to users.
- Scripts would become a huge pain -- you'd have to dump a bunch of stuff into ENV.
- Some environments have limited ability to set ENV vars.
- SSH is also a pain.
- Have them live in a normal config file.
- This probably isn't really too awful, but:
- Since we deploy/upgrade with git, we can't currently let them edit a file which already exists, or their working copy will become dirty.
- So they have to copy or create a file, then edit it.
- The biggest issue I have with this is that it will be difficult to give specific, easily-followed directions from Setup. The instructions need to be like "Copy template.conf.php to real.conf.php, then edit these keys: x, y, z". This isn't as easy to follow as "run script Y".
- Have them live in an abnormal config file with script access (this diff).
- I think this is a little better than a normal config file, because we can tell users 'run phabricator/bin/config set mysql.user phabricator' and such, which is easier to follow than editing a config file.
I think this is only a marginal improvement over a normal config file and am open to arguments against this approach, but I think it will be a little easier for users to deal with than a normal config file. In most cases they should only need to store three values in this file -- db user/host/pass -- since once we have those we can bootstrap everything else. Normal config files also aren't going away for more advanced users, we're just offering a simple alternative for most users.
This also adds an ENVIRONMENT file as an alternative to PHABRICATOR_ENV. This is just a simple way to specify the environment if you don't have convenient access to env vars.
Test Plan: Ran `config set x y`, verified writes. Wrote to ENVIRONMENT, ran `PHABRICATOR_ENV= ./bin/repository`.
Reviewers: btrahan, vrana, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2221
Differential Revision: https://secure.phabricator.com/D4294
Summary:
This is somewhat clowny, particularly in how it handles JSON encode/decode, but
I've commented why I did things the way I did. The goal is to store minified JSON
but show pretty-printed JSON where possible, to the user editing it.
Test Plan:
* Went to /config/ and saw a list of keys from the `default` config.
* Clicked on one of them, submitted the default value successfully.
* Changed the value to invalid JSON and got a decent error.
* Changed the value to valid JSON and checked the DB to confirm it saved.
* Confirmed the DB values were minified.
* Confirmed the user-facing values were pretty-printed where they could be.
* Confirmed that PHIDs were getting assigned properly and that isDeleted
properly defaulted to false/0.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2246
Differential Revision: https://secure.phabricator.com/D4290
Summary: Currently, we have a configuration stack for unit tests, but they're built in to `PhabricatorEnv`. Pull them out and formalize them, so we can add more configuration sources (e.g., database).
Test Plan: Ran unit tests, web requests, scripts. This code had fairly good existing test coverage.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2223, T2221
Differential Revision: https://secure.phabricator.com/D4284
Summary:
We have a bunch of code duplication now between __init_script__.php and webroot/index.php. Consoldiate these methods and move them into PhabricatorEnv.
Merge PhabricatorRequestOverseer into PhabricatorStartup.
Test Plan: Loaded page, ran script. Wiped PHABRICATOR_ENV; loaded page, ran script; got errors.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2223
Differential Revision: https://secure.phabricator.com/D4283
Summary:
We have a lot of mess to get through before we can load libphutil and enter Phabricator code properly. Move it to a dedicated class.
I'm probably going to merge PhabricatorRequestOverseer into this, although the check that lives there now is kind of weird. It also does not really need to be a pre-load check and could be handled better.
I stopped shoving stuff in here once I got to ENV stuff, I'm going to tackle that next.
Test Plan: Ran phabricator normally; introduced fatals and misconfigurations. Grepped for changed symbols.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, asherkin
Maniphest Tasks: T2223
Differential Revision: https://secure.phabricator.com/D4282
Summary:
The search indexing API has several problems right now:
- Always runs in-process.
- It would be nice to push this into the task queue for performance. However, the API currently passses an object all the way through (and some indexers depend on preloaded object attributes), so it can't be dumped into the task queue at any stage since we can't serialize it.
- Being able to use the task queue will also make rebuilding indexes faster.
- Instead, make the API phid-oriented.
- No uniform indexing API.
- Each "Editor" currently calls SomeCustomIndexer::indexThing(). This won't work with AbstractTransactions. The API is also just weird.
- Instead, provide a uniform API.
- No uniform CLI.
- We have `scripts/search/reindex_everything.php`, but it doesn't actually index everything. Each new document type needs to be separately added to it, leading to stuff like D3839. Third-party applications can't provide indexers.
- Instead, let indexers expose documents for indexing.
- Not application-oriented.
- All the indexers live in search/ right now, which isn't the right organization in an application-orietned view of the world.
- Instead, move indexers to applications and load them with SymbolLoader.
Test Plan:
- `bin/search index`
- Indexed one revision, one task.
- Indexed `--type TASK`, `--type DREV`, etc., for all types.
- Indexed `--all`.
- Added the word "saboteur" to a revision, task, wiki page, and question and then searched for it.
- Creating users is a pain; searched for a user after indexing.
- Creating commits is a pain; searched for a commit after indexing.
- Mocks aren't currently loadable in the result view, so their indexing is moot.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: 20after4, aran
Maniphest Tasks: T1991, T2104
Differential Revision: https://secure.phabricator.com/D4261
Summary:
See discussion in D4204. Facebook currently has a 314MB remarkup cache with a 55MB index, which is slow to access. Under the theory that this is an index size/quality problem (the current index is on a potentially-384-byte field, with many keys sharing prefixes), provide a more general index with fancy new features:
- It implements PhutilKeyValueCache, so it can be a component in cache stacks and supports TTL.
- It has a 12-byte hash-based key.
- It automatically compresses large blocks of data (most of what we store is highly-compressible HTML).
Test Plan:
- Basics:
- Loaded /paste/, saw caches generate and save.
- Reloaded /paste/, saw the page hit cache.
- GC:
- Ran GC daemon, saw nothing.
- Set maximum lifetime to 1 second, ran GC daemon, saw it collect the entire cache.
- Deflate:
- Selected row formats from the database, saw a mixture of 'raw' and 'deflate' storage.
- Used profiler to verify that 'deflate' is fast (12 calls @ 220us on my paste list).
- Ran unit tests
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4259
Summary: Does this seem reasonable? It's a bit more compact than digest() (6 bits / byte instead of 4 bits / byte) and 72 bits of entropy @ 12 bytes instead of 128 bits of entropy @ 32 bytes. I feel like it's important to preserve the printability, though, and this seemed like a fairly good balance of concerns.
Test Plan: unit tests
Reviewers: vrana
Reviewed By: vrana
CC: aran, yemao932
Differential Revision: https://secure.phabricator.com/D4253
Summary:
- Build "sshd-auth" (for authentication) and "sshd-exec" (for command execution) binaries. These are callable by "sshd-vcs", located [[https://github.com/epriestley/sshd-vcs | in my account on GitHub]]. They are based on precursors [[https://github.com/epriestley/sshd-vcs-glue | here on GitHub]] which I deployed for TenXer about a year ago, so I have some confidence they at least basically work.
- The problem this solves is that normally every user would need an account on a machine to connect to it, and/or their public keys would all need to be listed in `~/.authorized_keys`. This is a big pain in most installs. Software like Gitosis/Gitolite solve this problem by giving you an easy way to add public keys to `~/.authorized_keys`, but this is pretty gross.
- Roughly, instead of looking in `~/.authorized_keys` when a user connects, the patched sshd instead runs `echo <public key> | sshd-auth`. The `sshd-auth` script looks up the public key and authorizes the matching user, if they exist. It also forces sshd to run `sshd-exec` instead of a normal shell.
- `sshd-exec` receives the authenticated user and any command which was passed to ssh (like `git receive-pack`) and can route them appropriately.
- Overall, this permits a single account to be set up on a server which all Phabricator users can connect to without any extra work, and which can safely execute commands and apply appropriate permissions, and disable users when they are disabled in Phabricator and all that stuff.
- Build out "sshd-exec" to do more thorough checks and setup, and delegate command execution to Workflows (they now exist, and did not when I originally built this stuff).
- Convert @btrahan's conduit API script into a workflow and slightly simplify it (ConduitCall did not exist at the time it was written).
The next steps here on the Repository side are to implement Workflows for Git, SVN and HG wire protocols. These will mostly just proxy the protocols, but also need to enforce permissions. So the approach will basically be:
- Implement workflows for stuff like `git receive-pack`.
- These workflows will implement enough of the underlying protocol to determine what resource the user is trying to access, and whether they want to read or write it.
- They'll then do a permissons check, and kick the user out if they don't have permission to do whatever they are trying to do.
- If the user does have permission, we just proxy the rest of the transaction.
Next steps on the Conduit side are more simple:
- Make ConduitClient understand "ssh://" URLs.
Test Plan: Ran `sshd-exec --phabricator-ssh-user epriestley conduit differential.query`, etc. This will get a more comprehensive test once I set up sshd-vcs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T550
Differential Revision: https://secure.phabricator.com/D4229
Summary: Some fallout from D4191.
Test Plan: Sent "D12" in IRC and got a response.
Reviewers: epriestley, vrana, zeeg
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4235
Summary:
Load the data for daemon worker tasks when viewing them, and present
the information in a useful way. This defaults to printing the json data,
but for some classes of worker it will also link to the corresponding
object, to make debugging problems with workers easier.
Test Plan:
load /daemon/task/NNN for a CommitParserWorker and a MetaMTAWorker, and
see the addition of a data field with useful content and link.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4226
Summary:
D4188 adds a preview of Paste contents to the list, but gets slow for large lists if you have Pygments installed since it has to spend ~200ms/item highlighting them. Instead, cache the highlighted output.
- Adds a Paste highlighting cache. This uses RemarkupCache because it has automatic GC and we don't have a more generalized cache for the moment.
- Uses the Paste cache on paste list and paste detail.
- Adds a little padding to the summary.
- Adds "..." if there's more content.
- Adds line count and language, if available. These are hidden on Mobile.
- Nothing actually uses needRawContent() but I left it there since it doesn't hurt anything and is used internally (I thought the detail view did, but it uses the file content directly, and must for security reasons).
Test Plan:
{F27710}
- Profiled paste list, saw good performance and few service calls.
- Viewed paste.
- Viewed raw paste content.
Reviewers: codeblock, btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4204
Summary: Include task ID and class when raising this exception. I took a brief stab at doing this generically, but (a) we specifically raise this exception outside of normal try/catch because we can't follow normal recovery rules for it and (b) we don't have a reasonable PhutilProxyException or similar right now which would preserve stack traces, and don't have builtin exception nesting support until PHP 5.3.
Test Plan: Faked this exception, verified we get more information in the logs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2193
Differential Revision: https://secure.phabricator.com/D4205
Summary:
When a user submits an action with no effect (like an empty comment, an "abandon" on an already-accepted revision, or a "close, resolved" on a closed task) we want to alert them that their action isn't effective. These warnings fall into two general buckets:
- User is submitting two or more actions, and some aren't effective but some are. Prompt them to apply the effective actions only.
- A special case of this is where the only effective action is a comment. We provide tailored text ("Post Comment") in this case.
- User is submitting one action, which isn't effective. Tell them they're out of luck.
- A special case of this is an empty comment. We provide tailored text in this case.
By default, the transaction editor throws when transactions have no effect. The caller can then deal with this, or use `PhabricatorApplicationTransactionNoEffectResponse` to provide a standard dialog that gives the user information as above. For cases where we expect transactions to have no effect (notably, "edit" forms) we just continue on no-effect unconditionally.
Also fix an issue where new, combined or filtered transactions would not be represented properly in the Ajax response (i.e., return final transactions from `applyTransactions()`), and translate some strings.
Test Plan:
- Submitted empty and nonempy comments in Macro and Pholio.
- Submitted comments with new and existing "@mentions".
- Submitted edits in both applications.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T912, T2104
Differential Revision: https://secure.phabricator.com/D4160
Summary: Currently, AphrontProxyResponse is expected to build a string. This prevents some response types (like Dialog) from being proxied, because they have special rules. Instead, make proxy responses reduce into a non-proxied response so it's possible to proxy any type of response and hit all the normal rules for it.
Test Plan: Built a proxied DialogResponse on top of this.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104, T912
Differential Revision: https://secure.phabricator.com/D4159
Summary:
Split Pholio's transaction implementation into generic and application-specific parts. Moves us toward generic transactions, with support for:
- Editing and deleting comments.
- Setting visibility of individual comments (I'm not a fan of this feature but we'll see).
I want to move everything to a more generic piece of infrastructure but there's very little they can share right now so adding transactions to, e.g., Paste or Macros (T2157) means massive amounts of similar code.
Tons of work left to do here, but I think it basically works. Here's a screenshot:
{F26820}
Test Plan: Made transactions in Pholio.
Reviewers: btrahan, vrana, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4136
Summary:
- Remove unused CSS rules and non-sprite images.
- Sprite the logo.
Test Plan: Looked at site, looked good.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4113
Summary:
Issues here:
- Need an application-sized "eye", or a "home" icon for "Phabricator Home".
- Some of the "apps_lb_2x" sliced images are the "_dark_" versions, not the light versions.
- If you slice an application-sized "logout" (power off) icon and application-sized "help" (questionmark in circle) icon I can replace the current menu icons and nearly get rid of "autosprite".
- To replace the icons on /applications/, the non-retina size is "4x", so we'd need "8x" for retina. Alternatively I can reduce the icon sizes by 50%.
- The "Help", "Settings" and "Logout" items currently have a "glowing" hover state, which needs a variant (or we can drop it).
- The /applications/ icons have a white hover state (or we can drop it).
- The 1x application (14x14) icons aren't used anywhere right now, should they be? Maybe in the feed in the future, etc?
- The "apps-2x" and "apps-large" sheets are the same image, but getting them to actually use the same file is a bit tricky, so I just left them separate for now.
Test Plan:
{F26698}
{F26699}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4108
Summary: Do we have an icon with 2x for the right menu?
Test Plan: {F26590}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4101
Summary:
This doesn't lay in everything, but:
- Break the buttons gradient apart into components and rebuild it (along with other gradients) into a single gradient sprite (possible after {D4099}).
- Use the sliced gradient for the crumbs background.
- Use the sliced image for the crumb divider.
- Adds the black/white app sheets, but I'm not generating them quite yet.
Test Plan: {F26537} {F26540}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4100
Summary: Switch to the final versions of these
Test Plan: Will add screenshots...
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4032
Summary:
- The filesystem is now the authority for which sprites are available. If you add new icons, the generation process will pick them up.
- I broke out icon generation and added retina support. App icon generation still uses the old method.
- Update ActionList and RemarkupControl to use the new sheet.
- Use white icons on hover.
- Also fixed a couple of minor issues with some stuff in Firefox/Chrome.
Test Plan:
{F25750}
{F25751}
{F25752}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2013
Differential Revision: https://secure.phabricator.com/D4027
Summary:
When users middle click or command-click an image, we should open it in a new tab, not open a lightbox.
See https://github.com/facebook/phabricator/issues/234
Test Plan: Left, middle, and command-clicked a lightbox image.
Reviewers: vrana, chad, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4020
Summary:
Just laying more groundwork out of ready-made UI.
{F22349}
Test Plan: Looked at a mock.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2097
Differential Revision: https://secure.phabricator.com/D3832
Summary:
I'm not going to land this until it's a bit more fleshed out since it would just confuse users, but this is probably more reviewable as a few diffs adding a couple features than one ULTRA-diff adding everything. Implement application basics for Pholio. This does more or less nothing, but adds storage, subscribe, flag, markup, indexing, query basics, PHIDs, handle loads, a couple of realy really basic controllers, etc.
Basic hierarchy is:
- **Moleskine**: Top-level object like a Differential Revision, like "Ponder Feed Ideas".
- **Image**: Each Moleskine has one or more images, like the unexpanded / expanded / mobile / empty states of feed.
- **Transaction**: Comment or edit, like Maniphest. I generally want to move most apps to a transaction model so we can log edits.
- **PixelComment**: Equivalent of an inline comment.
Test Plan: Created a fake object and viewed it.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran, davidreuss
Maniphest Tasks: T2097
Differential Revision: https://secure.phabricator.com/D3817
Summary: Companion diff to D4003.
Test Plan: Rigged up a response which loaded a CSS file with `body { background: red; }`, verified page turned red after I clicked the button.
Reviewers: vrana, chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T2086
Differential Revision: https://secure.phabricator.com/D4004
Summary:
People hit three issues with D3914:
- As per T2059, we applied a schema change from a `.php` patch, which currently does not work if you use a different user to make schema changes than for normal use.
- Since the change in question is idempotent, just move it to a `.sql` patch. We'll follow up in T2059 and fix it properly.
- Rogue daemons at several installs used old code (expecting autoincrement) to insert into the new table (no autoincrement), thereby creating tasks with ID 0.
- Rename the table so they'll fail.
- This also makes the code a little more consistent.
- Some installs now have tasks with ID 0.
- Use checks against null rather than against 0 so we can process these tasks.
The major issues this fixes are the schema upgrade failure in T2059, and the infinite loops in T2072 and elsewhere.
This isn't really a fully statisfactory fix. I'll discuss some next steps in T2072.
Test Plan: Created new tasks via MetaMTA/Differential. Ran tasks with `phd debug taskmaster`. Inserted a task 0 and verified it ran and archived correctly.
Reviewers: btrahan, vrana, nh
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2072, T2059
Differential Revision: https://secure.phabricator.com/D3973
Test Plan:
/diffusion/ARC/lint/master/src/, clicked on count link.
/diffusion/ARC/browse/master/src/difference/?lint=XHP9, clicked on file name.
/diffusion/ARC/browse/master/src/difference/ArcanistDiffUtils.php?lint=, verified that all messages are displayed.
/diffusion/ARC/browse/master/src/difference/ArcanistDiffUtils.php?lint=XHP9.
/diffusion/ARC/browse/master/src/difference/ArcanistDiffUtils.php?lint=TXT3, verified that 0 messages are displayed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3929
Summary: This saves lint errors to the path change of current commit. It requires pushed revision. It doesn't save difference from previous commit mentioned in T2038#comment-4 - I don't plan doing it after all, everything would be much more complicated and the amount of data saved with this approach isn't that bad.
Test Plan: Applied patch, ran script, verified DB.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2038
Differential Revision: https://secure.phabricator.com/D3899
Summary: don't need it now that uploading files is so easy. Plus it made for some buggy jonx if / when there were bad image links coupled with caching. In theory this is a lot less pretty though if folks linked to a bunch of files served elsewhere using images.
Test Plan: http://does-not-exist.com/imaginary.jpg rendered as a link!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2000
Differential Revision: https://secure.phabricator.com/D3908
Summary: See D3912 for discussion. InnoDB may reuse autoincrement IDs after restart; provide a way to avoid it.
Test Plan: Unit tests. Scheduled and executed tasks through `drydock lease --type host` and `phd debug taskmaster`.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3914
Summary:
- Remove EC2, RemoteHost, Application, etc., blueprints for now. They're very proof-of-concept and Blueprints are getting API changes I don't want to bother propagating for now. Leave the abstract base class and the LocalHost blueprint. I'll restore the more complicated ones once better foundations are in place.
- Remove the Allocate controller from the web UI. The original vision here was that you'd manually allocate resources in some cases, but it no longer makes sense to do so as all allocations come from leases now. This simplifies allocations and makes the rule for when we can clean up resources clear-cut (if a resource has no more active leases, it can be cleaned up). Instead, we'll build resources like the localhost and remote hosts lazily, when leases come in for them.
- Add some configuration to manage the localhost blueprint.
- Refactor `canAllocateResources()` into `isEnabled()` (for config checks) and `canAllocateMoreResources()` (for quota checks, e.g. too many resources are allocated already).
- Juggle some signatures to align better with a world where blueprints generally do allocate.
- Add some more logging and error handling.
- Fix an issue with log ordering.
Test Plan: Allocated some localhost leases.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3902
Summary:
use PhutilURI class to get the slug and the fragment, normalize the slug, and then glue it back together with another PhutilURI
fixes https://github.com/facebook/phabricator/issues/228
Test Plan: had a few links like [[ example/doc#title | wiki fun ]] and verified links generated were correct
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3894
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in LICENSE file and that it didn't modify externals.
Reviewers: epriestley, davidrecordon
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3886
Summary:
Tightens up a bunch of stuff:
- In `drydock lease`, pull and print logs so the user can see what's happening.
- Remove `DrydockAllocator`, which was a dumb class that did nothing. Move the tiny amount of logic it held directly to `DrydockLease`.
- Move `resourceType` from worker task metadata directly to `DrydockLease`. Other things (like the web UI) can be more informative with this information available.
- Pass leases to `allocateResource()`. We always allocate in response to a lease activation request, and the lease often has vital information. This also allows us to associate logs with leases correctly.
Test Plan: Ran `drydock lease --type host` and saw it perform a host allocation in EC2.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3870
Summary:
We lock tasks by setting `leaseOwner` to a unique value, but the value is currently unique-to-the-process rather than unique-to-the-query. This means that if a process leases a task, then leases another task, both tasks will have the same `leaseOwner`. This can cause an issue where we go to select the task we just leased and get the other task instead, if we aren't careful about the select construction.
We can avoid this by being clever and making sure the select is constructed correctly, but making the `leaseOwner` unique to the query is much simpler and more foolproof. This guarantees we always select only the rows we just leased.
Also remove `PhabricatorGoodForNothingWorker` since `PhabricatorTestWorker` fills its role of allowing things to be tested, and simplify the unit tests since we don't need to be clever about avoiding this issue any more.
Test Plan: Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3862
Summary:
This was the major goal of D3859/D3855, and to a lesser degree D3854/D3852.
As Drydock is allocating a resource, it may need to allocate other resources first. For example, if it's allocating a working copy, it may need to allocate a host first.
Currently, we have the process basically queue up the allocation (insert a task into the queue) and sleep() until it finishes. This is problematic for a bunch of reasons, but the major one is that if allocation takes more resources (host, port, machine, DNS) than you have daemons, they could all end up sleeping and waiting for some other daemon to do their work. This is really stupid. Even if you only take up some of them, you're spending slots sleeping when you could be doing useful work.
To partially get around this and make the CLI experience less dumb, there's this goofy `synchronous` flag that gets passed around everywhere and pushes the workflow through a pile of special cases. Basically the `synchronous` flag causes us to do everything in-process. But this is dumb too because we'd rather do things in parallel if we can, and we have to have a lot of special case code to make it work at all.
Get rid of all of this. Instead of sleep()ing, try to work on the tasks that need to be worked on. If another daemon grabbed them already that's fine, but in the worst case we just gracefully degrade and do everything in process. So we get the best of both worlds: if we have parallelizable tasks and free daemons, things will execute in parallel. If we have nonparallelizable tasks or no free daemons, things will execute in process.
Test Plan: Ran `drydock_control.php --trace` and saw it perform cascading allocations without sleeping or special casing.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3861
Summary:
- Clean up a TODO about permanent failures.
- Clean up a TODO about failing tasks after too many retries.
- Clean up a TODO about testing for bad leases.
- Make the lease/retry implementation more flexible and natural.
- Make completely bogus tasks fail permanently.
- Make PhabricatorMetaMTAWorker use new `getWaitBeforeRetry()` (as intended), not hackily implement logic in `getRequiredLeaseTime()`.
- Document worker hooks for failures and retries.
- Provide coverage on everything.
Test Plan: Ran unit tests. Ran `bin/phd debug taskmaster`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3859
Summary: This simplifies the fairly thorny logic of leasing tasks a bit. I'm planning to introduce another callsite shortly for Drydock.
Test Plan: Ran `bin/phd debug taskmaster`, observed sensible queries and correct operation.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3855
Summary:
When a PhabricatorTestCase dies after creating storage fixtures, it leaves
those storage fixtures around. This doesn't happen often, but when it does
happen it's a pain to cleanup. The --unittest-fixtures option helps automate
that cleanup.
Test Plan:
Ran it with --dryrun, then for real. Became overwhelmed with a Zen like peace
after regarding the tidiness and beauty of SHOW DATABASES.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3858
Summary: Make mobile-friendly and provide UI to cancel/retry tasks. Remove display of task data to arbitrary users, as it may be sensitive.
Test Plan:
{F22502}
{F22503}
{F22504}
{F22505}
{F22506}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3854
Summary:
Currently, when taskmasters complete a task it is immediately deleted. This prevents us from doing some general things, like:
- Supporting the idea of permanent failure (e.g., after N failures just stop trying).
- Showing the user how fast taskmasters are completing tasks.
- Showing the user how long tasks took to complete.
Having better visibility into this is important to Drydock, which builds on the task system. Also, generally buff debug output for task execution.
Test Plan: Ran `bin/phd debug taskmaster`. Ran `bin/phd debug garbage`. Queued some tasks via various systems.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3852
Test Plan: Looked at diff with several different lint errors, saw correct messages in their inline comments.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3827
Summary: you can now add, edit, and delete status events. also added a "description" to status events and surface it in the big calendar view on mouse hover. some refactoring changes as well to make validation logic centralized within the storage class.
Test Plan: added, edited, deleted. yay.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T407
Differential Revision: https://secure.phabricator.com/D3810
Summary: Followup to D3804. Makes Diffusion main comments (not just inlines) render properly with the modern markup pipeline.
Test Plan: Created previews and inline previews. Edited inlines. Saved comment, viewed comment. Verified caches were read and written using "Services" tab.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3805
Summary:
See T1963 for discussion of the Facebook-specific hack.
Differential currently uses a one-stage cache (render -> postprocess -> save in cache) rather than the two-stage cache (render -> save in cache -> postprocess) offered by `PhabricatorMarkupInteface`. This breaks Differential comments coming out of cache for the lightbox, and makes various other things suboptimal (status of handles like @mentions and embeds are not displayed accurately).
Instead, use the modern stuff.
Test Plan:
- Created preview comments and inlines in Differential.
- Edited a Differential inline.
- Submitted main and inline Differential comments.
- Viewed and edited Differential summary and test plan.
- Created preview comments and inlines in Diffusion.
- Submitted comments and inlines in Diffusion.
- Verified Differential now loads and saves to the generalized markup cache (Diffusion is close, but main comments still hold a single-stage cache).
- Verified old Differential comments work correctly with the lightbox.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1963
Differential Revision: https://secure.phabricator.com/D3804
Summary:
Provides a simple way for policy-aware queries to pre-filter results without needing to maintain separate cursors, and fixes a bunch of filter-related edge cases.
- For reverse-paged cursor queries, we previously reversed each individual set of results. If the final result set is built out of multiple pages, it's in the wrong order overall, with each page in the correct order in sequence. Instead, reverse everything at the end. This also simplifies construction of queries.
- `AphrontCursorPagerView` would always render a "<< First" link when paging backward, even if we were on the first page of results.
- Add a filtering hook to let queries perform in-application pre-policy filtering as simply as possible (i.e., without maintaing their own cursors over the result sets).
Test Plan: Made feed randomly prefilter half the results, and paged forward and backward. Observed correct result ordering, pagination, and next/previous links.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3787
Summary:
images attached to maniphest tasks and mentioned in remarkup anywhere now invoke a lightbox control that lets the user page through all the images.
lightbox includes a download button, next / prev buttons, and if we're not at the tippy toppy of hte page an "X" or close button.
we also respond to left, right, and esc for navigating.
next time we should get non-images working in here...!
Test Plan:
played with maniphest - looks good
made comments with images. looks good.
made sure multiple image comments worked.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, chad
Maniphest Tasks: T1896
Differential Revision: https://secure.phabricator.com/D3705
Summary: The property is called 'actor', not 'user'. Extend from Phobject to catch this class of error automatically. Upgrade a couple of getActor() to requireActor().
Test Plan: Created new users.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3776
