Summary:
For a single line, I can use the right click.
But for highlighting multiple lines, I have to wait for page reload.
It would be nice to track this in history but that's more involved.
This is actually maybe better behavior.
Test Plan: Clicked on the line link, dragged and dropped on it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5172
Summary:
Fixes T2059. Ref T2517.
Currently, you can run `bin/storage upgrade` with `--user` and `--password` arguments. However, these clownishly apply only to `.sql` patches -- the `.php` migrations still use the default user and password.
This is dumb. Stop doing it. Respect `--user` and `--password` for PHP patches.
(I implemented "override", which is very similar to "repair", but kept them separate since I think they're semantically distinct enough to differentiate.)
Test Plan: Ran `./bin/storage upgrade --user x --pass y --apply phabricator:20130219.commitsummarymig.php`. Verified the correct user and password were used both for the initial connect and patch application.
Reviewers: chad, vrana
Reviewed By: chad
CC: aran
Maniphest Tasks: T2059, T2517
Differential Revision: https://secure.phabricator.com/D5115
Summary:
Some transactions (like editing configuration values, task descriptions, or Conpherence images) can't be simply explained and need an additional larger element to show them fully (like a text diff).
Support change details like this in ApplicationTransactions. Implements the element in Config, so you can see changes.
Test Plan: {F32974}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2213
Differential Revision: https://secure.phabricator.com/D4984
Summary:
The map had "conph" but everything else refers to "conpher". The "conph" sprite thing won when I regenerated sprites for tokens.
I should just fix this so it can't happen, but unbreak for now. Renamed "conph" -> "conpher", regenerated sprites, nuked all the "conph" stuff.
Test Plan: Looked at Conpherence, saw icons.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4982
Summary: Pare this down to the bare bones. Diviner is going away soon anyway.
Test Plan: Read instructions.
Reviewers: edward
Reviewed By: edward
CC: aran
Differential Revision: https://secure.phabricator.com/D4951
Summary: Convert most phabricator_render_form callsites. In the case of the "headsup view", it converts it by deleting the element entirely (this is the very old Maniphest/Differential header which we no longer use).
Test Plan: Poked around a bit.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4726
Summary:
This accomplishes three major goals:
# Fixes phutil_render_tag -> phutil_tag callsites in DarkConsole.
# Moves the Ajax request log to a new panel on the left. This panel (and the tabs panel) get scrollbars when they get large, instead of making the page constantly scroll down.
# Loads the panel content over ajax, instead of dumping it into the page body / ajax response body. I've been planning to do this for about 3 years, which is why the plugins are architected the way they are. This should make debugging easier by making response bodies not be 50%+ darkconsole stuff.
Additionally, load the plugins dynamically (the old method predates library maps and PhutilSymbolLoader).
Test Plan:
{F30675}
- Switched between requests and tabs, reloaded page, saw same tab.
- Used "analyze queries", "profile page", triggered errors.
- Verified page does not load anything by default if dark console is closed with Charles.
- Generally banged on it a bit.
Reviewers: vrana, btrahan, chad
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4692
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:
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: 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:
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: 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:
See discussion in D4438. Allows users to customize application tiles, and implements generally reasonable defaults so they hopefully won't.
Sizes are "invisible" (internal only, used to hide admin apps from non-admins), "hidden" (hide by default, show after clicking "Show More Applications"), "show" (show a small square tile) and "full" (show a full-width tile with subtitle).
Test Plan:
Default view for a non-admin:
{F29375}
Adjusted settings, hidden:
{F29373}
Adjusted settings, shown:
{F29374}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4439
Summary:
No fancy-pants smarty stuff yet, but merges /applications/ and the awful application buttons into the dark navigation.
Hover state is maybe a little weird.
Test Plan: {F29324}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran, btrahan, codeblock
Differential Revision: https://secure.phabricator.com/D4431
Summary: Move all navs to use the newer-style, darker, textured look. I'm //pretty// sure this doesn't break anything.
Test Plan: Looked at a bunch of apps.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4376
Summary:
This implements most/all of the difficult parts of Diviner on top of Phabricator instead of as standalone components. See T988. In particular, here are the things I want to fix:
**Performance** The Diviner parser works in two stages. The first stage breaks source files into "Atoms". The second stage renders atoms into a display format (e.g., HTML). Diviner currently has a good caching story on the first step of the pipeline, but zero caching in the second step. This means it's very slow, even for a fairly small project like Phabricator. We must re-render every piece of documentation every time, instead of only changed documentation. Most of this diff concerns itself with addressing this problem. There's a fairly large explanatory comment about it, but the trickiest part is that when an atom changes, other atoms (defined in other places) may also change -- for example, if `class B extends A`, editing A should dirty B, even if B is in an entirely different file. We perform analysis in two stages to propagate these changes: first detecting direct changes, then detecting indirect changes. This isn't completely implemented -- we need to propagate 'extends' through more levels -- but I believe it's structurally correct and good enough until we actually document classes.
**Inheritance** Diviner currently has a very weak story on inheritance. I want to inherit a lot more metas/docs. If an interface documents a method, we should just pull that documentation in to every implementation by default (implementations can still override it if they want). It can be shown in grey or something, but it should be desirable and correct to omit documentation of a method implementation when you are implementing a parent. Similarly, I want to pull in inherited methods and @tasks and such. This diff sets up for that, by formalizing "extends" relationships between atoms.
**Overspecialization** Diviner currently specializes atoms (FileAtom, FunctionAtom, ClassAtom, etc.). This is pretty much not useful, because Atomizers (which produce the atoms) need to be highly specialized, and Renderers/Publishers (which consume the atoms) also need to be highly specialized. Nothing interesting actually lives in the atom specializations, and we don't benefit from having them -- it just costs us generality in storage/caches for them. In the new code, I've used a single Atom class to represent any type of atom.
**URIs** We have fairly hideous URIs right now, which are very cumbersome For in-app doc links, I want to provide nice URIs ("/h/notfications" or similar) which are stable redirects, and probably add remarkup for it: !{notifications} or similar. This diff isn't related to that since it's too premature.
**Search** Once we have a database generation target, we can index the documentation.
**Design** Chad has some nice mocks.
Test Plan: Ran `bin/diviner generate`, `bin/diviner generate --clean`. Saw appropriate graph propagation after edits. This diff doesn't do anything very useful yet.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D4340
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: We have enough z-index rules that they're fairly hard to visualize with "git grep". Consolidate them. Then fix T2253 (missing z-index on left menu background).
Test Plan: Made a Differential window really narrow, then scrolled it horizontally.
Reviewers: btrahan, chad, ender
Reviewed By: chad
CC: aran
Maniphest Tasks: T2253
Differential Revision: https://secure.phabricator.com/D4302
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:
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:
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:
- Original command is in SSH_ORIGINAL_COMMAND, not normal argv.
- Use PhutilShellLexer to parse it.
- Fix a protocol encoding issue with ConduitSSHWorkflow. I think I'm going to make this protocol accept multiple commands anyway because SSH pipes are crazy expensive to build (even locally, they're ~300ms).
Test Plan: With other changes, successfully executed "arc list --conduit-uri=ssh://localhost:2222".
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T550
Differential Revision: https://secure.phabricator.com/D4232
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:
This is very preliminary and doesn't actually do anything useful. In theory, it uses Drydock to check out a working copy and run tests. In practice, it's not actually capable of running any of our tests (because of complicated interdependency stuff), but does check out a working copy and //try// to run tests there.
Adds various sorts of utility methods to various things as well.
Test Plan: Ran `reparse.php --harbormaster --trace <commit>`, observed attempt to run tests via Drydock.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015, T1049
Differential Revision: https://secure.phabricator.com/D4215
Summary:
Modernizes file uploads. In particular:
- Adds a mobile menu, with an "Upload File" item.
- Adds crumbs to the list view, detail view and upload view.
- Adds "Upload File" action to crumbs.
- Moves upload file to a separate page.
- Removes the combined upload file + recent files page.
- Makes upload file use a normal file control by default (works on mobile).
- Home page, file list and file upload page are now global drop targets which accept files dropped anywhere on them. Dragging a file into the window shows a mask and an instructional message.
- User education on this is a little weak but I think that's a big can of worms?
- Fixes a bug where dropping multiple files into a Remarkup text area produced bad results (resolves T2190).
T879 is related, although it's specifically about Maniphest. I've declined to make global drop targets yet there because there are multiple drop targets on the page with different meanings. That UI needs updating in general.
@chad, do we have an "upload" icon (counterpart to "download")?
Test Plan: Uploaded files in Maniphest, Differential, Files, and from Home. Dragged and dropped multiple files into Differential. Used crumbs, mobile.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2190
Differential Revision: https://secure.phabricator.com/D4200
Summary: Permit the forcible release of Drydock leases. The implementation isn't very exciting for now.
Test Plan: Released leases via web and CLI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D4181
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:
Add a basic breadcrumbs element, and implement it in Paste.
This needs some polish but is most of the way there.
Test Plan:
{F26443}
{F26444}
{F26445}
(This element is not visible on devices.)
Reviewers: chad
Reviewed By: chad
CC: aran, btrahan
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4087
Summary: I need to run this in `xargs`.
Test Plan:
$ echo 'E' | xargs -n 1 ./reparse.php --message --all
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4094
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:
A bunch of various fixes for the RHEL install script.
Most of them are stylistic, one of them fixes the EPEL repo release
RPM URL for RHEL 5. (enough acronyms there?)
Test Plan:
Tried installing on Fedora (which is treated as RHEL 6 due to how we handle
being unable to find version in the script).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4038
Summary: This does nothing fancy, just closes the resource and releases/breaks leases. They'll get cleaned up in some to-be-written GC process.
Test Plan: Closed resources from web UI and CLI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3998
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:
- Since we'll never serve these directly, move them to resources/. This makes generating the Celerity map faster and reduces the size of the result map, since we don't need to analyze resources we'll never serve.
- Also Rename the 2x `subscribe-remove` to `subscribe-delete` since they were named inconsistently. Everything else is in good shape.
Test Plan: Generated sprites as per D4025
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2013
Differential Revision: https://secure.phabricator.com/D4026
Summary: These are a bit out of date; bundle things together better.
Test Plan: Viewed page source for home, differential list, differential diff, maniphest list, maniphest task. Verified reasonable resource packaging.
Reviewers: chad, vrana, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4002
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:
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: Add a bin/drydock symlink and break it into workflows. Nothing too special here.
Test Plan: Ran `bin/drydock wait-for-lease`, `bin/drydock lease`, `bin/drydock help`, etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3867
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:
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
Summary: Add an Application class for Drydock and move routing rules there.
Test Plan: Looked at /applications/, clicked around drydock.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3847
Summary: fancy title. really just make the delete() method aware of related objects and build a quick workflow which calls delete(). also make commit delete savvy about audit requests.
Test Plan: deleted a repository per the instructions given to me in the web UI
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1416, T1958, T1372
Differential Revision: https://secure.phabricator.com/D3822
Summary: Quora requested this (moving to S3) but it's also clearly a good idea.
Test Plan:
Ran with various valid/invalid options to test options. Error/sanity checking seemed OK.
Migrated individual local files.
Migrated all my local files back and forth between engines several times.
Uploaded some new files.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1950
Differential Revision: https://secure.phabricator.com/D3808
Summary:
Make the example page a little more useful by showing available icons.
Also replace the "new" image, it had a little arrow which I thought was a "+". Use the one with a "+".
Test Plan: {F21966}
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3794
Summary: Generate a gunsights stylesheet entry for use in Releeph.
Test Plan: None!
Reviewers: edward
Reviewed By: edward
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3773
Summary:
- Better icons and action order.
- "Move Post" action.
- (Bugfix) Allow multiple blogs to be set to not having custom domains.
- Make "Write Post" skip the "select a blog" step when coming from a blog view.
- Sort blog list on "Write Post".
- Show messages when a post is a draft or not on a blog.
Test Plan: Created posts, blogs, moved posts, preview/live'd posts, etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3708
Summary:
Cleans up some of the mess I made in D3694. Basically:
- All blogs have an "internal" view with posts that uses mobile-friendly UIs, etc., so we don't have to do as much work with skins -- they just have to look pretty.
- Blogs now have a separate "live" view that we use to handle domains / skins.
- Simplified some views and use IDs in some URLs for consistency.
- Delete a bunch of edge/blogger/multi-blog code that's now obsolete.
Test Plan: Will attach screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3695
Summary:
- I made a "?" icon for help/reference.
- The `<>` icon was slightly too wide so I carved it down to 14x14.
- All the icons are in `/Phabriactor/remarkup_icon_sources.psd` if you want to tweak anything.
- Tooltips don't look like the mock but I'll tackle those separately.
- Removed strikethrough.
- Removed tag/image/text size for now since they don't have reasonable JS implementations yet.
- I think everything else is accurate to the mock.
Test Plan:
Normal state:
{F20621, size=full}
Hover + Click states:
{F20622, size=full}
Clicked state:
{F20620, size=full}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1848
Differential Revision: https://secure.phabricator.com/D3650
Summary: @chad, can you do the icon sheets based on 1.6? We're using a few icons not present in 1.5. I put the 1.6 "pro" source on Dropbox.
Test Plan:
Nav hover and selected states:
{F20598}
Launch hover state:
{F20596}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1856
Differential Revision: https://secure.phabricator.com/D3649
Summary: I needed to figure out how to do this, and it took a couple of minutes.
Test Plan: ran that command on my Ubuntu 12.04 system
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3479
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:
This is mostly to unblock D3547.
- Move "Macros" to a first-class application called "Macros".
- After D3547, this application will also house "Memes" (macros with text on them).
- This will also make them easier to find; the top navigational query I field is "where are image macros?" nowadays, since it's not intuitive they're part of files.
- This makes some of the UI mobile-aware but doesn't set the `device` flag yet, since there are still some missing pieces.
- I'll separate storage out and continue modernizing the UI as we unblock and integrate D3547.
Test Plan: Created, edited and deleted macros. Viewed files.
Reviewers: btrahan, vrana, teisenbe
Reviewed By: vrana
CC: aran
Maniphest Tasks: T175
Differential Revision: https://secure.phabricator.com/D3572
Test Plan: I wish I could tell that I ran it but I don't have the PNG files.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3582
Summary:
I was running the generate_ctags_symbols script on a project, and some
combination of this script and ctags was generating invalid rows for this
script. It's nice if this script can ignore invalid input instead of having
to add another step to filter out invalid input.
Test Plan: run script with input that has some invalid lines
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3532
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
Test Plan:
Built a DB provider using readonly connection for 'r' mode. Then:
$ arc unit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3484
Summary:
- Get rid of an AphrontSideNavView callsite.
- Modernize and simplify the application implementation.
- Doesn't work perfectly on tablet/phone but that's because not all the UI examples work there yet.
Test Plan: Looked at /applications/ and /uiexample/.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3431
Summary: the former is self explanatory. the latter is necessary for installations that require email verification. since many system agents are given bogus email address there can become a problem where these accounts can't be verified
Test Plan: created system agent account from scratch. edited user and toggled system agent accountness. created system agent with unverified email address and verified it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1656
Differential Revision: https://secure.phabricator.com/D3401
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: Permits the name and langauge of a paste to be edited. This will eventually allow the visibility policy to be edited as well.
Test Plan: Edited name/langauge of some pastes. Tried to edit a paste I didn't own, was harshly rebuffed.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1690
Differential Revision: https://secure.phabricator.com/D3376
Summary:
Adds a flexible navigation menu to diffs that shows you your current position in the diff.
Anticipating some "this is the best thing ever" and some "this is the wosrt thing ever" on this, but let's see how much pushback we get? It seems pretty good to me.
Test Plan: Will attach screenshots.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1633, T1591
Differential Revision: https://secure.phabricator.com/D3355
Summary:
This is another experiment for reducing reviewers response time.
I stole the idea (and colors) from [[ http://www.reviewboard.org/media/screenshots/2009/02/02/dashboard.png | ReviewBoard ]].
I actually quite like it (except when everything is red) and I can image that people will review just to have better color balance.
The code is not production ready for these reasons:
- We load holidays again and again for each revision. I couldn't cache it to static variable because it could persist multiple requests, right?
- I don't know how to expand height to the whole cell (I'm really bad in CSS).
- CSS rules are probably in wrong file.
- We probably want to use different colors.
This is how it looks:
{F16406}
Test Plan: Displayed revision list.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3190
Summary:
- Fix width, corresponding to wider sprites.
- Sprite the "Audit" icon.
- Mark the meta-application as device-ready.
- Fix some collapse/expand bugs with the draggable local navs.
- Add texture to local nav.
- Darken the application nav to make it more cohesive with the main nav. I think this is an improvement?
Test Plan: See screenshots.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran, netfoxcity
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3338
Summary:
For Nefarious Facebook Internal Purposes, which may or may not
include incremental symbol database updates.
Give the import script an option to not clear all symbols from the
project, and make a new script that clears only symbols from paths given
on stdin.
(I'm not yet sure how much of the NFIPs is going to be ported over
here. So there might be a future diff that uses this. Conversely,
since there's no real use case out here, I'm fine just moving this to
the Facebook configuration if necessary.)
Test Plan: Run scripts in innumerable bizarre and eldritch configurations.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1602
Differential Revision: https://secure.phabricator.com/D3305
Summary:
- Add a PhabricatorApplication.
- Make most of the views work well on tablets / phones. The actual "Create" form doesn't, but everything else is good -- need to make device-friendly form layouts before I can do the form.
Test Plan: Will attach screenshots.
Reviewers: btrahan, chad, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3293
Summary:
See D3277, D3278.
- Sprite all the menu icons.
- Delete the unsprited versions.
- Notification bolt now uses the same style as everything else.
Test Plan: Looked at page, hovered, clicked things.
Reviewers: btrahan, chad, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3279
Summary: This probably needs some tweaks, but gets the point across. I'll lay in the actual usage in the next diff.
Test Plan: Looked at generated CSS/PNG.
Reviewers: btrahan, chad, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3278
Summary:
Update how arguments get parsed in reparse.php and add two new options
--force-local and --min-date (both to be used with --all). This diff also
fixes a bug in destroy_revision.php
Test Plan: ran the script
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3275
Summary: we had a hole in scaling pics from oauth from a bit. that was patched in D2848. this cleans up the old data.
Test Plan: ran it in production. issue as described on task is resolved. also spot checked some profiles and they look good.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1634
Differential Revision: https://secure.phabricator.com/D3268
Summary: See D3252.
Test Plan: This one is nasty to test, I'm going to make some coffee first.
Reviewers: davidreuss, vrana, btrahan
Reviewed By: davidreuss
CC: aran
Maniphest Tasks: T452
Differential Revision: https://secure.phabricator.com/D3254
Summary:
We can use `.gitattributes` instead but there's no way how to set repository config for all users in Git, right?
So provide a script writting to `.git/info/attributes` instead so that we don't have to .gitignore `.gitattributes`.
Test Plan:
$ scripts/celerity/install_merge.sh
$ git pull # with merge conflict in Celerity map
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3228
Summary:
Scripts now return scoped symbols -- in particular, PHP class constants, fields, and methods. ctags gives some for other languages.
(Turns out XHPAST doesn't support traits. But no one uses traits anyway so it's probably fine.)
I couldn't find a list of the context types ctags uses (class/struct/union/enum/maybe others?), so the context code just ignores that. Also, it uses a blacklist for the symbol type instead of a whitelist because there are a ton, they vary by language, and I didn't want to unintentionally exclude anything (P480).
Test Plan: Scrape symbols from arcanist and phabricator. Upload them to sandbox. Search for things.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran, Korvin
Maniphest Tasks: T1602
Differential Revision: https://secure.phabricator.com/D3202
Summary:
- import_project_symbols supports an optional extra field, which is the
context of the symbol.
- Symbol query can take a symbol argument, either as a parameter or a
URL component (so you can now jump nav to `s Zerg/rush`, for
example).
- Conduit method not yet updated. Will do that later.
NOTE: Not providing a context is distinct from providing an empty
context, because an empty context stands for top-level context, i.e.
functions and classes for PHP. It will not find class methods, etc. It
is possible that we should use some weird token that could not normally
be a context name to stand in for empty context.
Test Plan: Do a bunch of symbol searches.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran, Korvin
Maniphest Tasks: T1602
Differential Revision: https://secure.phabricator.com/D3148
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:
- Add PhabricatorApplication. This is a general class that I have grand designs for, but used here to allow applications to provide objects for analysis by the facts appliction.
- Add FactCursors, to keep track of where iterators are.
- Make the daemon do something sort of useful.
- Add `bin/fact cursors` for showing and managing objects and cursors.
- Add some options to `bin/fact analyze`.
Test Plan:
- `bin/fact cursors`, `bin/fact cursors --reset DifferentialRevision`, `bin/fact cursors --reset X`
- `bin/fact analyze`, `bin/fact analyze --all`, `bin/fact analyze --iterator DifferentialRevision --skip-aggregates`
- `bin/phd debug fact`
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1562
Differential Revision: https://secure.phabricator.com/D3098
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: 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:
See D2991 / T1526. Two major changes here:
- PHP just straight-up logs passwords on ldap_bind() failures. Suppress that with "@" and keep them out of DarkConsole by enabling discard mode.
- Use PhutilOpaqueEnvelope whenever we send a password into a call stack.
Test Plan:
- Created a new account.
- Reset password.
- Changed password.
- Logged in with valid password.
- Tried to login with bad password.
- Changed password via accountadmin.
- Hit various LDAP errors and made sure nothing appears in the logs.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2993
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:
binary_safe_diff is needed in arcanist too. Moved it over to
arcanist. See D2915.
Test Plan: diffusion page rendered correctly on binary file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2916
Summary: I broke this when converting to PhabricatorUserEditor.
Test Plan:
Ran `accountadmin`, created an admin account, verified the "new value" column showed "Y".
ACCOUNT SUMMARY
OLD VALUE NEW VALUE
Username derp
Real Name derprp
Email derp@derp.com
Password Unchanged
Admin N Y
Reviewers: btrahan, nikil
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1434
Differential Revision: https://secure.phabricator.com/D2908
Summary:
Nothing new or exciting here yet, just moving the random scripts/repositories/ things to bin/repository. Also add `repository list`.
(Console stuff comes from D2841.)
Test Plan: Ran `repository list`, `repository pull`, `repository discover`, `repository discover --verbose`, `repository help`.
Reviewers: jungejason, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2849
Summary: Add verbose logging. This logging is activated by setting "phd.verbose" in the config, running "phd debug", or explicitly in scripts/repository/pull.php and scripst/repository/discover.php
Test Plan:
>>> orbital ~/devtools/phabricator $ ./scripts/repository/discover.php GTEST
Discovering 'GTEST'...
<VERB> PhabricatorRepositoryPullLocalDaemon Discovering commits in repository 'GTEST'...
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch '()_+abcd$100', at a37bc285a12efa7224fe19f3df54cd90fa2b897a.
<VERB> PhabricatorRepositoryPullLocalDaemon Skipping, HEAD is known.
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch '+abcd$100', at a37bc285a12efa7224fe19f3df54cd90fa2b897a.
<VERB> PhabricatorRepositoryPullLocalDaemon Skipping, HEAD is known.
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch '_+abcd$100', at a37bc285a12efa7224fe19f3df54cd90fa2b897a.
<VERB> PhabricatorRepositoryPullLocalDaemon Skipping, HEAD is known.
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch 'abcd$100', at a37bc285a12efa7224fe19f3df54cd90fa2b897a.
<VERB> PhabricatorRepositoryPullLocalDaemon Skipping, HEAD is known.
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch 'arcpatch', at 774c7737b2d560a291697126bf4513204ccf661a.
<VERB> PhabricatorRepositoryPullLocalDaemon Skipping, HEAD is known.
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch 'arcpatch-1', at dc97539bee07293f95990d71f4638335a2531d69.
<VERB> PhabricatorRepositoryPullLocalDaemon Skipping, HEAD is known.
<VERB> PhabricatorRepositoryPullLocalDaemon Examining branch 'arcpatch-2', at 1acfaec313c46dd3caa90448800181fb91b0270f.
Reviewers: jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2843
Summary: Installs may set global translation which may not be available before loading libraries.
Test Plan:
$ aphrontpath.php /
Reviewers: nh
Reviewed By: nh
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2819
Summary: loadRunningDaemons loads daemons that may be running (have a file pid). other commands handle this appropriately so just make sure the start command knows whats up
Test Plan: phd start, stop, reload all seemed to work just fine.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1284
Differential Revision: https://secure.phabricator.com/D2808
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:
- 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:
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:
- We currently have some bugs in account creation due to nontransactional user/email editing.
- We save $user, then try to save $email. This may fail for various reasons, commonly because the email isn't unique.
- This leaves us with a $user with no email.
- Also, logging of edits is somewhat inconsistent across various edit mechanisms.
- Move all editing to a `PhabricatorUserEditor` class.
- Handle some broken-data cases more gracefully.
Test Plan:
- Created and edited a user with `accountadmin`.
- Created a user with `add_user.php`
- Created and edited a user with People editor.
- Created a user with OAuth.
- Edited user information via Settings.
- Tried to create an OAuth user with a duplicate email address, got a proper error.
- Tried to create a user via People with a duplicate email address, got a proper error.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: tberman, aran
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2569
Summary:
- Delete an unreachable block of code.
- Pass "--" to terminate overseer args.
Test Plan: Ran "phd repository-launch-readonly", didn't get argument errors out of the daemon.
Reviewers: jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2456
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:
See D2418. This merges the commit discovery daemon into the same single daemon, and applies all the same rules to it.
There are relatively few implementation changes, but a few things did change:
- I simplified/improved Mercurial importing, by finding full branch tip hashes with "--debug branches" and using "parents --template {node}" so we don't need to do separate "--debug id" calls.
- Added a new "--not" flag to exclude repositories, since I switched to real arg parsing anyway.
- I removed a web UI notification that you need to restart the daemons, this is no longer true.
- I added a web UI notification that no pull daemon is running on the machine.
NOTE: @makinde, this doesn't change anything from your perspective, but it something breaks this is the likely cause.
This implicitly resolves T792, because discovery no longer runs before pulling.
Test Plan:
- Swapped databases to a fresh install.
- Ran "pulllocal" in debug mode. Verified it correctly does nothing (fixed a minor issue with min() on empty array).
- Added an SVN repository. Verified it cloned and discovered correctly.
- Added a Mercurial repository. Verified it cloned and discovered correctly.
- Added a Git repository. Verified it cloned and discovered correctly.
- Ran with arguments to verify behaviors: "--not MTEST --not STEST", "P --no-discovery", "P".
Reviewers: btrahan, csilvers, Makinde
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T792
Differential Revision: https://secure.phabricator.com/D2430
Summary:
Allow the pull daemon to take a list of repositories. By default, pull all repositories.
Make some effort to respect pull frequencies, although we'll necessarily suffer a bit if running with only one process.
NOTE: We still launch one discovery daemon per working copy, so this only cuts the daemon count in half.
Test Plan:
- Ran `phd debug pulllocal`, verified behavior.
- Ran `pull.php P MTEST SVNTEST --trace`, verified it pulled the repos and ran the right commands.
- Ran `phd repository-launch-master`, verified the right daemons launched, checked daemon console.
- Ran `phd repository-launch-readonly`, verified the right daemon launched, checked daemon console.
Reviewers: btrahan, csilvers, davidreuss
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2418
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: This script is not used since D976.
Test Plan:
grep init_env
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2379
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:
I think this improves things, let me know if you have feedback.
Also addresses T840.
Test Plan: See screenshots...
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, zeeg
Maniphest Tasks: T840
Differential Revision: https://secure.phabricator.com/D2357
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:
Also reduce the memory usage a little bit (before increasing it again).
I use the same CSS class as for the copied code.
Test Plan: Parsed 100 diffs and checked about 10 of them - looks good.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2339
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: I usually don't dare to fix English but this one doesn't seem correct even to me.
Test Plan: Read.
Reviewers: epriestley, jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2214
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: People are hella lazy and don't want to do this themselves.
Test Plan: Generated a symbol file with duplicates and piped it in, got an import under --ignore-duplicates.
Reviewers: kdeggelman, btrahan, vrana, jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2145
Summary: detect all revisions that don't have a diff, then delete them.
Test Plan:
we have been using this script for several months in
Facebook and it's working well.
Reviewers: epriestley, nh, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T605
Differential Revision: https://secure.phabricator.com/D2061
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:
We spend a significant amount of time running includes, even with APC. However, we have rigidly structured includes and can safely run them all in workers before requests occur.
Right now, requests go like this:
- Apache spawns a worker.
- Client sends an HTTP request.
- Apache interprets it.
- Apache sees it's ".php", so it hands it off to the PHP SAPI.
- The PHP SAPI starts the PHP interpreter in the worker.
- The request is handled, etc.
Instead, we want to do this:
- Worker spawns and loads the world.
- Client sends an HTTP request.
- Webeserver interprets it.
- Sees it's a ".php", hands it off to the SAPI.
- SAPI executes it on a loaded world.
No SAPIs I know of support this, but I added support to PHP-FPM fairly easily (in the sense that it took me 6 hours and I have a hacky, barely-working mess). Over HTTP (vs HTTPS) the performance improvement is pretty dramatic.
HPHP doesn't significantly defray this cost so we're probably quite a bit faster (to the user) under nginx+PHP-FPM than HPHP after this works for real.
I have the php-fpm half of this patch in a messy state, I'm going to try to port it to be vs php 5.4.
Test Plan: Ran a patched php-fpm, browsed around, site works, appears dramatically faster.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2030
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:
I noticed that documentation said it is possible to have 'ctags' symbol import, so I hacked a quick version. I tested it on Python based project and successfuly imported symbols.
It is limited to classes right now, as the importer script complained about not-unique method names (there are a lot of 'get' & 'post' methods accross classes in my project).
If you would have any feedback about this, I would definetly try to wrap it up for possibly merging into main repository.
Test Plan:
Required 'ctags' tool (ctags.sourceforge.net/) Tested to work with version 5.8+ and didn't work with 3.x.
1. `find . -type f '*.py' | ./generate_ctags_symbols.php > /tmp/symbols`
2. `./import_project_symbols.php` < /tmp/symbols
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: seporaitis, aran, epriestley
Maniphest Tasks: T1034
Differential Revision: https://secure.phabricator.com/D1995
Summary:
libphutil has some basic environmental sanity checks that we should use when initializing scripts in Phabricator. Principally this:
https://secure.phabricator.com/diffusion/PHU/browse/master/scripts/__init_script__.php;db643ee9f5f524e7$26
Without this, the default ini may set CLI errors to go to some logfile, which means exceptions aren't shown on stderr.
See https://github.com/facebook/phabricator/issues/98/
Test Plan:
- Ran "php -derror_log=/dev/null -f ./bin/phd debug adslkfnasdfnalks" prior to change; got confusing lack of output.
- Ran "phd -derror_log=/dev/null -f ./bin/phd debug asdkflnaslfdnala" after change; got exception on stderr.
Reviewers: btrahan, killermonk
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1950
Summary: I'm not aware of an easy way to get this information through normal tools. I'm sure there's some fancy GUI client that has it but this seems worth keeping around.
Test Plan: ran script, got helpful information about data sizes
Reviewers: nh, btrahan
Reviewed By: nh
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1916
Summary:
Smaller diffs, better blame, less noise on a file which is usually ignored.
Less conflicts in merge!
Test Plan: php -l __celerity_resource_map__.php
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1909
Summary: Tests and docstubs don't provide anything and they are not required in celerity map.
Test Plan: View Options in Differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1893
Summary: If someone accidentally pushes a bunch of commits, revisions might get marked as "Committed" incorrectly. This will restore them to their previous state without too much fuss.
Test Plan: Ran the script on some commits to undo them, it seemed to work correctly.
Reviewers: davidreuss, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1877
Summary: This is the script used for conversion: P319
Test Plan:
Update diff with UTF-8 characters in description.
`sql/upgrade_schema.php`
Verify data in DB and that it looks good on web.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T327
Differential Revision: https://secure.phabricator.com/D1830
Summary: It would be better to test if a key is passworded, but I couldn't figure out how to do that.
Test Plan: Ran "test_connection.php"
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T924
Differential Revision: https://secure.phabricator.com/D1856
Summary: Urgent request.
Test Plan: Ran "add_macro.php" on some things; verified results in web console.
Reviewers: btrahan, tcook
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1832
Summary: New implicit fallthrough linter detected a few issues; none of these have behavioral impacts but they can clearly be tightened up. See D1824.
Test Plan: Lint; inspection.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1825
