Summary:
This is an attempt to satisfy a lot of the one-off requests a little more
generally, by providing a relatively generic piece of event architecture.
Allow the registation of event listeners which can react to various application
events (currently, task editing).
I'll doc this a bit better but I wanted to see if anyone had massive objections
to doing this or the broad approach. The specific problem I want to address is
that one client wants to do a bunch of routing for tasks via email, so it's
either build a hook, or have them override most of ManiphestReplyHandler, or
something slightly more general like this.
Test Plan: Wrote a silly listener that adds "Quack!" to a task every time it is
edited and edited some tasks. I was justly rewarded.
Reviewers: nh, jungejason, tuomaspelkonen, aran
Reviewed By: aran
CC: aran, epriestley
Differential Revision: 881
Summary:
Splitting up D960 a bit, see that for context.
We currently have two scripts, "parse_one_commit.php" and
"reparse_all_commit_messages.php", but they're sort of silly and you can't do
certain things with them. Replace them with one script which is more flexible
and can do specific reparse steps on individual commits or entire repos.
I left the old scripts as stubs since I think there are some FB wiki docs and
stuff that mention them. I'll delete them in a month or whenever I remember or
something.
Test Plan: Ran "reparse.php" with various arguments, including vs-one-commit,
vs-repository, with --trace, and against different types of repos.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 964
Summary:
We currently rely on "remote_hooks_enabled" in .arcconfig to determine whether
commands like "arc amend" and "arc merge" should imply "arc mark-committed".
However, this is a historical artifact that is now bad for a bunch of reasons:
- The option name is confusing, it really means 'repository is tracked'.
- The option is hard to discover and generally sucks.
- We can empirically determine the right answer since we now know if a project
is in a tracked repository.
Add a call which arcanist can make on these workflows to figure out if it is
interacting with a project in a tracked repository or not.
Also added an "isTracked()" convenience method to reduce the number of magic
strings all over the place.
Test Plan: Ran "arcanist.projectinfo" for nonexistent, untracked and tracked
projects.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, epriestley, Makinde
Differential Revision: 945
Summary:
Repository import has three major steps:
- Commit discovery (serial)
- Message parsing (parallel, mostly VCS independent)
- Change parsing (parallel, highly VCS dependent)
This implements commit discovery for Mercurial, similar to git's parsing:
- List the heads of all the branches.
- If we haven't already discovered them, follow them back to their roots (or
the first commit we have discovered).
- Import all the newly discovered commits, oldest first.
This is a little complicated but it ensures we discover commits in depth order,
so the discovery process is robust against interruption/failure. If we just
inserted commits as we went, we might read the tip, insert it, and then crash.
When we ran again, we'd think we had already discovered commits older than HEAD.
This also allows later stages to rely on being able to find Phabricator commit
IDs which correspond to parent commits.
NOTE: This importer is fairly slow because "hg" has a large startup time
(compare "hg --version" to "git --version" and "svn --version"; on my machine,
hg has 60ms of overhead for any command) and we need to run many commands (see
the whole "hg id" mess). You can expect something like 10,000 per hour, which
means you may need to run overnight to discover a large repository (IIRC, the
svn/git discovery processes are both about an order of magnitude faster). We
could improve this with batching, but I want to keep it as simple as possible
for now.
Test Plan: Discovered all the commits in the main Mercurial repository,
http://selenic.com/repo/hg.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 943
Summary:
Move toward storing credentials in configuration so it's easier to get the
daemons working. This should eventually solve all the key juggling junk you have
to do right now.
This only gets us part of the way to actually using these credentials in the
daemons since I have to go swap everything for $repository->execBlah().
I tried to write a web "Test Connection" button but it was too much of a mess to
get git to work since git doesn't give you access to its SSH command and SSH has
a bunch of interactive prompts which you can't really do anything about without
it or a bunch of ~/.ssh/config editing. This is what Git recommends:
https://git.wiki.kernel.org/index.php/GitFaq#How_do_I_specify_what_ssh_key_git_should_use.3F
..but it's not a great match for this use case.
Test Plan:
- Only partial.
- Ran "test_connection.php" on a Git repo with and without SSH, and with and
without valid credentials. This part works properly.
- Ran "test_connection.php" on a public SVN repo, but I don't have private or
WEBDAV repos set up at the moment.
- Mercurial doesn't work yet.
- Daemons haven't been converted yet.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, abdul, nmalcolm, epriestley, jungejason
Differential Revision: 888
Summary: We already detect this in setup but double-check just in case, see
gmail thread.
Test Plan: Rebuilt PHP without pcntl and tried to run phd, got an error.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran
Differential Revision: 819
Summary:
Provide a catchall mechanism to find unprotected writes.
- Depends on D758.
- Similar to WriteOnHTTPGet stuff from Facebook's stack.
- Since we have a small number of storage mechanisms and highly structured
read/write pathways, we can explicitly answer the question "is this page
performing a write?".
- Never allow writes without CSRF checks.
- This will probably break some things. That's fine: they're CSRF
vulnerabilities or weird edge cases that we can fix. But don't push to Facebook
for a few days unless you're prepared to deal with this.
- **>>> MEGADERP: All Conduit write APIs are currently vulnerable to CSRF!
<<<**
Test Plan:
- Ran some scripts that perform writes (scripts/search indexers), no issues.
- Performed normal CSRF submits.
- Added writes to an un-CSRF'd page, got an exception.
- Executed conduit methods.
- Did login/logout (this works because the logged-out user validates the
logged-out csrf "token").
- Did OAuth login.
- Did OAuth registration.
Reviewers: pedram, andrewjcg, erling, jungejason, tuomaspelkonen, aran,
codeblock
Commenters: pedram
CC: aran, epriestley, pedram
Differential Revision: 777
Summary:
A bunch of the .sql patch files don't explicitly specify the table engine, but
we should always use InnoDB with the exception of one table which needs MyISAM
for FULLTEXT.
MySQL doesn't no-op an ALTER TABLE statment that changes the engine back to
itself and converting large tables can be time consuming, so convert only the
required tables.
Test Plan: Ran on secure.phabricator.com and my local box, it fixed all the
issues in about 3 seconds on secure.phabricator.com and <<1 second on my local.
Reviewed By: codeblock
Reviewers: codeblock, tuomaspelkonen, jungejason, aran
CC: aran, epriestley, codeblock
Differential Revision: 641
Summary:
People want to see all the tasks they have subscribed to in one
view. A new table was added for this to make queries faster.
Test Plan:
Tested that the view was initially empty. After running the
reindex_maniphest.php script, I saw the correct tasks there. Added
myself as a subscriber to one task and made sure the view was
updated. Removed myself as a subscriber from one task and made sure
the view was updated again.
Reviewed By: epriestley
Reviewers: epriestley, jungejason, codeblock
CC: aran, rm, epriestley
Differential Revision: 603
Summary:
Phabricator generates a bunch of data that we don't need to keep around forever,
add a GC daemon to get rid of it with some basic configuration options.
This needs a couple more diffs to get some of the details but I think this is a
reasonable start.
I also fixed a couple of UI things related to this, e.g. the daemon logs page
going crazy when a daemon gets stuck in a loop and dumps tons of data to stdout.
Test Plan:
- Ran gc daemon in 'phd debug' mode and saw it delete stuff, then sleep once
it had cleaned everything up.
- Mucked around with TTLs and verified they work correctly.
- Viewed gc'd transcripts in the web interface and made sure they displayed
okay.
- Viewed daemon logs before/after garbage collection.
- Running some run-at / run-for tests now, I'll update if the daemon doesn't
shut off in ~10-15 minutes. :P
Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 583
Summary: We don't currently install this component, but should.
Test Plan: iiam
Reviewed By: codeblock
Reviewers: codeblock
CC: aran, codeblock
Differential Revision: 560
Summary:
Major things taking place here:
- A new table for storing <task, project> relationships.
- Moved all task query logic into a dedicated class.
- Added a "projects" filter to the UI.
I was originally going to try to drive this off the main search index but the
perf benefits of a custom schema make an overwhelming argument in favor of doing
it this way.
Test Plan:
Filtered tasks by author and owner and zero, one, and more than one project.
Exercised all the group/sort options. Ran the index script over my 100k task
corpus. Edited task-project membership and verified the index updated.
Reviewed By: cadamo
Reviewers: gc3, jungejason, cadamo, tuomaspelkonen, aran
CC: aran, cadamo, epriestley
Differential Revision: 556
Summary:
Currently we send logs to the logfile in 'phd debug', but we should send them to
the console instead.
Also fixed some %C stuff which could theoretically cause problems if a user had
percentage symbols in their paths (heaven forbid).
fratrik, this or D535 might have been involved in frustrating your efforts to
debug the "sudo" stuff.
Test Plan:
Ran "phd debug irc derpderp" and "phd launch irc derpderp". In the former case,
the exception appeared in the console. In the latter, it appeared in the log.
Reviewed By: codeblock
Reviewers: codeblock, jungejason, tuomaspelkonen, aran, fratrik
CC: aran, codeblock
Differential Revision: 536
Summary:
create the indexer for commit
Test Plan:
run the reindex_one_commit.php against one existing commit
and it is returned in search result on the webpage; run parse_one_commit
against another commit; modified reindex_everything.php to let it only
parse one commit after loading all commits.
Reviewed By: epriestley
Reviewers: epriestley, aran
CC: aran, jungejason, epriestley, debow
Differential Revision: 490
Summary:
Add users to the search results. I need to follow this up with a patch to make
the search results stop being terrible. I'll do that.
Test Plan:
Searched for users, ran "reindex_all_users.php"
Reviewed By: jungejason
Reviewers: tomo, jungejason, aran
CC: aran, jungejason
Differential Revision: 508
Summary:
remove accessing the db config info directly. Use
DatabaseConfigurationProvider instead. Also fixed a minor issue where
different number of newlines are output in PhabricatorSetup.php's output.
Test Plan:
executed upgrade_schema.php; executed PhabricatorSetup.php by
setting 'phabricator.setup' to true.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 443
Summary:
Make it easier to find obvious problems in daemons by letting them run
undaemonized in the console without requiring the user to know the magical
incantations for loading libraries, etc.
Test Plan:
Ran "phd debug nice", simulated some failures (e.g., bringing down Phabricator,
daemon fatal) and got useful error messages.
Reviewed By: jungejason
Reviewers: toulouse, jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 448
Summary:
- Make the instructional text generally more useful.
- Show the current configured adapter.
- When the configuration prevents outbound email from being delivered, show a
warning.
- Detect 'curl' extension during setup since it's more-or-less required
- Add curl extension to the install scripts
codeblock: can you verify the rhel-derivs changes are correct?
Test Plan:
Set adapter to test, verified warning; entered setup mode and verified curl. Ran
apt-get on an ubuntu box. Ran yum on an amazon linux box.
Reviewed By: toulouse
Reviewers: toulouse, codeblock
Commenters: codeblock
CC: aran, jungejason, tuomaspelkonen, codeblock, epriestley, toulouse
Differential Revision: 438
error message if that doesn't work
Summary:
This workflow is needlessly bad right now, make it not terrible.
Also removed some related, unused code.
Test Plan:
Ran 'phd launch nice' with no directory and with a failing mkdir command.
Reviewed By: toulouse
Reviewers: hsb, toulouse, codeblock
CC: aran, toulouse
Differential Revision: 440
Summary:
Mail clients can send messages where the body is represented as 'inline'
attachments. Don't treat any such text attachments as actual attachments.
Test Plan:
toulouse, can you verify this fixes the issue?
Reviewed By: toulouse
Reviewers: toulouse
CC: aran, toulouse, epriestley
Differential Revision: 441
Summary:
This already pretty much works, document it explicitly.
Test Plan:
Moved my MySQL server over to port 3307.
Reviewed By: aran
Reviewers: jungejason, aran
CC: aran
Differential Revision: 411
Summary: Implements a simple infrastructure for keyboard shortcuts, see T184, and a "help" shortcut.
There's a lot of room for refinement here but I think it basically works. Each shortcut can also provide a "tooltip" handler which allows it to show help when the alt/option key is held down.
Test Plan: Pressed "?" and got help. Pressed "?" in various contexts where it should not activate (modifier keys, text input focused) and didn't get help.
Reviewers: aran, tuomaspelkonen, jungejason
CC: moskov
Differential Revision: 362
Summary: Normally this gives you a prompt about taking down services, provide a noninteractive mode for scripting the upgrade process.
Also drop a generally bad/confusing/irrelevant piece of advice from the documentation and replace it with information about -f.
Test Plan: Ran with and without -f. Ran with -h.
Reviewers: moskov, tuomaspelkonen, jungejason, aran
CC:
Differential Revision: 387
Summary:
Simple script to install dependencies on Ubuntu.
There's probably lots of room for improvement here.
Test Plan:
Imaged a clean Ubuntu box in EC2 and ran this script, it appeared to work?
Reviewed By: tuomaspelkonen
Reviewers: kevinwallace, jungejason, tuomaspelkonen, aran
Commenters: aran
CC: moskov, aran, tuomaspelkonen, epriestley
Differential Revision: 384
Summary:
In RHEL6 at the least, pcntl installs from distro package management to the CLI
but not to Apache. Since we don't need it in apache and it's a pain to build
manually, just verify it exists on the CLI.
Test Plan:
Simulated script failures and verified setup output.
Reviewed By: codeblock
Reviewers: codeblock, aran, jungejason, tuomaspelkonen
CC: aran, epriestley, kevinwallace, codeblock
Differential Revision: 380
Summary:
the daemon is generating a lot of warning about timezone not
being set. Add it.
Test Plan:
php -l. Launched the daemon and it didn't crash. It's
difficult to make it parse a commit on my sandbox though. I will see if
it fixes the problem once it is live.
Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
CC: aran, epriestley, jungejason
Differential Revision: 356
Summary:
Keep him from getting killed every 24 hours by the overseer, add basic commit
support.
Test Plan:
Ran irc bot, fed him a commit, fed him "http://blah/D1".
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen, codeblock, mroch
CC: aran, epriestley
Differential Revision: 377
Summary:
Allow files to be attached to a task by attaching them to an email reply to the
task.
Test Plan:
Applied this patch live since I haven't managed to get inbound email configured
locally, then attached files to a task via email.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
CC: anjali, aran, jungejason
Differential Revision: 369
Summary:
Currently, we echo the password as the user types it. This turns out to be a bit
of an issue in over-the-shoulder installs. Instead, disable tty echo while the
user is typing their password so nothing is shown (like how 'sudo' works).
Also show a better error message if the user chooses a duplicate email; without
testing for this we just throw a duplicate key exception when saving, which
isn't easy to understand. The other duplicate key exception is duplicate
username, which is impossible (the script updates rather than creating in this
case).
There's currently a bug where creating a user and setting their password at the
same time doesn't work. This is because we hash the PHID into the password hash,
but it's empty if the user hasn't been persisted yet. Make sure the user is
persisted before setting their password.
Finally, fix an issue where $original would have the new username set, creating
a somewhat confusing summary at the end.
I'm also going to improve the password behavior/explanation here once I add
welcome emails ("Hi Joe, epriestley created an account for you on Phabricator,
click here to login...").
Test Plan:
- Typed a password and didn't have it echoed. I also tested this on Ubuntu
without encountering problems.
- Chose a duplicate email, got a useful error message instead of the exception
I'd encountered earlier.
- Created a new user with a password in one pass and logged in as that user,
this worked properly.
- Verified summary table does not contain username for new users.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: moskov, jr, aran, jungejason
Differential Revision: 358
Summary:
Some PHP has junky defaults for error_reporting / display_errors, and the "@"
silences fatals. The @ should never have been there, I just copied it from the
libphutil initializer where we use @ because the default error message can be
confusing and we display a more useful one.
Test Plan:
Added fatals to my conf file, got a decent error message instead of silent exit
with err=255.
Reviewed By: aran
Reviewers: tuomaspelkonen, aran, jungejason
CC: aran
Differential Revision: 355
Summary:
Get rid of the Phabricator-level DarkConsole-specific API and use the more
general Phutil-level one.
Test Plan:
Loaded DarkConsole services plugin, viewed Diffusion, got execs in the trace.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran
Differential Revision: 293
Summary:
There was an old "create_user.php" script but it really was only useful for
creating agents. Provide a more user-friendly script for creating the first
account.
Depends on D278.
Test Plan:
Used 'accountadmin' to create and edit accounts. Read documentation.
Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran
CC: ccheever, aran, tuomaspelkonen
Differential Revision: 279
Summary:
Alters the installation instructions to guide installers into a "setup" mode
which does config file sanity checking.
Test Plan:
Put myself in setup mode, simulated all the failures it detects, took myself out
of setup mode, Phabricator works OK.
Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 230
Summary:
We always return HTTP 200 right now and don't send a "Last-Modified" header, so
browsers download more data then necessary if you sit on a page mashing reload
(for example).
Test Plan:
Used Charles to verify HTTP response codes from 400, 404 and 304 responses.
Mashed reload a bunch and saw that the server sent back 304s.
Changed the resource hash seed and saw 200s, then 304s on reload.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: bmaurer, aran, tuomaspelkonen
Differential Revision: 253
Summary:
Javelin is currently embedded in Phabricator via copy-and-paste of prebuilt
packages. This is not so great.
Pull it in as a submodule instead and make all the Phabriator resources declare
proper dependency trees. Add Javelin linting.
Test Plan:
I tried to run through pretty much all the JS functionality on the site. This is
still a high-risk change, but I did a pretty thorough test
Differential: inline comments, revealing diffs, list tokenizers, comment
preview, editing/deleting comments, add review action.
Maniphest: list tokenizer, comment actions
Herald: rule editing, tokenizers, add/remove rows
Reviewed By: tomo
Reviewers: aran, tomo, mroch, jungejason, tuomaspelkonen
CC: aran, tomo, epriestley
Differential Revision: 223
Summary:
Sendmail isn't actually OK with passing ENV stuff via 'aliases', accept it as an
argument instead.
Test Plan:
Sent real email to a real server, got differential updates!
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 233
Summary:
Provides support for per-user x per-object unique reply-to email addresses, plus
SMTP integration.
This does not actually make Phabricator use these in outbound email.
Test Plan:
Used test console to validate in-Phabricator routing and handling.
Piped emails into the "mail_handler.php" script to validate mail parsing.
Configured sendmail and sent mail to Phabricator.
Technically I haven't conducted all parts of this test on the same machine since
I lost the will to configure more SMTP servers after configuring phabricator.com
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 226
Summary:
Restores the old "pokedex" feature and allows easy definition of new macros.
Only good can come of this!
Critical feature!!
Test Plan:
nyancat
Reviewed By: tuomaspelkonen
Reviewers: aran, bh, tomo, jungejason, tuomaspelkonen
CC: aran, tuomaspelkonen
Differential Revision: 220
Summary:
there are several places we open an 'r' connection but use it
for writing. Fix them.
Test Plan:
ran parse_one_commit.php against one revision which executes
the code with problem. It used to throw exception. Now it works fine.
Reviewed By: Girish
Reviewers: tuomaspelkonen, Girish
Commenters: aran
CC: aran, Girish
Differential Revision: 213
Summary:
In a basically reasonable configuration where you connect
with a non-privileged user from the web workflow, upgrade_schema.php
won't have enough privileges. Allow the user to override the normal
auth with -u and -p.
Test Plan:
Tried to do a schema upgrade with an underprivileged user,
got a useful error message instead of garbage.
Reviewed By: Girish
Reviewers: Girish, davidrecordon, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, Girish
Differential Revision: 191
Summary:
This global 'const' syntax was introduced in PHP 5.3:
http://www.php.net/manual/en/language.constants.syntax.php
We're PHP 5.2.x elsewhere so just use define(). Made the constant a little more
specific too.
Test Plan:
Ran upgrade_schema.php script.
Reviewed By: Girish
Reviewers: tuomaspelkonen, Girish, davidrecordon
CC: jungejason, aran, epriestley, Girish
Differential Revision: 190
Summary: While I should fix the transactional stuff, that patch is going to be
tricky and transactions have some performance implications. This is a simple
fix which prevents the race.
Instead of having the data point at the event ID, have the event point at a
data ID. Insert the data first, then insert the event with the right data
pointer. This is super simple and prevents the race issue.
Test Plan:
- Ran the schema upgrade script, verified that the database was
correctly upgraded. Was also prompted to stop daemons.
- Ran 'repository-launch-master', verified that the discovery daemons were
able to discover new commits and insert events for them. Verified the
committask daemon was consuming events and converting them into tasks.
- Verified new tasks looked correct in the database.
- Browsed web interface.
Reviewers: jungejason
CC: tuomaspelkonen
Differential Revision: 133
Summary:
Created a script the runs all the necessary patches for db schema.
Stores information in the db about the latest patch that was applied.
Test Plan:
Created two test files '024.test.sql' and '023.test.sql' in this order.
'023' creates a database and '024' creates a table in this db. First ran
'./upgrade_schema.php 23' and made sure that patches were applied in order.
Then ran './upgrade_schema.php' to make sure db was up-to-date. Checked
manually from the db that the database and table exists.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, epriestley, tuomaspelkonen
Differential Revision: 115
Summary:
Make PhabricatorMetaMTADaemon extend PhabricatorDaemon.
Test Plan:
send mail with the new daemon.
Reviewed By: epriestley
Reviewers: epriestley
CC: epriestley
Differential Revision: 74
Summary: Amazon SES seems to be working well, except that it takes more than a
second to send mail in-process. Kick it out of process. (Between this and the
ImplementationAdapter layer, MetaMTA almost makes sense. :/)
Test Plan: Ran the daemon and got a flood of unsent test email.
Reviewers:
CC:
