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
Summary: I need this shortly and it seems like something we're likely to need more of in the future now that fixtures work.
Test Plan: Ran unit tests. Used this productively in an upcoming diff.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3775
Summary:
Q1 to Q4 is used for parts of the year.
Also unlink all `[A-Z]0.*`, we don't route them anymore.
Test Plan:
Q0
Q1
Q10
Reviewers: epriestley, pieter
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3781
Summary: See comments.
Test Plan: Uploaded a small image in Safari via drag-and-drop.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3771
Summary:
We don't support this and say so in the documentation, but can check explicitly.
https://github.com/facebook/phabricator/issues/148
Test Plan: Set base-uri to stuff with/without paths, verified setup caught mistakes and gave useful errors.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3768
Summary:
See comment.
This can reveal some pretty bad bugs but HPHP handles this correctly so we already know about them.
Test Plan:
Added `phlog()` to `__call()` and observed what is defined for each method (under PHP). Also:
class C {
function __call($name, $args) {
static $class;
if (!$class) {
$class = get_class($this);
}
return $class;
}
}
class D extends C {
}
class E extends C {
}
$d = new D;
$e = new E;
var_dump($d->x());
var_dump($e->x()); // Prints D under PHP!
See also D3754.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1261
Differential Revision: https://secure.phabricator.com/D3753
Summary:
Allow skins to serve arbitrary resources without needing to be mapped, so we can have a vibrant community of amateur skinners.
For "basic" skins, just put all the "css/" on the page always.
Includes an image to prove that works.
@vrana, pretty sure this has no impact outside of Phame but it does change Celerity so it might be to blame if there's any weirdness with static resources.
Test Plan:
{F21341}
{F21340}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3719
Summary: Restore summarization. Use the remarkup cache, and try to do it somewhat-intelligently (pick the first paragraph that looks like it's text).
Test Plan:
{F21323}
{F21324}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3715
Summary:
Two high-level things happening here:
- We no longer ever need to put meta-UI (content creation, editing, notices, etc.) on live blog views, since this is all in Phame now. I pulled this out.
- On the other hand, I pushed more routing/control logic into Skins and made the root skin a Controller instead of a View. This simplifies some of the code above skins, and the theory behind this is that it gives us greater flexibility to, e.g., put a glue layer between Phame and Wordpress templates or whatever else, and allows skins to handle routing and thus add pages like "About" or "Bio".
- I added a basic skin below the root skin which is more like the old root skin and has standard rendering hooks.
- "Ten Eleven" is a play on the popular (default?) Wordpress themes called "Twenty Ten", "Twenty Eleven" and "Twenty Twelve".
Test Plan: Viewed live blog and live posts. They aren't pretty, but they don't have extraneous resources.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3714
Summary: See discussion in D3722. Good catch -- I'd definitely say this is a bug.
Test Plan: Rendered two <div />s, one with mustcapture set to `true` and one to `false`. The first got `data-mustcapture`; the second did not.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3723
Summary: I think this is simpler and better than making them conditional. In properly configured installs this should have no impact (they already use a CDN URI). In not-quite-properly configured installs this will add a trivial, highly-compressible number of bytes to the source. In all cases we have less code.
Test Plan: Loaded some pages, everything worked.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3709
Summary:
This leaves the UI in a pretty rough state, but implements blog policy controls and queries, and 1:1 relationships between posts and blogs. Needs a bunch more cleanup but seemed like an okayish breaking point in terms of cohesiveness.
Posts have these rules:
- Drafts are visible only to the author.
- Published posts are visible to anyone who can see the blog they appear on.
- Posts are only editable by the author.
...so we don't need any special policy UI or state to accommodate these rules.
Posts may have no blog if they're grandfathered in or you write a post to a blog and then lose the ability to see the blog. This is the messiest edge case -- specifically:
- You write a post to blog A.
- You publish the post.
- I edit the "Visible To:" for blog A and set it to exclude you.
What we do in this case is let you see the post in "My Posts", but you can no longer see the blog and you'll see the post as not being part of a blog. We can maybe give you some UI to let you move it later or something.
Test Plan: Hit all (I think?) of the interfaces without issues. Definitely some UI problems still right now.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3694
Summary:
Adds "can view" and "can edit" policies to blogs. Replaces "bloggers" with "can join".
This doesn't fully remove "bloggers" because I didn't want this to get too crazy/huge.
Test Plan: Created, edited, deleted blogs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3693
Summary: Provide array() default so we don't foreach() over null in the case of a missing config (from @dctrwatson).
Test Plan: Will verify with @dctrwatson.
Reviewers: vrana, btrahan, dctrwatson
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3686
Summary: When we receive an email, figure out if any of the other tos and ccs are users. If they are, pass their phids through the stach as "exclude phids" and exclude them from getting the email.
Test Plan: used the various applications (audit, differential, maniphest) and noted emails were sent as expected.
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin, vrana
Maniphest Tasks: T1676
Differential Revision: https://secure.phabricator.com/D3645
Summary:
Blames to D3659.
It's not that old (< 1 day) to fix it properly (add patch for dropping xhpast DB).
Test Plan:
$ arc unit src/applications/calendar/storage/__tests__/PhabricatorCalendarHolidayTestCase.php
Reviewers: btrahan, epriestley
Reviewed By: btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3670
Summary: ...and use 'em in the phame blog case.
Test Plan: viewed blog.phabricator.dev and it actually looked right!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3666
Summary:
We need to go slightly farther to stub reply handler functionality for Ponder in at least some configurations, where we rely on the presence of a unique random key to generate per-object or per-object+user reply addresses.
This should probably be formalized in an interface since it's currently pretty ad-hoc.
Test Plan:
- Made comments in Ponder under a per-user email configuration.
- Ran migration, verified mail keys were generated.
- Ran migration again (with --apply), verified existing questions were skipped.
- Created a new question, verified mail key generation.
Reviewers: pieter
Reviewed By: pieter
CC: aran
Maniphest Tasks: T1873
Differential Revision: https://secure.phabricator.com/D3665
Summary:
It isn't deleted by `storage destroy`.
This should be a no-op on current storage because we execute `CREATE DATABASE IF NOT EXISTS`.
Test Plan:
$ bin/storage destroy --dryrun
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3659
Summary: For immutable objects, just use the ID as a cursor.
Test Plan:
- Analyzed commits from an empty cursor.
- Checked that cursor was good.
- Pulled some more commits.
- Analyzed commits again, verified it only hit the new ones.
- Verified the graph of "Count of CMIT" looked reasonable.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1866
Differential Revision: https://secure.phabricator.com/D3656
Summary:
Alternate proposal for D3635.
- Works better with small images.
- Produces a predictable thumbnail size.
- Somewhat reasonable output on 3000x10 images.
- Increase the size of Macro thumbnails to 240px.
Test Plan: {F20497}
Reviewers: vrana, chad
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3638
Summary:
Basic infrastructure for generalizing subscriptions/CCs for T1808, T1514 and T1663.
- Implement `PhabricatorSubscribableInterface` and you'll get a subscribe/unsubscribe button for free.
- If there are any auto-subscribed users (like the question author) you can specify them; this makes more sense for Tasks and Revisions than Ponder probably, but maybe the author should be auto-subscribed.
- Subscriptions are either "explicit" (the user clicked 'subscribe') or "implicit" (the user did something which causes them to become subscribed naturally). If a user unsubscribes, they'll no longer be added by implicit subscriptions. This may or may not be relevant to Ponder but is an existing Herald feature in Differential.
- Helper method on PhabricatorSubscribersQuery to load subscribers.
- This doesn't handle actually sending email, etc. I think that's all so application-specific that it doesn't belong here.
- Now seems to work.
Test Plan:
{F20552}
{F20553}
Reviewers: pieter, btrahan
Reviewed By: pieter
CC: aran
Maniphest Tasks: T1663, T1514, T1808
Differential Revision: https://secure.phabricator.com/D3637
Summary: We never use this and almost certainly never will. It's been in Lisk for ~7 years but is a solution in search of a problem. It causes a conflict with any DAO that has a `version` column.
Test Plan: Browsed around, performed inserts and updates. Edited a Phriction document.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, leslie.chong
Differential Revision: https://secure.phabricator.com/D3625
Summary:
Fixes a TODO, and silences a warning introduced by D3601.
There are several cases where we load data like:
SELECT *, ... AS extraData FROM ...
...and then pass it to `loadAllFromArray()`. Currently, this causes us to set an `extraData` property on the object.
This idiom seems fairly useful and non-dangerous, so I made `loadFromArray()` just drop extra keys.
Since we hit this loop a potentially huge number of times (10,000+ for full Maniphest pages) I did some microoptimization. Lisk is hot enough that it's one of the few places where it's worthwhile (see D1291).
Test Plan: Loaded homepage, no longer got warnings about `viewerIsMember` from Project queries. Browsed ~10 apps, didn't see any issues.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3606
Summary:
I make this error quite often: I forget to declare a property I am writing to or I make a typo in it.
PHP implicitly creates a public property which I don't like.
I would much rather see a linter warning me against this than this runtime check but writing it is very difficult:
- We need to explore all parents of the class we are checking.
- It is even possible that children will declare that property but it's OK to treat this as error anyway.
- We can extend also builtin or external classes.
- It's somewhat doable for `$this` but even more complex for any `$obj` because we don't know the class of it.
This should catch significant part of these errors and I'm fine with that.
I don't plan escalating to exception because this error is not fatal and should not stop the application from working.
Test Plan: Loaded homepage, checked log.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3601
Summary:
Calling `->setPHID()` or other common Lisk setters creates an implicit public property `$phid`.
I don't like implicit properties and I see them as errors.
Its public visibility also makes me nervous and is vulnerable to bypassing any setters we may create.
Test Plan: Loaded homepage, checked log.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3600
Summary: this then enables people to create blog.theircompany.com. And for us, blog.phacility.com...!
Test Plan:
- created custom URIs of various goodness and verified the error messages were sensical.
- verified if "false" in configuration then custom uri stuff disappears
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3542
Summary: We want to allow a broader access to our installation but we need to check the request in that case.
Test Plan:
Created a simple `PhabricatorRequestChecker` returning a custom controller.
Verified that this controller is used when accessing any page.
Returned `null` from this checker and verified that all 209 Phabricator pages are accessible.
Reviewers: epriestley
Reviewed By: epriestley
CC: scottmac, aran, Korvin, btrahan
Differential Revision: https://secure.phabricator.com/D2488
Summary:
It happens to me quite often that I leave the window with revision (by closing it or by visiting a link from it).
When I return then the comment draft is there so I clowncopterize it but forget that I wanted to take some other action than Comment.
Test Plan: Selected "Add Reviewers", added some reviewers, closed the window, opened it - the action and reviewers were still there.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3530
Summary:
We have two troubles with this script:
# Our revisions and commits don't fit in the memory. (Our tasks do :-).)
# Reindexing revisions is slow.
Test Plan: Ran it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3483
Summary:
People have occasionally complained about phabricator being slow. We have
the access log to look at to see when slowness happens, but it doesn't tell
us much about why it happened. Since it's usually a sporadic issue that's
reported, it's hard to reproduce and then profile. This change will allow us
to collect sampled profiles so we can look at them when slowness occurs.
Test Plan:
checking that sampling works correctly:
- set rate to 0; do several page loads; check no new entries in table
- set rate to 1; check that there's a new row in the table for each page load
- set rate to 10; check that some requests write to table and some don't
check new ui for samples:
- load /xhprof/list/all/, see a list with a lot of samples
- load /xhprof/list/sampled/, see only sampled runs
- load /xhprof/list/manual/, see only non-sampled runs
- load /xhprof/list/my-runs/, se only my manual runs
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3458
Summary:
- Renames `PhabricatorPolicyQuery` to `PhabricatorPolicyAwareQuery` (a query which respects policy settings).
- Introduces `PhabricatorPolicyQuery`, which loads available policies (e.g., "member of project X").
- Introduces `PhabricatorPolicy`, which describes a policy.
- Allows projects to be set as policies.
- Allows Paste policies to be edited.
- Covers crazy cases where you make projects depend on themselves or each other because you are a dastardly villan.
Test Plan: Set paste and project policies, including crazy policies like A -> B -> A, A -> A, etc.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3476
Summary: Add storage to Pastes for view policies.
Test Plan: Set policies on pastes, see next diff.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3474
Summary: This is pretty spartan, but it does the job.
Test Plan:
Patch, update storage, add some comment
to your favorite question or answer.
Reviewers: nh, vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin, starruler, syrneus, me.here, victorzarate7
Maniphest Tasks: T1645
Differential Revision: https://secure.phabricator.com/D3471
Summary:
I want to implement a `{P123}` rule to embed pastes, but we need viewers everywhere before it will work with privacy.
This is not exhaustive; many Remarkup callsites haven't been converted to `PhabricatorMarkupInterface` yet.
Test Plan: Looked at Maniphest, Differential, Diffusion, Phriction; added markup, made edits and hit previews.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3428
Summary:
I've replaced all `id(new PhabricatorObjectHandleData(...))->loadHandles()` by `$this->loadViewerHandles(...)`.
Lint caught one usage in a static method.
Test Plan: Displayed revision with sporadic author.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3432
Summary: Previously, the identification string was thrown at the server long before you were connected, I've moved this to the end of the motd raw, and now errthangz gud
Test Plan: Register an account for your bot to use, give your bot the correct nick and password, then watch
Reviewers: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D3410
Summary:
Currently, CelerityController extends AphrontController, not PhabricatorController. (I think I imagined Celerity being somewhat stand-alone and didn't want to create a dependency.)
This creates a concrete problem if a static resource is missing, since we throw an exception, but the higher-level exception handlers depend on the User existing in order to show an appropriate response page. This is the only controller which doesn't extend PhabricatorController, and it doesn't seem worthwhile to make a weird edge case out of it.
Specific repro case is:
- Remove `externals/javelin/` (or forget to run `git submodule update --init`).
- Load a static resource.
- Get "[Rendering Exception] Argument 1 passed to PhabricatorMainMenuView::setUser() must be an instance of PhabricatorUser, null given, called in /services/apache/phabricator/phabricator/src/view/page/PhabricatorStandardPageView.php on line 435 and defined"
Test Plan:
- Followed above steps, no more fataling.
- Verified this is the only weird controller.
Reviewers: voldern, vrana, btrahan
Reviewed By: voldern
CC: aran
Differential Revision: https://secure.phabricator.com/D3389
Summary:
I like systems that just work. It is possible to store files larger than max_allowed_packet in MySQL and we shouldn't demand it.
It also fixes a problem when file was smaller than `storage.mysql-engine.max-size` but its escaped version was larger than `max_allowed_packet`.
Test Plan: Reduced the size to 5e4, uploaded 90 kB file, checked the queries in DarkConsole, downloaded the file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3392
Summary: In my haste, I forgot a trailing ?
Test Plan: Try both "Where is Derp?" and "Where in the world is Derp?"
Reviewers: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D3387
Summary:
This does a few things:
- Allows you to flag pastes. This is straightforward.
- Allows Applications to register event listeners.
- Makes object action lists emit a 'didrenderactions' event, so other applications can add more actions. The Flags application injects its action in this way. This should generally make it much easier to add actions to objects when we add new applications, with less code duplication and better modularity. We have a really hacky version of this in Differential that I want to get rid of in lieu of this more general approach. I'm going to make object lists do the same thing, so any application can jump in and add stuff.
Test Plan: Flagged and unflagged pastes. Viewed home page, differential, flags list.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3377
Summary:
More and more relations are going under edges and I can't work with them from Relatives framework.
This doesn't have the nice transitive property of normal relatives (loading relative objects from relatives loads all of them at once) but I can add it when I need it.
I plan to use it in D3085 (after converting relationships to edges).
Test Plan:
$task = id(new ManiphestTask())
->loadOneWhere('phid = %s', $phid);
print_r($task->loadRelativeEdges(4));
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3344
Summary:
Currently, when a user runs "arc diff" and the diff exceeds PHP's 'post_max_size', they get a very confusing and irrelevant error about a missing Conduit session token. The reason for this is that 'post_max_size' doesn't build $_POST, so //all// the data is missing.
We try to detect this, but currently only do so effectively for specific file upload forms. Broaden the detection to cover all cases.
Previously, we ran into an issue where Firefox + HTML5 drag-and-drop uploads would get a false positive on this detection. I dug into this and added the Content-Type checks, which correctly handle that case.
Test Plan: With small and large 'post_max_size', ran small and large normal, HTML5 and multipart/form-data POST requests against Phabricator in Safari and Firefox. Got desired beahviors.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: tido, aran
Differential Revision: https://secure.phabricator.com/D3320
Summary: We use numbers here and I see no reason for strings.
Test Plan:
$ bin/storage upgrade
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3303
Summary: Added match to the novel statement: Where in the world is derp?
Test Plan: Say something like "Where in the world is CarmenSandiego?"
Reviewers: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D3318
Summary:
See D3126, T1667, T1658. Prior to D3126, `phd` did not use MySQL directly. Now that it does, there are at least two specific problems (see inline comment).
In the long term, we should probably break this dependency and use Conduit. However, we don't currently have access to the daemon log ID and getting it is a mess (the overseer generates it), and I think I want to rewrite how all this works at some point anyway (the daemon calls are currently completely unauthenticated, which is silly -- we should move them to an authenticated channel at some point, I think).
Test Plan: Ran `phd stop` with a bad MySQL config against a non-running daemon, didn't get a query error.
Reviewers: nh, vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1667, T1658
Differential Revision: https://secure.phabricator.com/D3314
Summary: We need to open the envelope here.
Test Plan: Ran `bin/storage dump` without errors.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3315
Summary:
You can now embed countdowns in Remarkup! Not sure what it's
useful for, but there you have it.
Also I may have made a hash of the markup code; I don't really know what
I'm doing.
Test Plan: Make a new countdown, put `{C###}` in a Differential comment.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1053
Differential Revision: https://secure.phabricator.com/D3290
Summary: This is arguably a more useful view than listing all daemons.
Test Plan: Looked at list, only saw daemons that haven't exited
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3286
Summary:
Fact engines loading dependent objects are super slow because they load them one by one.
This diff put each page in a Lisk set allowing engines to use `loadRelatives()`.
It also introduces `clearSet()` method which is somewhat neccessary in PHP < 5.3 or with disabled cyclic [[ http://php.net/gc | GC ]].
Test Plan:
$iterator = new PhabricatorFactUpdateIterator(new DifferentialRevision());
foreach ($iterator as $revision) {
$diffs = $revision->loadRelatives(new DifferentialDiff(), 'revisionID');
echo memory_get_usage() . "\n";
}
Experienced not-steadily-increasing memory usage and much faster loading.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3247
Summary:
Currently, we're showing projets in reverse order (Z..A) because most cursor pagers go from high IDs to low IDs.
Allow sequence to be reversed; reverse it.
Also simplify some query/paging stuff.
Test Plan: Set page size to 1, paged back and forth.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3221
Summary:
- In ProjectQuery, always load the viewer's membership in the project because we need it to perform a CAN_VIEW test.
- Add storage for the view, edit and join policies.
- A user can always view a project if they are a member.
- A user can always join a project if they can edit it.
- Editing a project requires both "view" and "edit" permissions, and edit does not imply view.
- This has no effect on the application yet.
Test Plan: See next diff.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3219
Summary:
- Allow PolicyQuery to require specific sets of capabilities other than "CAN_VIEW", like edit, etc. The default set is "view".
- Add some convenience methods to PolicyFilter to test for capabilities.
Test Plan: Viewed pastes, projects, etc. Used other stuff in future diff.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3212
Summary:
We currently cache all connections in LiskDAO so we can roll back transactions when fixtured unit tests complete.
Since we establish a new connection wrapper each time we establish a global lock, this cache currently grows without bound.
Instead, pool global lock connections so we never have more than the largest number of locks we've held open at once (in PullLocalDaemon, always 1).
Another way to solve this is probably to add an "onclose" callback to `AphrontDatabaseConnection` so that it can notify any caches that it been closed. However, we currently allow a connection to be later reopened (which seeems reasonable) so we'd need a callback for that too. This is much simpler, and this use case is unusual, so I'd like to wait for more use cases before pursing a more complicated fix.
Test Plan:
Ran this in a loop:
while (true) {
for ($ii = 0; $ii < 100; $ii++) {
$lock = PhabricatorGlobalLock::newLock('derp');
$lock->lock();
$lock->unlock();
}
$this->sleep(1);
}
Previously it leaked ~100KB/sec, now has stable memory usage.
Reviewers: vrana, nh, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1636
Differential Revision: https://secure.phabricator.com/D3239
Summary:
Ponder is similar in spirit to the Wiki tool, but uses a Q&A
format and up/downvotes to signal user sentiment. Popular
questions are moved to the top of the feed on a 5-minute
cycle based on age (younger is better) and vote count (higher
is better).
Pre-apologies for noob diff.
Test Plan:
- `./bin/phd list` Should include `PonderHeatDaemon`; phd launch it
if necessary.
- Navigate to /ponder/ ; observe sanity when adding questions,
voting on them, and adding answers.
- Confirm that questions and answers are linkable using Q5 / Q5#A5 formatted object links.
- Confirm that searching for Ponder Questions works using built-in
search.
Feedback on code / schema / whatever organization very welcome.
Reviewers: nh, vrana, epriestley
Reviewed By: epriestley
CC: gmarcotte, aran, Korvin, starruler
Differential Revision: https://secure.phabricator.com/D3136
Summary: Apparently I am not qualified to do basic math.
Test Plan: Unit test.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3218
Summary: See discussion in D3078 for why I've separated this. Pretty sure it's not quite ready yet -- I want to build a couple of things on it so we have a better idea of what we need (autoincrement ID? <factType, objectA, epoch> primary key? objectB column? valueZ?) and don't need to do a ton of schema patches.
Test Plan: Applied patches, ran D3078.
Reviewers: vrana, btrahan, majak
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1581, T1562
Differential Revision: https://secure.phabricator.com/D3088
Summary: I think this is simpler? Includes test cases.
Test Plan: Ran tests. Loaded /paste/.
Reviewers: vrana, nh
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3209
Summary: See title. Adds features needed for D3136.
Test Plan:
Observe sanity (or run D3136 in a sandbox
and observe that voting works).
Reviewers: epriestley
Reviewed By: epriestley
CC: gmarcotte, aran, Korvin
Differential Revision: https://secure.phabricator.com/D3205
Summary:
A few goals here:
- Slightly simplify the Query classtree -- it's now linear: `Query` -> `OffsetPagedQuery` (adds offset/limit) -> `PolicyQuery` (adds policy filtering) -> `CursorPagedPolicyQuery` (adds cursors).
- Allow us to move from non-policy queries to policy queries without any backward compatibility breaks, e.g. Conduit methods which accept 'offset'.
- Separate the client limit ("limit") from the datafetch hint limit ("rawresultlimit") so we can make the heurstic smarter in the future if we want. Some discussion inline.
Test Plan: Expanded unit tests to cover offset behaviors.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3192
Summary:
- Commit detail view
- List of projects
- "edit" action which takes the user to a simple form where they can only add / remove projects.
- Integrated the project relationship into the commit search indexer
- fixed a bug from D790; it seems you must select the column if you're going to join against it later. Without this change searching for author or projectfails 100% for me.
Test Plan: added and removed projects. verified appropriate projects showed up in detail and edit view. searched for commits by project and found the ones I was supposed to...!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1614
Differential Revision: https://secure.phabricator.com/D3189
Summary:
- Store project members in edges.
- Migrate existing members to edge storage.
- Delete PhabricatorProjectAffiliation.
- I left the actual underlying data around just in case something goes wrong; we can delete it evenutally.
Test Plan:
- Ran migration.
- Created a new project.
- Joined and left a project.
- Added and removed project members.
- Manually called PhabricatorOwnersOwner::loadAffiliatedUserPHIDs() to verify its behavior.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3186
Summary:
I'm trying to make progress on the policy/visibility stuff since it's a blocker for Wikimedia.
First, I want to improve Projects so they can serve as policy groups (e.g., an object can have a visibility policy like "Visible to: members of project 'security'"). However, doing this without breaking anything or snowballing into a bigger change is a bit awkward because Projects are name-ordered and we have a Conduit API which does offset paging. Rather than breaking or rewriting this stuff, I want to just continue offset paging them for now.
So I'm going to make PhabricatorPolicyQuery extend PhabricatorOffsetPagedQuery, but can't currently since the `executeWithPager` methods would clash. These methods do different things anyway and are probably better with different names.
This also generally improves the names of these classes, since cursors are not necessarily IDs (in the feed case, they're "chronlogicalKeys", for example). I did leave some of the interals as "ID" since calling them "Cursor"s (e.g., `setAfterCursor()`) seemed a little wrong -- it should maybe be `setAfterCursorPosition()`. These APIs have very limited use and can easily be made more consistent later.
Test Plan: Browsed around various affected tools; any issues here should throw/fail in a loud/obvious way.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3177
Summary:
See T1602.
This is just the minimal functional patch; the scripts will continue
working because of the `DEFAULT ''`.
Test Plan:
Can't fully test this until I get more code working, but
nothing broke horribly yet.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran, Korvin
Maniphest Tasks: T1602
Differential Revision: https://secure.phabricator.com/D3147
Summary: The Log and PID directory should be separable in the config file
Test Plan: Start the daemons, and check if the pid and log files are stored in directories that were specified in the config file.
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3149
Summary:
- Adds a new "Applications" application.
- Builds an application list via application config instead of via hard-coding, so we can move toward better concepts of installing/uninstalling applications, etc.
- Applications indicate that they need attention with notice counts and brief status messages rathern than 50 giant tables of all sorts of app data.
I want to try replacing the home screen with this screen, pretty much. Not sure if this is totally crazy or not. What does everyone else think?
Test Plan: Will add screenshots.
Reviewers: btrahan, chad, vrana, alanh
Reviewed By: vrana
CC: aran, davidreuss, champo
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3129
Summary:
To make it easier to monitor daemons, let's store their current state
(running, died, exited, or unknown) to the db. The purpose of this is to
provide more information on the daemon console about the status of daemons,
especially when they are running on multiple machines. This is mostly backend
work, with only a few frontend changes. (It is also dependent on a change
to libphutil.)
These changes will make dead or stuck daemons more obvious, and will allow
more work on the frontend to hide daemons (and logs) that have exited cleanly,
i.e. ones we don't care about any more.
Test Plan:
- run db migration, check in db that all daemons were marked as exited
- start up a daemon, check in db that it is marked as running
- open web interface, check that daemon is listed as running
- after daemon has been running for a little bit, check in db that dateModified
is being updated (indicating daemon is properly sending heartbeat)
- kill -9 daemon (but don't run bin/phd yet), and check that db still shows it
as running
- edit daemon db entry to show it as being on a different host, and backdate
dateModified field by 3 minutes, and check the web ui to show that the status
is unknown.
- change db entry to have proper host, check in web ui that daemon status is
displayed as dead. Check db to see that the status was saved.
- run bin/phd stop, and see that the formerly dead daemon is now exited.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3126
Summary: You need to use -- to separate arguments for phd and the daemon.
Test Plan: Ran with the extra --.
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3106
Summary:
Some facts are aggregations of other facts. For example, we may compute how many times each macro is used in each object as a "raw fact":
Dnnn uses macro "psyduck" 6 times.
But we want to present this data in aggregate form, e.g. "order macros by popularity". We can do this at runtime and it probably won't be too awful a query, but we can also aggregate it cheaply:
Macro "psyduck" is used 3920 times across all objects.
...and then do a query like "select macros ordered by usage".
"Aggregate" facts support facts like this. The aggregate facts I've implemented are:
- Count of all objects.
- Count of objects of type X.
- Last time facts were updated.
These clearly fit the "aggregate" facts template well. I'm not 100% sure macros do. We can use this table to answer a question like "What are the most popular macros, ordered by use?" We can also use it to answer a question like "What are the most popular macros in the last 6 months?", if we build a specific fact for that. But we can't use it to answer a question like "What are the most popular macros between times X and Y?". Maybe that's important; maybe not.
This seems like a good fit for at least some types of facts.
I'll de-magic the keys a bit in the next diff.
Test Plan: Ran the engines and got some aggregated facts about other facts.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1562
Differential Revision: https://secure.phabricator.com/D3089
Summary:
Basic "Fact" application with some storage, part of a daemon, and a control binary.
= Goals =
The general idea is that we have various statistics we'd like to compute, like the frequency of image macros, reviewer responsiveness, task close rates, etc. Computing these on page load is expensive and messy. By building an ETL pipeline and running it in a daemon, we can precompute statistics and just pull them out of "stats" tables.
One way to do this is just to completely hard-code everything, e.g. have a daemon that runs every hour which issues a big-ass query and dumps results into a table per-fact or per fact-group. But this has a bunch of drawbacks: adding new stuff to the pipeline is a pain, various fact aggregators can't share much code, updates are slow and expensive, we can never build generic graphs on top of it, etc.
I'm hoping to build an ETL pipeline which is generic enough that we can use it for most things we're interested in without needing schema changes, and so that installs can use it also without needing schema changes, while still being specific enough that it's fast and we can build useful stuff on top of it. I'm not sure if this will actually work, but it would be cool if it does so I'm starting pretty generally and we'll see how far I get. I haven't built this exact sort of thing before so I might be way off.
I'm basing the whole thing on analyzing entire objects, not analyzing changes to objects. So each part of the pipeline is handed an object and told "analyze this", not handed a change. It pretty much deletes all the old data about that thing and then writes new data. I think this is simpler to implement and understand, and it protects us from all sorts of weird issues where we end up with some kind of garbage in the DB and have to wipe the whole thing.
= Facts =
The general idea is that we extract "facts" out of objects, and then the various view interfaces just report those facts. This change has on type of fact, a "raw fact", which is directly derived from an object. These facts are concerete and relate specifically to the object they are derived from. Some examples of such facts might be:
D123 has 9 comments.
D123 uses macro "psyduck" 15 times.
D123 adds 35 lines.
D123 has 5 files.
D123 has 1 object.
D123 has 1 object of type "DREV".
D123 was created at epoch timestamp 89812351235.
D123 was accepted by @alincoln at epoch timestamp 8397981839.
The fact storage looks like this:
<factType, objectPHID, objectA, valueX, valueY, epoch>
Currently, we supprot one optional secondary key (like a user PHID or macro PHID), two optional integer values, and an optional timestamp. We might add more later. Each fact type can use these fields if it wants. Some facts use them, others don't. For instance, this diff adds a "N:*" fact, which is just the count of total objects in the system. These facts just look like:
<"N:*", "PHID-xxxx-yyyy", ...>
...where all other fields are ignored. But some of the more complex facts might look like:
<"DREV:accept", "PHID-DREV-xxxx", "PHID-USER-yyyy", ..., ..., nnnn> # User 'yyyy' accepted at epoch 'nnnn'.
<"FILE:macro", "PHID-DREV-xxxx", "PHID-MACR-yyyy", 17, ..., ...> # Object 'xxxx' uses macro 'yyyy' 17 times.
Facts have no uniqueness constraints. For @vrana's reviewer responsiveness stuff, we can insert multiple rows for each reviewer, e.g.
<"DREV:reviewed", "PHID-DREV-xxxx", "PHID-USER-yyyy", nnnn, ..., mmmm> # User 'yyyy' reviewed revision 'xxxx' after 'nnnn' seconds at 'mmmm'.
The second value (valueY) is mostly because we need it if we sample anything (valueX = observed value, valueY = sample rate) but there might be other uses. We might need to add "objectB" at some point too -- currently we can't represent a fact like "User X used macro Y on revision Z", so it would be impossible to compute macro use rates //for a specific user// based on this schema. I think we can start here though and see how far we get.
= Aggregated Facts =
These aren't implemented yet, but the idea is that we can then take the "raw facts" and compute derived/aggregated/rollup facts based on the raw fact table. For example, the "count" fact can be aggregated to arrive at a count of all objects in the system. This stuff will live in a separate table which does have uniqueness constraints, and come in the next diff.
We might need some kind of time series facts too, not sure about that. I think most of our use cases today are covered by raw facts + aggregated facts.
Test Plan: Ran `bin/fact` commands and verified they seemed to do reasonable things.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, majak
Maniphest Tasks: T1562
Differential Revision: https://secure.phabricator.com/D3078
Summary: PhutilBufferedIterator now implements all the nonspecific logic here.
Test Plan:
Created a test script like this:
$iterator = new LiskMigrationIterator(new DifferentialRevision());
$iterator->setPageSize(3);
foreach ($iterator as $key => $rev) {
echo "{$key}: ".$rev->getID()."\n";
}
Ran it and verified sensible iteration results.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1562
Differential Revision: https://secure.phabricator.com/D3076
Summary: Also move the other tests up so they'll trigger when this stuff is touched.
Test Plan: liberate
Reviewers: nh, btrahan, vrana
Reviewed By: nh
CC: aran
Maniphest Tasks: T1283
Differential Revision: https://secure.phabricator.com/D3074
Summary:
Currently, we have this cumbersome `PhabricatorRepositoryCommitMessageDetailParser` hook. This is really old and outdated; I want to just use the Differential custom field parser. See T945 for a specific application.
However, it allows installs to override author/committer association. Instead, provide an event hook for doing this.
Test Plan: Added a listener, made every commit resolve to "turtle", parsed some commits, verified the events looked sane and they now correctly were all attributed to "turtle".
Reviewers: btrahan, vrana, nh
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1337
Differential Revision: https://secure.phabricator.com/D3040
Summary: Currently, MySQL/MySQLi connections store passwords in plain text on the object. Allow them to be stored in PhutilOpaqueEnvelopes instead. See D3053.
Test Plan: Loaded site.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3054
Summary: We pull "retries" and a doc link from PhabricatorEnv directly. Break these dependencies so the classes can move to libphutil.
Test Plan: Browsed site, triggered a schema exception and verified I still got the useful footer text.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1283
Differential Revision: https://secure.phabricator.com/D3053
Summary: Add an explicit close() method to connections and call it in GlobalLock.
Test Plan:
Wrote a script like this:
$lock = PhabricatorGlobalLock::newLock('test');
echo "LOCK";
$lock->lock();
sleep(10);
echo "UNLOCK";
$lock->unlock();
sleep(9999);
Using `SHOW FULL PROCESSLIST`, verified the connection closed after 10 seconds with both the "MySQL" and "MySQLi" implementations.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1470
Differential Revision: https://secure.phabricator.com/D3035
Summary: created a PhabricatorInlineCommentPreviewController so controllers in Diffusion and Differential respectively just have to handle the URI mapping and data loading like good little controllers.
Test Plan:
left inline comments on commits, deleted inline commits, submitted inline comments -- all worked well
did the same on some diffs
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1176
Differential Revision: https://secure.phabricator.com/D3034
Summary:
- Add edges for this relationship.
- Use edges to store this data.
- Migrate old data.
- Fix some warnings with generating feed stories about Aux and Edge transactions.
- Fix a task-task edge issue with "Create Subtask".
Test Plan:
- Migrated data, verified reivsions showed up.
- Attached and detached tasks to revisions and vice versa.
- Created a new revision with attached tasks.
- Created a subtask.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3018
Summary: Theses are sort of silly anyway since they should all have the actor in them rather than being sentence fragments, but make them work OK for English at least. See D3013.
Test Plan:
Ran:
echo pht('added %d dependencie(s): %s', 1, 'derp')."\n";
echo pht('added %d dependencie(s): %s', 2, 'derp, derp')."\n";
Got:
added dependency: derp
added dependencies: derp, derp
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3015
Summary:
blogs are collections of posts. a blog also has metadata like a name, description and "bloggers" that can edit the metadata of the blog and contribute posts.
changes include the post edit flow where bloggers can now select which blogs to publish to. also made various small tweaks throughout the UI to make things sensical and clean as the concept of blogs is introduced.
there's edges powering this stuff. bloggers <=> blogs and posts <=> blogs in particular.
Test Plan:
made blogs, deleted blogs, tried to make blogs with no bloggers. all went well.
verified ui to publish only showed up for public posts, published posts to blogs, un-published posts to blogs, re-published posts to blogs, deleted posts and verified they disappeared from blogs.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3003
Summary: See D3006. Move this data to the edge store.
Test Plan:
- Created dependencies, migrated, verified dependencies were preserved.
- Added new dependencies, they worked.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D3007
Summary:
- Use edges to store "X depends on Y" information in Maniphest.
- Show both "Depends On" and "Dependent Tasks".
- Migrate all the old edges.
Test Plan:
- Added some relationships, migrated, verified they were preserved.
- Added some new valid relationships, verified tasks got updated with sensible transactions and sent reasonable emails.
- Tried to add a cycle, got an ugly but effective error.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D3006
Summary: This should simplify a bunch of stuff in D3006 and D3003.
Test Plan: Will update D3006.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3010
Summary: See D3006, D3007. Make it easier to do migrations like that without holding all results in memory.
Test Plan:
Ran this code with an artificially small page size (2):
foreach (new LiskMigrationIterator(new DifferentialRevision()) as $rev) {
echo "Revision ".$rev->getID()."\n";
}
Verified each revision as loaded and processed.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D3008
'phd status' should have a stable result when invoked multiple times.
Automatically removing PID files for dead daemons every time 'phd status' is
invoked prevents tools from noticing that a daemon has died if something
happens to invoke 'phd status' before the tool looks. This affects Puppet
noticably, since it probably runs the status command every half hour.
'phd status' may be invoked by tools (such as puppet) which need to make
automated decisions about whether to start/restart the daemon. To enable this,
'phd status' now exits with 0 if all daemons are running, 1 if no daemons are
running, and 2 if some (but not all) daemons are running.
Summary:
Lisk currently behaves in two different ways if you call it like `load("cow")` (throws) versus `load(99999999)` (returns null), where neither ID exists.
This was intended to catch programming errors as distinct from missing data, but in practice the former is very rare and you have to handle the latter in most cases anyway. The case where you pass "0" is particularly confusing. See D2971 for an example.
On the balance, I think this ends up being far more confusing than helpful. Instead, just return NULL if we're sure there's no such object.
Test Plan: Reasoned about program behavior.
Reviewers: alanh, btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2977
Summary: http://www.networksorcery.com/enp/protocol/irc.htm
Test Plan: augment code with an additional debug line (phlog('hi');) so I can see my case was trigged and it will fall through. setup an ill-configured IRC server with ngircd. Configure an ircbot to connect to said ill-configured IRC server. verify ircbot connected to channel. verify in irc bot logs that debug line was invoked.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1452
Differential Revision: https://secure.phabricator.com/D2962
Summary: Allow the GC daemon to collect the new markup cache.
Test Plan: Ran gc daemon in "debug" mode, saw it collect cache entries.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2947
Summary:
- See D2945.
- Drop `cache` field from ManiphestTransaction.
- Render task descriptions and transactions through PhabricatorMarkupEngine.
- Also pull the list of macros more lazily.
Test Plan:
- Verified transactions and transaction preview work correctly and interact with cache correctly.
- Verified tasks descriptions and task preview work correctly.
- Verified we don't hit the imagemacro table when we're rendering everything from cache anymore.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2946
Summary:
The immediate issue this addresses is T1366, adding a rendering cache to Phriction. For wiki pages with code blocks especially, rerendering them each time is expensive.
The broader issue is that out markup caches aren't very good right now. They have three major problems:
**Problem 1: the data is stored in the wrong place.** We currently store remarkup caches on objects. This means we're always loading it and passing it around even when we don't need it, can't genericize cache management code (e.g., have one simple script to drop/GC caches), need to update authoritative rows to clear caches, and can't genericize rendering code since each object is different.
To solve this, I created a dedicated cache database that I plan to move all markup caches to use.
**Problem 2: time-variant rules break when cached.** Some rules like `**bold**` are time-invariant and always produce the same output, but some rules like `{Tnnn}` and `@username` are variant and may render differently (because a task was closed or a user is on vacation). Currently, we cache the raw output, so these time-variant rules get locked at whatever values they had when they were first rendered. This is the main reason Phriction doesn't have a cache right now -- I wanted `{Tnnn}` rules to reflect open/closed tasks.
To solve this, I split markup into a "preprocessing" phase (which does all the parsing and evaluates all time-invariant rules) and a "postprocessing" phase (which evaluates time-variant rules only). The preprocessing phase is most of the expense (and, notably, includes syntax highlighting) so this is nearly as good as caching the final output. I did most of the work here in D737 / D738, but we never moved to use it in Phabricator -- we currently just do the two operations serially in all cases.
This diff splits them apart and caches the output of preprocessing only, so we benefit from caching but also get accurate time-variant rendering.
**Problem 3: cache access isn't batched/pipelined optimally.** When we're rendering a list of markup blocks, we should be able to batch datafetching better than we do. D738 helped with this (fetching is batched within a single hunk of markup) and this improves batching on cache access. We could still do better here, but this is at least a step forward.
Also fixes a bug with generating a link in the Phriction history interface ($uri gets clobbered).
I'm using PHP serialization instead of JSON serialization because Remarkup does some stuff with non-ascii characters that might not survive JSON.
Test Plan:
- Created a Phriction document and verified that previews don't go to cache (no rows appear in the cache table).
- Verified that published documents come out of cache.
- Verified that caches generate/regenerate correctly, time-variant rules render properly and old documents hit the right caches.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1366
Differential Revision: https://secure.phabricator.com/D2945
Summary:
Certain types of things we should be storing in edges (notably, Task X depends on Task Y) should always be acyclic. Allow `PhabricatorEdgeEditor` to enforce this, since we can't correctly enforce it outside of the editor without being vulnerable to races.
Each edge type can be marked acyclic. If an edge type is acyclic, we perform additional steps when writing new edges of that type:
- We acquire a global lock on the edge type before performing any reads or writes. This ensures we can't produce a cycle as a result of a race where two edits add edges which independently do not produce a cycle, but do produce a cycle when combined.
- After performing writes but before committing transactions, we load the edge graph for each acyclic type and verify that it is, in fact, acyclic. If we detect cycles, we abort the edit.
- When we're done, we release the edge type locks.
This is a relatively high-complexity change, but gives us a simple way to flag an edge type as acyclic in a robust way.
Test Plan: Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D2940
Summary:
Currently, multiple unit tests that acquire global locks will interfere with each other. Namespace the locks so they don't.
(Possibly we should also rename this to PhabricatorStorageNamespaceLock or something since it's not really global any more, but that's kind of unwieldy...)
Test Plan: Acquired locks with --trace and verified they were namespaced properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D2939
Summary:
In unit tests which use fixtures, we open transactions on every connection we establish. However, since we don't track connections that are established with "$force_new" (currently, only GlobalLock connections) we never close these transactions normally.
Instead of not tracking these connections, track them using unique keys so we'll never get a cache hit on them.
Test Plan: Built unit tests on top of this, had them stop dying from unclosed transactions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D2938
Summary:
A later diff adds unit tests against edges, but we need real objects to connect with edges. Add some trivial objects to the Harbormaster database to compliment the similar HarbormasterScratchTable.
On its own, this does nothing interesting.
Test Plan: Built unit tests on this in a followup.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D2937
Summary:
See discussion here:
https://secure.phabricator.com/chatlog/channel/%23phabricator/?at=21186
Basically, MySQL usually raises a good error if we exceed "max_allowed_packet":
EXCEPTION: (AphrontQueryException) #1153: Got a packet bigger than 'max_allowed_packet' bytes
But sometimes it gives us a #2006 instead. This is documented, at least:
>"With some clients, you may also get a Lost connection to MySQL server during query error if the communication packet is too large."
http://dev.mysql.com/doc/refman//5.5/en/packet-too-large.html
Try to improve the error message to point at this as a possible explanation.
Test Plan: Faked an error, had it throw, read exception message. See also chatlog.
Reviewers: btrahan, skrul
Reviewed By: skrul
CC: aran
Differential Revision: https://secure.phabricator.com/D2923
Summary: See D2924.
Test Plan: Ran locks with blocking timeouts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2925
Summary: See D2906. This just adds text so they render pretty.
Test Plan:
Got pretty emails and rendered transactions.
{F13706}
Reviewers: btrahan, davidreuss
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2907
Summary:
- See D2741.
- When EdgeEditor performs edits, emit events.
- Listen for Maniphest edge events and save the changes as transactions.
- Do all this in a reasonably generic way that won't take too much rewriting as we use edges more generally.
Test Plan: Attached and detached commits from tasks, saw reasonable-looking transactions spring into existence.
Reviewers: btrahan, davidreuss
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2906
Summary: Simplify FeedQuery by making it extend from PhabricatorIDPagedPolicyQuery
Test Plan: Looked at feed on home, projects, user profile, and called `feed.query`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2905
Summary: Implementation is a little crazy but this seems to work as advertised.
Test Plan: Acquired locks with "lock.php". Verified they held as long as the process reamined open and released properly on kill -9, ^C, etc.
Reviewers: nh, jungejason, vrana, btrahan, Girish, edward
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1400
Differential Revision: https://secure.phabricator.com/D2864
Summary: Related to D2873.
Test Plan: Specified it and verified that highlighting still works.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2874
Summary:
Support the `--verbose` flag added in D2795 in `phd`. See T1389.
Also simplify argument generation a little bit.
Test Plan: Ran "nice" daemon with debug, daemon + verbose, daemon + no verbose.
Reviewers: vrana, jungejason, edward, aurelijus
Reviewed By: aurelijus
CC: aran
Maniphest Tasks: T1389
Differential Revision: https://secure.phabricator.com/D2797
Summary: I tried also filling the column by empty space but this looks better.
Test Plan: Displayed a comment both with visible and hidden inlines.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1216
Differential Revision: https://secure.phabricator.com/D2789
Summary: It requires `allow_url_fopen` which we don't check in setup and our installation is about to disable it.
Test Plan:
Login with OAuth.
/oauth/facebook/diagnose/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2787
Test Plan:
Altered database.
Wrote a custom translation and selected it in preferences.
Verified that the text is custom translated.
Set language back to default.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D2757
Summary:
This is the first step in Phabricator internationalization.
It adds a translation selector and calls it at startup.
Installations can add custom selectors to override some texts.
We can add official translations in future.
Next step is to allow user to choose his translation which will override the global one.
This is currently used only for English plurals.
Test Plan: Displayed a diff with unit test error, verified that it says 'Detail' or 'Details' and not 'Detail(s)'.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D2753
Summary:
Some e-mail clients display this header and it needs to be constant.
This is somehow involved but I doubt that there is a simpler solution.
Test Plan:
Applied SQL patch.
Commented on revision, commented on commit, changed package.
Verified that the `Thread-Topic` has constant and human readable value.
Reviewers: epriestley
Reviewed By: epriestley
CC: ola, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2745
Summary: This is //extremely// basic but dead simple and should cover us for v1, I think. Let me know what features you need.
Test Plan: Used UI example page.
Reviewers: allenjohnashton, ddfisher, keebuhm
Reviewed By: ddfisher
CC: aran, ender
Maniphest Tasks: T944
Differential Revision: https://secure.phabricator.com/D2732
Summary: Made it possible to link and unlink LDAP accounts with Phabricator accounts.
Test Plan:
I've tested this code locally and in production where I work.
I've tried creating an account from scratch by logging in with LDAP and linking and unlinking an LDAP account with an existing account. I've tried to associate the same LDAP account with different Phabricator accounts and it failed as expected.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, auduny, svemir
Maniphest Tasks: T742
Differential Revision: https://secure.phabricator.com/D2722
Summary:
HPHP doesn't like resolved symlinks.
Also I like this code better.
Test Plan: Used and not used custom Celerity map.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2694
Summary: Added `PhabricatorBuiltinPatchList` entry so that "storage upgrade" will update the database. Renamed and numbered the notification.sql patch.
Test Plan: Drop phabricator_feed.feed_storynotification table if it exists and run bin/storage upgrade to check if the patch is correctly applied.
Reviewers: epriestley, btrahan, allenjohnashton
Reviewed By: epriestley
CC: ddfisher, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2687
Summary:
- The config is called "resource-path" and the script references "resource-path", but the actual value checked for is "resource-map".
- Use nonempty(), since defaulting with getEnvConfig() will give you null if the setting exists but is set to null. This default is nearly useless so maybe we should change it to use coalesce().
- Remove Celerity map initialization from warmup. We don't currently initialize the environment in warmup, and Celerity initialization now depends on the environment.
Test Plan: Ran patch locally and on FPM-Warmup.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: hsb, aran
Differential Revision: https://secure.phabricator.com/D2662
Summary:
See T1303, which presents a reasonable case for inclusion of these characters in valid usernames.
Also, unify username validity handling.
Test Plan: Created a new user with a valid name. Tried to create a new user with an invalid name. Ran unit tests.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1303
Differential Revision: https://secure.phabricator.com/D2651
Summary: replace differential.find with differential.query and display in requested order
Test Plan: say D444D222D443D442 and ensure they are in the correct order
Reviewers: epriestley
Reviewed By: epriestley
CC: Mnkras, aran
Differential Revision: https://secure.phabricator.com/D2656
Summary:
We have custom static resources.
We currently include them in Phabricator's celerity resource map which is causing some pain - we need to regenerate the file without our custom resources before pushing upstream, we need to discard our changes before pulling from upstream and we need to rebuild with our changes to run Phabricator.
This diff allows writing and reading the map in other location.
The plan is this - I will run `celerity_mapper.php` twice - once to build Phabricator-only resources (to push to upstream) and once to build Phabricator + ours resoruces to put in our directory.
Better solution would be to create a map just with our resources and read and combine it with Phabricator resources.
But it is complicated because we have dependencies on Phabricator resources.
Test Plan:
`celerity_mapper.php webroot`
`celerity_mapper.php webroot ../facebook/src/__celerity_resource_map__.php`
Delete Phabricator's celerity map, set 'celerity.resource-path' and successfully load Phabricator.
Reviewers: epriestley, edward
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T721
Differential Revision: https://secure.phabricator.com/D2630
Summary:
- `kill_init.php` said "Moving 1000 files" - I hope that this is not some limit in `FileFinder`.
- [src/infrastructure/celerity] `git mv utils.php map.php; git mv api/utils.php api.php`
- Comment `phutil_libraries` in `.arcconfig` and run `arc liberate`.
NOTE: `arc diff` timed out so I'm pushing it without review.
Test Plan:
/D1234
Browsed around, especially in `applications/repository/worker/commitchangeparser` and `applications/` in general.
Auditors: epriestley
Maniphest Tasks: T1103
Summary:
Unittest databases are not always destroyed in our setup.
It could be caused by `__destruct()` not called in case of a fatal error.
Test Plan:
arc unit src/applications/calendar/storage/holiday
Reviewers: edward, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2568
Summary:
- We currently write every PHID we generate to a table. This was motivated by two concerns:
- **Understanding Data**: At Facebook, the data was sometimes kind of a mess. You could look at a random user in the ID tool and see 9000 assocs with random binary data attached to them, pointing at a zillion other objects with no idea how any of it got there. I originally created this table to have a canonical source of truth about PHID basics, at least. In practice, our data model has been really tidy and consistent, and we don't use any of the auxiliary data in this table (or even write it). The handle abstraction is powerful and covers essentially all of the useful data in the app, and we have human-readable types in the keys. So I don't think we have a real need here, and this table isn't serving it if we do.
- **Uniqueness**: With a unique key, we can be sure they're unique, even if we get astronomically unlucky and get a collision. But every table we use them in has a unique key anyway. So we actually get pretty much nothing here, except maybe some vague guarantee that we won't reallocate a key later if the original object is deleted. But it's hard to imagine any install will ever have a collision, given that the key space is 36^20 per object type.
- We also currently use PHIDs and Users in tests sometimes. This is silly and can break (see D2461).
- Drop the PHID database.
- Introduce a "Harbormaster" database (the eventual CI tool, after Drydock).
- Add a scratch table to the Harbormaster database for doing unit test meta-tests.
- Now, PHID generation does no writes, and unit tests are isolated from the application.
- @csilvers: This should slightly improve the performance of the large query-bound tail in D2457.
Test Plan: Ran unit tests. Ran storage upgrade.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: csilvers, aran, nh, edward
Differential Revision: https://secure.phabricator.com/D2466
Summary: D2490 was not my finest hour and I incorrectly thought it was a null value from error. In reality this error is impossible and its just a valid empty array so instead use the empty predicate to bail.
Test Plan: with our logic combined, this be tested
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2503
Summary: 'cuz github issue 114 came into existence. instead, just return false early here. note i am not sure if I should phlog that this is happening or not but its not exception worthy IMO.
Test Plan: lint-only 'cuz i don't want to setup an IRC server locally / somehow get my local phabricator instance accessible out there. happy to test end to end if there's an easier way...!
Reviewers: vrana, epriestley
Reviewed By: vrana
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2490
Summary:
D2470 added Package mailhandler, which was configured incorrectly in the getRequiredClasses function. this makes it like the other mail handlers
Reported at https://github.com/facebook/phabricator/issues/112
Test Plan: setup mode no longer fails
Reviewers: epriestley, jungejason, royklopper
Reviewed By: royklopper
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2476
Summary:
For package creation and deletion, send email to all the owners For
package modification, detect important fields such as owners and paths, and then
send out emails to all owners (including deleted owners and current owners)
Also start using transaction for package creation/deletion/modification.
Test Plan:
- tested mail creation and deletion
- tested modification to auditing enabled, primary owners, owners, paths
Reviewers: epriestley, nh, vrana
Reviewed By: epriestley
CC: prithvi, aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2470
Summary:
Required in order to run tests successfully in the HipHop interpreter.
Similar to D2362.
Test Plan: Run the tests in an HipHop runtime.
Reviewers: epriestley
Reviewed By: epriestley
CC: jungejason, aran, Koolvin, vrana
Differential Revision: https://secure.phabricator.com/D2365
Summary:
- Merge CommitTask daemon into PullLocal daemon. This is another artifact of past instability (and order-dependent parsers). We still publish to the timeline, although this was the last consumer. Long term we'll probably delete timeline and move to webhooks, since everyone who has asked about this stuff has been eager to trade away the durability and ordering of the timeline for the ease of use of webhooks. There's also no reason to timeline this anymore since parsing is no longer order-dependent.
- Add `phd start` to start all the daemons you need. Add `phd restart` to restart all the daemons you need. So cool~
- Simplify and improve phd and Diffusion daemon documentation.
Test Plan:
- Ran `phd start`.
- Ran `phd restart`.
- Generated/read documentation.
- Imported some stuff, got clean parses.
Reviewers: btrahan, csilvers
Reviewed By: csilvers
CC: aran, jungejason, nh
Differential Revision: https://secure.phabricator.com/D2433
Summary:
- Move email to a separate table.
- Migrate existing email to new storage.
- Allow users to add and remove email addresses.
- Allow users to verify email addresses.
- Allow users to change their primary email address.
- Convert all the registration/reset/login code to understand these changes.
- There are a few security considerations here but I think I've addressed them. Principally, it is important to never let a user acquire a verified email address they don't actually own. We ensure this by tightening the scoping of token generation rules to be (user, email) specific.
- This should have essentially zero impact on Facebook, but may require some minor changes in the registration code -- I don't exactly remember how it is set up.
Not included here (next steps):
- Allow configuration to restrict email to certain domains.
- Allow configuration to require validated email.
Test Plan:
This is a fairly extensive, difficult-to-test change.
- From "Email Addresses" interface:
- Added new email (verified email verifications sent).
- Changed primary email (verified old/new notificactions sent).
- Resent verification emails (verified they sent).
- Removed email.
- Tried to add already-owned email.
- Created new users with "accountadmin". Edited existing users with "accountadmin".
- Created new users with "add_user.php".
- Created new users with web interface.
- Clicked welcome email link, verified it verified email.
- Reset password.
- Linked/unlinked oauth accounts.
- Logged in with oauth account.
- Logged in with email.
- Registered with Oauth account.
- Tried to register with OAuth account with duplicate email.
- Verified errors for email verification with bad tokens, etc.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2393
Summary:
I will use it for highlighting users which are not currently available.
Maybe I will also use it in the nagging tool.
I don't plan creating a UI for it as API is currently enough for us.
Maybe I will visualize it at /calendar/ later.
I plan creating `user.deletestatus` method when this one will be done.
Test Plan:
`storage upgrade`
Call Conduit `user.addstatus`.
Verify DB.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2382
Summary:
We will need it for two purposes:
- Status tool.
- Nagging tool - @aran suggested using "3 business days" and I don't want it to fall on New Year's Eve or such.
I don't plan working on any interface for editing this as this kind of data should be always imported.
Test Plan:
`bin/storage upgrade`
`scripts/calendar/import_us_holidays.php`
/calendar/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2375
Summary:
- Unit tests can request storage fixtures.
- We build one fixture across all tests in the process, which can quickstart (takes roughly 1s to build, 200ms to destroy for me). This is a one-time cost for running an arbitrary number of fixture-based tests.
- We isolate all the connections inside transactions for each test, so individual tests don't affect one another.
Test Plan: Ran unit tests, which cover the important properties of fixtures.
Reviewers: btrahan, vrana, jungejason, edward
Reviewed By: btrahan
CC: aran, davidreuss
Maniphest Tasks: T140
Differential Revision: https://secure.phabricator.com/D2345
Summary: Allow the default namespace to be set in configuration, so you can juggle multiple copies of sandbox test data or whatever.
Test Plan: Changed default namespace, verified web UI and "storage" script respect it.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T345
Differential Revision: https://secure.phabricator.com/D2341
Summary:
This addresses three issues with the current patch management system:
# Two people developing at the same time often pick the same SQL patch number, and then have to go rename it. The system catches this, but it's silly.
# Second/third-party developers can't use the same system to manage auxiliary storage they may want to add.
# There's no way to build mock databases for unit tests that need to do reads.
To resolve these things, you can now name your patches whatever you want and conflicts are just merge conflicts, which are less of a pain to fix than filename conflicts.
Dependencies are now a DAG, with implicit dependencies created on the prior patch if no dependencies are specified. Developers can add new concrete subclasses of `PhabricatorSQLPatchList` to add storage management, and define the dependency branchpoint of their patches so they apply in the correct order (although, generally, they should not depend on the mainline patches, presumably).
The commands `storage upgrade --namespace test1234` and `storage destroy --namespace test1234` will allow unit tests to build and destroy MySQL storage.
A "quickstart" mode allows an upgrade from scratch in ~1200ms. Destruction takes about 200ms. These seem like fairily reasonable costs to actually use in tests. Building from scratch patch-by-patch takes about 6000ms.
Test Plan:
- Created new databases from scratch with and without quickstart in a separate test namespace. Pointed the webapp at the test namespaces, browsed around, everything looked good.
- Compared quickstart and no-quickstart dump states, they're identical except for mysqldump timestamps and a few similar things.
- Upgraded a legacy database to the new storage format.
- Destroyed / dumped storage.
Reviewers: edward, vrana, btrahan, jungejason
Reviewed By: btrahan
CC: aran, nh
Maniphest Tasks: T140, T345
Differential Revision: https://secure.phabricator.com/D2323
Summary: "Committed" is SVN-specific language, and confusing in Git and Mercurial. Use neutral language instead.
Test Plan: Inspection.
Reviewers: btrahan, Makinde, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T909
Differential Revision: https://secure.phabricator.com/D2087
Summary:
Couple of small improvements:
- Delete `randomon` macro.
- Make name unique (deleting current conflicts randomly).
- Image macro must be alone on the line.
- Filter by name.
Test Plan:
Run SQL.
/file/macro/
/file/macro/?name=imagemacro
Try to create conflicting name.
Write this comment:
Test imagemacro.
imagemacro
Reviewers: aran, epriestley
Reviewed By: epriestley
CC: epriestley, Koolvin
Differential Revision: https://secure.phabricator.com/D2230
Summary:
This event is fired after a task is created and assigned with an id.
Use case is sending an email notification to everyone in a project when a new task is
submitted to said project.
Test Plan:
Implement the event listener, submit a new task to a project, see if the project members
receive an email notification. I will submit the event handler in a separate diff once it's a bit
prettier and tested more thoroughly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D2159
Summary: Introduces a scope-guarded way to override the env config, for unit tests which are sensitive to config values.
Test Plan: Ran unit tests.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2237
Summary:
Provides a basic start for access policies. Objects expose various capabilities, like CAN_VIEW, CAN_EDIT, etc., and set a policy for each capability. We currently implement three policies, PUBLIC (anyone, including logged-out), USERS (any logged-in) and NOONE (nobody). There's also a way to provide automatic capability grants (e.g., the owner of an object can always see it, even if some capability is set to "NOONE"), but I'm not sure how great the implementation feels and it might change.
Most of the code here is providing a primitive for efficient policy-aware list queries. The problem with doing queries naively is that you have to do crazy amounts of filtering, e.g. to show the user page 6, you need to filter at least 600 objects (and likely more) before you can figure out which ones are 500-600 for them. You can't just do "LIMIT 500, 100" because that might have only 50 results, or no results. Instead, the query looks like "WHERE id > last_visible_id", and then we fetch additional pages as necessary to satisfy the request.
The general idea is that we move all data access to Query classes and have them do object filtering. The ID paging primitive allows efficient paging in most cases, and the executeOne() method provides a concise way to do policy checks for edit/view screens.
We'll probably end up with mostly broader policy UIs or configuration-based policies, but there are at least a few cases for per-object privacy (e.g., marking tasks as "Security", and restricting things to the members of projects) so I figured we'd start with a flexible primitive and the simplify it in the UI where we can.
Test Plan: Unit tests, played around in the UI with various policy settings.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D2210
Summary: See T993. MySQL fails very very softly if you request an engine which does not exist. Detect and fail if the InnoDB engine is missing or broken.
Test Plan: Faked InnoDB missing, got a failure. Ran normally, got success.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T993
Differential Revision: https://secure.phabricator.com/D2160
Summary:
We don't use versioned URIs for images, so when they change users may get old versions.
This was a particular issue with the recent logo change, which several users reported cache-related issues from.
Instead, use Celerity to manage image URI versions in addition to CSS/JS.
This is complicated, because we need to rewrite image URIs inside of CSS, which means the hash of a CSS file has to be derived from the current image data. Otherwise, when we updated an image the CSS wouldn't update, so we wouldn't be any better off.
So basically we:
- Find all the "raw" files, and put them into the map.
- Find all the CSS/JS, perform content-altering transformations on it (i.e., not minification) based on the partial map, and then put it into the map based on transformed hashes.
(If we wanted, we could now do CSS variables or whatever for "free", more or less.)
Test Plan:
- Regenerated celerity map, browsed site, verified images generated with versioned URIs.
- Moved "blue" flag image over "green" flag image, regenerated map, verified "green" flag image and the associated CSS changed hashes.
- Added transformation unit tests; ran unit tests.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1073
Differential Revision: https://secure.phabricator.com/D2146
Summary: This separates common MySQL stuff (identifiers and comments escaping, error codes, connection retries) from PHP extension specific stuff (connect, query, fetch, errors, escape string).
Test Plan:
/
Use `AphrontMySQLiDatabaseConnection` in `PhabricatorLiskDAO`, load homepage, edit task, save task.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran
Differential Revision: https://secure.phabricator.com/D2113
Summary: Use Edges to attach Commits and Tasks. Note, no "edit attached commits" interface from tasks yet since the search backend needs a little work to list commits in a sensible way.
Test Plan: Attached commits to tasks. Looked at commits, looked at tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D2105
Summary:
We have a lot of cases where we store object relationships, but it's all kind of messy and custom. Some particular problems:
- We go to great lengths to enforce order stability in Differential revisions, but the implementation is complex and inelegant.
- Some relationships are stored on-object, so we can't pull the inverses easily. For example, Maniphest shows child tasks but not parent tasks.
- I want to add more of these and don't want to continue building custom stuff.
- UIs like the "attach stuff to other stuff" UI need custom branches for each object type.
- Stuff like "allow commits to close tasks" is notrivial because of nonstandard metadata storage.
Provide an association-like "edge" framework to fix these problems. This is nearly identical to associations, with a few differences:
- I put edge metadata in a separate table and don't load it by default, to keep edge rows small and allow large metadata if necessary. The on-edge metadata seemed to get abused a lot at Facebook.
- I put a 'seq' column on the edges to ensure they have an explicit, stable ordering within a source and type.
This isn't actually used anywhere yet, but my first target is attaching commits to tasks for T904.
Test Plan: Made a mock page that used Editor and Query. Verified adding and removing edges, overwriting edges, writing and loading edge data, sequence number generation.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, 20after4
Differential Revision: https://secure.phabricator.com/D2088
Summary:
Most setters returns `$this` but some don't.
I guess it's not by purpose.
Test Plan:
arc lint
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2085
Summary:
These are the issues identified by the linter in D2052. I don't think any cause bugs, but they are all reasonable errors to raise and the linter correctly
detected that they are suspicious.
Test Plan: Mostly inspection.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2053
Summary: For production servers, minify CSS and JS by stripping comments, whitespace, etc.
Test Plan: Looked at CSS/JS, it was much smaller.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T264
Differential Revision: https://secure.phabricator.com/D2034
Summary:
I'd like to use this regex elsewhere and copying and pasting is
bad.
Test Plan: none
Reviewers: casey, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2038
Summary:
- Still really really rough.
- Adds a full synchronous mode for debugging.
- Adds some logging.
- It can now allocate EC2 machines and put webroots on them in a hacky, terrible way.
- Adds a base query class.
Test Plan: oh hey look a test page? http://ec2-50-18-65-151.us-west-1.compute.amazonaws.com:2011/
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D2026
Summary: D2023 adds a new '*' token to javelinsymbols (indicating that a behavior is 'installed'). This fixes a sanity-check regex in PhabricatorJavelinLinter that validates the output of javelinsymbols so that it is aware of this new token type.
Test Plan:
Patched javelinsymbols.cpp from D2023 to externals/javelin/support/javelinsymbols, build the new javelinsymbols binary, then ran
arc lint --lintall webroot/rsrc/js/application/core/behavior-drag-and-drop-textarea.js
(before this diff, that throws an error -- after it works with no lint)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2025
Summary:
We render a huge picture of a PDF for PDFs right now, etc. This is hella dumb.
Also allow users to force this rendering style, and change the link name.
Test Plan: Uploaded image and non-image files, used layout=link and name=....
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1040
Differential Revision: https://secure.phabricator.com/D2006
Test Plan:
Run setup with 'differential.attach-task-class' set to:
- ''
- 'FacebookTasksAttacher'
- 'X'
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1999
Summary: I'll mark this one up inline since it's all separate bugs.
Test Plan:
- Created a diff with eight changes: (newline absent -> newline present, newline present -> newline absent, newline present -> newline present, newline absent -> newline absent) x (short file with change near end, long file with change near middle).
- Viewed diff in Ignore All, Ignore Most, Ignore Trailing and Show All whitespace modes.
- All 32 results seemed sensible.
- Really wish this stuff was better factored and testable. Need to fix it. :(
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1030
Differential Revision: https://secure.phabricator.com/D1992
Summary:
It is now possible to set config setting requiring class of certain implementation to something completely else.
The consequence is that your Phabricator may stop working after update because you didn't implement some new method.
This diff validates the class upon usage.
It throws exception which is better than fatal thrown currently after calling undefined method.
Better solution would be to validate classes when setting the config but it would be too expensive - respective class definitions would have to be loaded and checked by reflection.
I was also thinking about some check script but nobody would run it after changing config.
The same behavior should be implemented for these settings:
- metamta.mail-adapter
- metamta.maniphest.reply-handler
- metamta.differential.reply-handler
- metamta.diffusion.reply-handler
- storage.engine-selector
- search.engine-selector
- differential.field-selector
- maniphest.custom-task-extensions-class
- aphront.default-application-configuration-class
- controller.oauth-registration
Test Plan:
Send comment, verify that it pass.
Change `metamta.differential.reply-handler` to incompatible class, verify that sending comment shows nice red exception.
Set `metamta.differential.reply-handler` to empty string, verify that it throws.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1919
Summary:
- Affects the "Inline Comments" summary table which appears in comments that have attached inlines in the discussion threads in Differential.
- Prepares for inclusion in Diffusion.
- No application changes (minor CSS), just factors code better.
- Simplify/separate CSS.
Test Plan: Looked at on-diff and off-diff comment summaries in Differential, display looked correct.
Reviewers: davidreuss, nh, btrahan
Reviewed By: davidreuss
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1928
Summary:
It is currently not possible to select source code covered by reticle when creating comment.
This diff hides reticle on mouseout from reply area.
Test Plan:
Hover inline comment, verify that reticle is displayed.
Reply, verify that reticle is displayed when mouseover reply, hidden otherwise.
Repeat for create.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1851
Summary:
- Add inline comments to Audits, like Differential.
- Creates new storage for the comments in the Audits database.
- Creates a new PhabricatorAuditInlineComment class, similar to DifferentialInlineComment.
- Defines an Interface which Differential and Audit comments conform to.
- Makes consumers of DifferentialInlineComments consume objects which implement that interface instead.
- Adds save
NOTE: Some features are still missing! Wanted to cut this off before it got crazy:
- Inline comments aren't shown in the main comment list.
- Inline comments aren't shown in the emails.
- Inline comments aren't previewed.
I'll followup with those but this was getting pretty big.
@vrana, does the SQL change look correct?
Test Plan:
- Created, edited, deleted, replied to, reloaded and saved inline comments in Diffusion, on the left and right side of diffs.
- Created, edited, deleted, replied to, reloaded and saved inline comments in Differentila, on the left and right side of primary and diff-versus-diff diffs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1898
Summary:
1. The setup flow complains if you haven't updated your schema, so that section
should be moved above the setup flow.
2. The setup flow tells you to lower your timeout, but it doesn't tell you how
low will make it stop complaining.
Test Plan:
Didn't test the setup.
Regenerated the docs and saw the change.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1888
Summary: Last of the big final patches. Left a few debatable classes (12 out of about 400) that I'll deal with individually eventually.
Test Plan: Ran testEverythingImplemented.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1881
Summary:
These are all unambiguously unextensible. Issues I hit:
- Maniphest Change/Diff controllers, just consolidated them.
- Some search controllers incorrectly extend from "Search" but should extend from "SearchBase". This has no runtime effects.
- D1836 introduced a closure, which we don't handle correctly (somewhat on purpose; we target PHP 5.2). See T962.
Test Plan: Ran "testEverythingImplemented" unit test to identify classes extending from `final` classes. Resolved issues.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1843
Summary:
Adds a macro handler that spams your channel with macros. Config is:
- macro.size: scale macros to this size before rasterizing
- macro.sleep: sleep this many seconds between lines (evade flood protection)
Test Plan: derpderp
Reviewers: kdeggelman, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1838
Summary:
This adds a new configuration setting:
"notification.actions" : [
"commit",
"abandon",
"actions"
]
if not set, displays all actions, if is set, display only what is set to display
Test Plan: add the notification.actions settings and set accordingly
Reviewers: epriestley, zeeg
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1820
Summary:
A user reported that their install (on an unusual piece of hardware) was hitting this timeout. We don't need to be quite so stingy; just use the default 30s timeout.
Also remove some kind of sentence fragment since I no longer remember what it meant and it doesn't make sense.
Test Plan: This change resolved the issue.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1805
Summary: The docs say "http://www.domain.com/" but if you don't put "/api/" it fails. GOTCHA!
Test Plan: Removed "/api/", launched bot, it worked.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T935
Differential Revision: https://secure.phabricator.com/D1763
