Summary: this can happen if you have Phabricator and email lists co-mingling such that Phabricator receives an email multiple times. we can prevent this from then spamming everyone or otherwise taking the action multiple times by storing a message id hash and dropping the message if we have more than one message that matches.
Test Plan: simulated sending the same email multiple times on the command line. noted only the first one made it through.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1726
Differential Revision: https://secure.phabricator.com/D4328
Summary:
This replaces D4175 and makes it so phabot doesn't message anyone.
The reasons for this are twofold:
- It was possible to get information from the bot, by private messaging it, even
if the bot was only in a +i channel (on a public network) -- meaning that if
someone knew the nickname of the bot, they could obtain e.g. ticket names
or diff titles.
- The other time it messaged people was when you typed e.g. "somenick: T123".
Most times when this is triggered, it's done so on accident.
See discussion on the old revision (D4175).
Test Plan:
15:29:33 ::: Irssi: Starting query in quartz with cb-phabot
15:29:38 <relrod> T2
(nothing back)
and
15:29:21 <@relrod> rublets: T1
15:29:21 < cb-phabot> T1: asdfasdf (Priority: Needs Triage) - http://local.elrod.me/T1
Reviewers: epriestley, btrahan, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4339
Summary:
- Ports MySQL settings to PHP.
- Removes "mysql.retries" -- this existed only because Magic Numbers Are Bad, but there is no concievable reason it should ever be set to anything other than 3.
- Introduced "Hidden" config, which isn't visible from the web (for SaaS, we'll just mark anything with secret keys as "hidden").
- Introduced "Masked" config, which will be masked in darkconsole once that gets updated.
- "Hidden" implies "Masked" and "Locked".
- Moved "storage.default-namespace" here -- it probably makes more sense than core; this was my bad in T2255.
- Put cancel button back for hidden/locked config.
- Introduce 'class' config type.
Test Plan: Viewed MySQL options. None are editable.
Reviewers: codeblock, btrahan
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4326
Summary: these existed once, are no more, and don't get cleaned up in the current code path
Test Plan: storage destroy --dryrun -- noted the correct database names
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2237
Differential Revision: https://secure.phabricator.com/D4329
Summary: We interpret "size" as "size => true", and "true == 'full'", so we hit the wrong branch in the switch(). String cast explicitly.
Test Plan: Typed `{Fnnn, size}`; saw it render as a thumb instead of full.
Reviewers: btrahan, codeblock
Reviewed By: codeblock
CC: frozendevil, aran
Differential Revision: https://secure.phabricator.com/D4323
Summary:
- Move GC options into PHP.
- Remove the "run at" and "run for" options. The GC daemon doesn't actually do any table scans, is very gentle, and runs for like 3 seconds per day in any normal install. Just limit it to running once every 4 hours when it's caught up and call it a day.
Test Plan: Edited GC options.
Reviewers: btrahan, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4321
Summary: Use ApplicationTransactions in Config to create an edit history. Resolves T2256.
Test Plan: {F28477}
Reviewers: btrahan, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2256
Differential Revision: https://secure.phabricator.com/D4314
Summary: Show the value for all loaded configuration sources.
Test Plan:
{F28469}
{F28470}
{F28471}
Reviewers: btrahan, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4312
Summary:
See discussion in T2221. Before we can move configuration to the database, we have a bootstrapping problem: we need database credentials to live //somewhere// if we can't guess them (and we can only really guess localhost / root / no password).
Some options for this are:
- Have them live in ENV variables.
- These are often somewhat unfamiliar to users.
- Scripts would become a huge pain -- you'd have to dump a bunch of stuff into ENV.
- Some environments have limited ability to set ENV vars.
- SSH is also a pain.
- Have them live in a normal config file.
- This probably isn't really too awful, but:
- Since we deploy/upgrade with git, we can't currently let them edit a file which already exists, or their working copy will become dirty.
- So they have to copy or create a file, then edit it.
- The biggest issue I have with this is that it will be difficult to give specific, easily-followed directions from Setup. The instructions need to be like "Copy template.conf.php to real.conf.php, then edit these keys: x, y, z". This isn't as easy to follow as "run script Y".
- Have them live in an abnormal config file with script access (this diff).
- I think this is a little better than a normal config file, because we can tell users 'run phabricator/bin/config set mysql.user phabricator' and such, which is easier to follow than editing a config file.
I think this is only a marginal improvement over a normal config file and am open to arguments against this approach, but I think it will be a little easier for users to deal with than a normal config file. In most cases they should only need to store three values in this file -- db user/host/pass -- since once we have those we can bootstrap everything else. Normal config files also aren't going away for more advanced users, we're just offering a simple alternative for most users.
This also adds an ENVIRONMENT file as an alternative to PHABRICATOR_ENV. This is just a simple way to specify the environment if you don't have convenient access to env vars.
Test Plan: Ran `config set x y`, verified writes. Wrote to ENVIRONMENT, ran `PHABRICATOR_ENV= ./bin/repository`.
Reviewers: btrahan, vrana, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2221
Differential Revision: https://secure.phabricator.com/D4294
Summary:
This is somewhat clowny, particularly in how it handles JSON encode/decode, but
I've commented why I did things the way I did. The goal is to store minified JSON
but show pretty-printed JSON where possible, to the user editing it.
Test Plan:
* Went to /config/ and saw a list of keys from the `default` config.
* Clicked on one of them, submitted the default value successfully.
* Changed the value to invalid JSON and got a decent error.
* Changed the value to valid JSON and checked the DB to confirm it saved.
* Confirmed the DB values were minified.
* Confirmed the user-facing values were pretty-printed where they could be.
* Confirmed that PHIDs were getting assigned properly and that isDeleted
properly defaulted to false/0.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2246
Differential Revision: https://secure.phabricator.com/D4290
Summary: Currently, we have a configuration stack for unit tests, but they're built in to `PhabricatorEnv`. Pull them out and formalize them, so we can add more configuration sources (e.g., database).
Test Plan: Ran unit tests, web requests, scripts. This code had fairly good existing test coverage.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2223, T2221
Differential Revision: https://secure.phabricator.com/D4284
Summary:
We have a bunch of code duplication now between __init_script__.php and webroot/index.php. Consoldiate these methods and move them into PhabricatorEnv.
Merge PhabricatorRequestOverseer into PhabricatorStartup.
Test Plan: Loaded page, ran script. Wiped PHABRICATOR_ENV; loaded page, ran script; got errors.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2223
Differential Revision: https://secure.phabricator.com/D4283
Summary:
We have a lot of mess to get through before we can load libphutil and enter Phabricator code properly. Move it to a dedicated class.
I'm probably going to merge PhabricatorRequestOverseer into this, although the check that lives there now is kind of weird. It also does not really need to be a pre-load check and could be handled better.
I stopped shoving stuff in here once I got to ENV stuff, I'm going to tackle that next.
Test Plan: Ran phabricator normally; introduced fatals and misconfigurations. Grepped for changed symbols.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, asherkin
Maniphest Tasks: T2223
Differential Revision: https://secure.phabricator.com/D4282
Summary:
The search indexing API has several problems right now:
- Always runs in-process.
- It would be nice to push this into the task queue for performance. However, the API currently passses an object all the way through (and some indexers depend on preloaded object attributes), so it can't be dumped into the task queue at any stage since we can't serialize it.
- Being able to use the task queue will also make rebuilding indexes faster.
- Instead, make the API phid-oriented.
- No uniform indexing API.
- Each "Editor" currently calls SomeCustomIndexer::indexThing(). This won't work with AbstractTransactions. The API is also just weird.
- Instead, provide a uniform API.
- No uniform CLI.
- We have `scripts/search/reindex_everything.php`, but it doesn't actually index everything. Each new document type needs to be separately added to it, leading to stuff like D3839. Third-party applications can't provide indexers.
- Instead, let indexers expose documents for indexing.
- Not application-oriented.
- All the indexers live in search/ right now, which isn't the right organization in an application-orietned view of the world.
- Instead, move indexers to applications and load them with SymbolLoader.
Test Plan:
- `bin/search index`
- Indexed one revision, one task.
- Indexed `--type TASK`, `--type DREV`, etc., for all types.
- Indexed `--all`.
- Added the word "saboteur" to a revision, task, wiki page, and question and then searched for it.
- Creating users is a pain; searched for a user after indexing.
- Creating commits is a pain; searched for a commit after indexing.
- Mocks aren't currently loadable in the result view, so their indexing is moot.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: 20after4, aran
Maniphest Tasks: T1991, T2104
Differential Revision: https://secure.phabricator.com/D4261
Summary:
See discussion in D4204. Facebook currently has a 314MB remarkup cache with a 55MB index, which is slow to access. Under the theory that this is an index size/quality problem (the current index is on a potentially-384-byte field, with many keys sharing prefixes), provide a more general index with fancy new features:
- It implements PhutilKeyValueCache, so it can be a component in cache stacks and supports TTL.
- It has a 12-byte hash-based key.
- It automatically compresses large blocks of data (most of what we store is highly-compressible HTML).
Test Plan:
- Basics:
- Loaded /paste/, saw caches generate and save.
- Reloaded /paste/, saw the page hit cache.
- GC:
- Ran GC daemon, saw nothing.
- Set maximum lifetime to 1 second, ran GC daemon, saw it collect the entire cache.
- Deflate:
- Selected row formats from the database, saw a mixture of 'raw' and 'deflate' storage.
- Used profiler to verify that 'deflate' is fast (12 calls @ 220us on my paste list).
- Ran unit tests
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4259
Summary: Does this seem reasonable? It's a bit more compact than digest() (6 bits / byte instead of 4 bits / byte) and 72 bits of entropy @ 12 bytes instead of 128 bits of entropy @ 32 bytes. I feel like it's important to preserve the printability, though, and this seemed like a fairly good balance of concerns.
Test Plan: unit tests
Reviewers: vrana
Reviewed By: vrana
CC: aran, yemao932
Differential Revision: https://secure.phabricator.com/D4253
Summary:
- Build "sshd-auth" (for authentication) and "sshd-exec" (for command execution) binaries. These are callable by "sshd-vcs", located [[https://github.com/epriestley/sshd-vcs | in my account on GitHub]]. They are based on precursors [[https://github.com/epriestley/sshd-vcs-glue | here on GitHub]] which I deployed for TenXer about a year ago, so I have some confidence they at least basically work.
- The problem this solves is that normally every user would need an account on a machine to connect to it, and/or their public keys would all need to be listed in `~/.authorized_keys`. This is a big pain in most installs. Software like Gitosis/Gitolite solve this problem by giving you an easy way to add public keys to `~/.authorized_keys`, but this is pretty gross.
- Roughly, instead of looking in `~/.authorized_keys` when a user connects, the patched sshd instead runs `echo <public key> | sshd-auth`. The `sshd-auth` script looks up the public key and authorizes the matching user, if they exist. It also forces sshd to run `sshd-exec` instead of a normal shell.
- `sshd-exec` receives the authenticated user and any command which was passed to ssh (like `git receive-pack`) and can route them appropriately.
- Overall, this permits a single account to be set up on a server which all Phabricator users can connect to without any extra work, and which can safely execute commands and apply appropriate permissions, and disable users when they are disabled in Phabricator and all that stuff.
- Build out "sshd-exec" to do more thorough checks and setup, and delegate command execution to Workflows (they now exist, and did not when I originally built this stuff).
- Convert @btrahan's conduit API script into a workflow and slightly simplify it (ConduitCall did not exist at the time it was written).
The next steps here on the Repository side are to implement Workflows for Git, SVN and HG wire protocols. These will mostly just proxy the protocols, but also need to enforce permissions. So the approach will basically be:
- Implement workflows for stuff like `git receive-pack`.
- These workflows will implement enough of the underlying protocol to determine what resource the user is trying to access, and whether they want to read or write it.
- They'll then do a permissons check, and kick the user out if they don't have permission to do whatever they are trying to do.
- If the user does have permission, we just proxy the rest of the transaction.
Next steps on the Conduit side are more simple:
- Make ConduitClient understand "ssh://" URLs.
Test Plan: Ran `sshd-exec --phabricator-ssh-user epriestley conduit differential.query`, etc. This will get a more comprehensive test once I set up sshd-vcs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T550
Differential Revision: https://secure.phabricator.com/D4229
Summary: Some fallout from D4191.
Test Plan: Sent "D12" in IRC and got a response.
Reviewers: epriestley, vrana, zeeg
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4235
Summary:
Load the data for daemon worker tasks when viewing them, and present
the information in a useful way. This defaults to printing the json data,
but for some classes of worker it will also link to the corresponding
object, to make debugging problems with workers easier.
Test Plan:
load /daemon/task/NNN for a CommitParserWorker and a MetaMTAWorker, and
see the addition of a data field with useful content and link.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4226
Summary:
D4188 adds a preview of Paste contents to the list, but gets slow for large lists if you have Pygments installed since it has to spend ~200ms/item highlighting them. Instead, cache the highlighted output.
- Adds a Paste highlighting cache. This uses RemarkupCache because it has automatic GC and we don't have a more generalized cache for the moment.
- Uses the Paste cache on paste list and paste detail.
- Adds a little padding to the summary.
- Adds "..." if there's more content.
- Adds line count and language, if available. These are hidden on Mobile.
- Nothing actually uses needRawContent() but I left it there since it doesn't hurt anything and is used internally (I thought the detail view did, but it uses the file content directly, and must for security reasons).
Test Plan:
{F27710}
- Profiled paste list, saw good performance and few service calls.
- Viewed paste.
- Viewed raw paste content.
Reviewers: codeblock, btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4204
Summary: Include task ID and class when raising this exception. I took a brief stab at doing this generically, but (a) we specifically raise this exception outside of normal try/catch because we can't follow normal recovery rules for it and (b) we don't have a reasonable PhutilProxyException or similar right now which would preserve stack traces, and don't have builtin exception nesting support until PHP 5.3.
Test Plan: Faked this exception, verified we get more information in the logs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2193
Differential Revision: https://secure.phabricator.com/D4205
Summary:
When a user submits an action with no effect (like an empty comment, an "abandon" on an already-accepted revision, or a "close, resolved" on a closed task) we want to alert them that their action isn't effective. These warnings fall into two general buckets:
- User is submitting two or more actions, and some aren't effective but some are. Prompt them to apply the effective actions only.
- A special case of this is where the only effective action is a comment. We provide tailored text ("Post Comment") in this case.
- User is submitting one action, which isn't effective. Tell them they're out of luck.
- A special case of this is an empty comment. We provide tailored text in this case.
By default, the transaction editor throws when transactions have no effect. The caller can then deal with this, or use `PhabricatorApplicationTransactionNoEffectResponse` to provide a standard dialog that gives the user information as above. For cases where we expect transactions to have no effect (notably, "edit" forms) we just continue on no-effect unconditionally.
Also fix an issue where new, combined or filtered transactions would not be represented properly in the Ajax response (i.e., return final transactions from `applyTransactions()`), and translate some strings.
Test Plan:
- Submitted empty and nonempy comments in Macro and Pholio.
- Submitted comments with new and existing "@mentions".
- Submitted edits in both applications.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T912, T2104
Differential Revision: https://secure.phabricator.com/D4160
Summary: Currently, AphrontProxyResponse is expected to build a string. This prevents some response types (like Dialog) from being proxied, because they have special rules. Instead, make proxy responses reduce into a non-proxied response so it's possible to proxy any type of response and hit all the normal rules for it.
Test Plan: Built a proxied DialogResponse on top of this.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104, T912
Differential Revision: https://secure.phabricator.com/D4159
Summary:
Split Pholio's transaction implementation into generic and application-specific parts. Moves us toward generic transactions, with support for:
- Editing and deleting comments.
- Setting visibility of individual comments (I'm not a fan of this feature but we'll see).
I want to move everything to a more generic piece of infrastructure but there's very little they can share right now so adding transactions to, e.g., Paste or Macros (T2157) means massive amounts of similar code.
Tons of work left to do here, but I think it basically works. Here's a screenshot:
{F26820}
Test Plan: Made transactions in Pholio.
Reviewers: btrahan, vrana, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4136
Summary:
- Remove unused CSS rules and non-sprite images.
- Sprite the logo.
Test Plan: Looked at site, looked good.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4113
Summary:
Issues here:
- Need an application-sized "eye", or a "home" icon for "Phabricator Home".
- Some of the "apps_lb_2x" sliced images are the "_dark_" versions, not the light versions.
- If you slice an application-sized "logout" (power off) icon and application-sized "help" (questionmark in circle) icon I can replace the current menu icons and nearly get rid of "autosprite".
- To replace the icons on /applications/, the non-retina size is "4x", so we'd need "8x" for retina. Alternatively I can reduce the icon sizes by 50%.
- The "Help", "Settings" and "Logout" items currently have a "glowing" hover state, which needs a variant (or we can drop it).
- The /applications/ icons have a white hover state (or we can drop it).
- The 1x application (14x14) icons aren't used anywhere right now, should they be? Maybe in the feed in the future, etc?
- The "apps-2x" and "apps-large" sheets are the same image, but getting them to actually use the same file is a bit tricky, so I just left them separate for now.
Test Plan:
{F26698}
{F26699}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4108
Summary: Do we have an icon with 2x for the right menu?
Test Plan: {F26590}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4101
Summary:
This doesn't lay in everything, but:
- Break the buttons gradient apart into components and rebuild it (along with other gradients) into a single gradient sprite (possible after {D4099}).
- Use the sliced gradient for the crumbs background.
- Use the sliced image for the crumb divider.
- Adds the black/white app sheets, but I'm not generating them quite yet.
Test Plan: {F26537} {F26540}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4100
Summary: Switch to the final versions of these
Test Plan: Will add screenshots...
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4032
Summary:
- The filesystem is now the authority for which sprites are available. If you add new icons, the generation process will pick them up.
- I broke out icon generation and added retina support. App icon generation still uses the old method.
- Update ActionList and RemarkupControl to use the new sheet.
- Use white icons on hover.
- Also fixed a couple of minor issues with some stuff in Firefox/Chrome.
Test Plan:
{F25750}
{F25751}
{F25752}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2013
Differential Revision: https://secure.phabricator.com/D4027
Summary:
When users middle click or command-click an image, we should open it in a new tab, not open a lightbox.
See https://github.com/facebook/phabricator/issues/234
Test Plan: Left, middle, and command-clicked a lightbox image.
Reviewers: vrana, chad, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4020
Summary:
Just laying more groundwork out of ready-made UI.
{F22349}
Test Plan: Looked at a mock.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2097
Differential Revision: https://secure.phabricator.com/D3832
Summary:
I'm not going to land this until it's a bit more fleshed out since it would just confuse users, but this is probably more reviewable as a few diffs adding a couple features than one ULTRA-diff adding everything. Implement application basics for Pholio. This does more or less nothing, but adds storage, subscribe, flag, markup, indexing, query basics, PHIDs, handle loads, a couple of realy really basic controllers, etc.
Basic hierarchy is:
- **Moleskine**: Top-level object like a Differential Revision, like "Ponder Feed Ideas".
- **Image**: Each Moleskine has one or more images, like the unexpanded / expanded / mobile / empty states of feed.
- **Transaction**: Comment or edit, like Maniphest. I generally want to move most apps to a transaction model so we can log edits.
- **PixelComment**: Equivalent of an inline comment.
Test Plan: Created a fake object and viewed it.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran, davidreuss
Maniphest Tasks: T2097
Differential Revision: https://secure.phabricator.com/D3817
Summary: Companion diff to D4003.
Test Plan: Rigged up a response which loaded a CSS file with `body { background: red; }`, verified page turned red after I clicked the button.
Reviewers: vrana, chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T2086
Differential Revision: https://secure.phabricator.com/D4004
Summary:
People hit three issues with D3914:
- As per T2059, we applied a schema change from a `.php` patch, which currently does not work if you use a different user to make schema changes than for normal use.
- Since the change in question is idempotent, just move it to a `.sql` patch. We'll follow up in T2059 and fix it properly.
- Rogue daemons at several installs used old code (expecting autoincrement) to insert into the new table (no autoincrement), thereby creating tasks with ID 0.
- Rename the table so they'll fail.
- This also makes the code a little more consistent.
- Some installs now have tasks with ID 0.
- Use checks against null rather than against 0 so we can process these tasks.
The major issues this fixes are the schema upgrade failure in T2059, and the infinite loops in T2072 and elsewhere.
This isn't really a fully statisfactory fix. I'll discuss some next steps in T2072.
Test Plan: Created new tasks via MetaMTA/Differential. Ran tasks with `phd debug taskmaster`. Inserted a task 0 and verified it ran and archived correctly.
Reviewers: btrahan, vrana, nh
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2072, T2059
Differential Revision: https://secure.phabricator.com/D3973
Test Plan:
/diffusion/ARC/lint/master/src/, clicked on count link.
/diffusion/ARC/browse/master/src/difference/?lint=XHP9, clicked on file name.
/diffusion/ARC/browse/master/src/difference/ArcanistDiffUtils.php?lint=, verified that all messages are displayed.
/diffusion/ARC/browse/master/src/difference/ArcanistDiffUtils.php?lint=XHP9.
/diffusion/ARC/browse/master/src/difference/ArcanistDiffUtils.php?lint=TXT3, verified that 0 messages are displayed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3929
Summary: This saves lint errors to the path change of current commit. It requires pushed revision. It doesn't save difference from previous commit mentioned in T2038#comment-4 - I don't plan doing it after all, everything would be much more complicated and the amount of data saved with this approach isn't that bad.
Test Plan: Applied patch, ran script, verified DB.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2038
Differential Revision: https://secure.phabricator.com/D3899
Summary: don't need it now that uploading files is so easy. Plus it made for some buggy jonx if / when there were bad image links coupled with caching. In theory this is a lot less pretty though if folks linked to a bunch of files served elsewhere using images.
Test Plan: http://does-not-exist.com/imaginary.jpg rendered as a link!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2000
Differential Revision: https://secure.phabricator.com/D3908
Summary: See D3912 for discussion. InnoDB may reuse autoincrement IDs after restart; provide a way to avoid it.
Test Plan: Unit tests. Scheduled and executed tasks through `drydock lease --type host` and `phd debug taskmaster`.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3914
Summary:
- Remove EC2, RemoteHost, Application, etc., blueprints for now. They're very proof-of-concept and Blueprints are getting API changes I don't want to bother propagating for now. Leave the abstract base class and the LocalHost blueprint. I'll restore the more complicated ones once better foundations are in place.
- Remove the Allocate controller from the web UI. The original vision here was that you'd manually allocate resources in some cases, but it no longer makes sense to do so as all allocations come from leases now. This simplifies allocations and makes the rule for when we can clean up resources clear-cut (if a resource has no more active leases, it can be cleaned up). Instead, we'll build resources like the localhost and remote hosts lazily, when leases come in for them.
- Add some configuration to manage the localhost blueprint.
- Refactor `canAllocateResources()` into `isEnabled()` (for config checks) and `canAllocateMoreResources()` (for quota checks, e.g. too many resources are allocated already).
- Juggle some signatures to align better with a world where blueprints generally do allocate.
- Add some more logging and error handling.
- Fix an issue with log ordering.
Test Plan: Allocated some localhost leases.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3902
Summary:
use PhutilURI class to get the slug and the fragment, normalize the slug, and then glue it back together with another PhutilURI
fixes https://github.com/facebook/phabricator/issues/228
Test Plan: had a few links like [[ example/doc#title | wiki fun ]] and verified links generated were correct
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3894
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in LICENSE file and that it didn't modify externals.
Reviewers: epriestley, davidrecordon
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3886
Summary:
Tightens up a bunch of stuff:
- In `drydock lease`, pull and print logs so the user can see what's happening.
- Remove `DrydockAllocator`, which was a dumb class that did nothing. Move the tiny amount of logic it held directly to `DrydockLease`.
- Move `resourceType` from worker task metadata directly to `DrydockLease`. Other things (like the web UI) can be more informative with this information available.
- Pass leases to `allocateResource()`. We always allocate in response to a lease activation request, and the lease often has vital information. This also allows us to associate logs with leases correctly.
Test Plan: Ran `drydock lease --type host` and saw it perform a host allocation in EC2.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3870
Summary:
We lock tasks by setting `leaseOwner` to a unique value, but the value is currently unique-to-the-process rather than unique-to-the-query. This means that if a process leases a task, then leases another task, both tasks will have the same `leaseOwner`. This can cause an issue where we go to select the task we just leased and get the other task instead, if we aren't careful about the select construction.
We can avoid this by being clever and making sure the select is constructed correctly, but making the `leaseOwner` unique to the query is much simpler and more foolproof. This guarantees we always select only the rows we just leased.
Also remove `PhabricatorGoodForNothingWorker` since `PhabricatorTestWorker` fills its role of allowing things to be tested, and simplify the unit tests since we don't need to be clever about avoiding this issue any more.
Test Plan: Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3862
Summary:
This was the major goal of D3859/D3855, and to a lesser degree D3854/D3852.
As Drydock is allocating a resource, it may need to allocate other resources first. For example, if it's allocating a working copy, it may need to allocate a host first.
Currently, we have the process basically queue up the allocation (insert a task into the queue) and sleep() until it finishes. This is problematic for a bunch of reasons, but the major one is that if allocation takes more resources (host, port, machine, DNS) than you have daemons, they could all end up sleeping and waiting for some other daemon to do their work. This is really stupid. Even if you only take up some of them, you're spending slots sleeping when you could be doing useful work.
To partially get around this and make the CLI experience less dumb, there's this goofy `synchronous` flag that gets passed around everywhere and pushes the workflow through a pile of special cases. Basically the `synchronous` flag causes us to do everything in-process. But this is dumb too because we'd rather do things in parallel if we can, and we have to have a lot of special case code to make it work at all.
Get rid of all of this. Instead of sleep()ing, try to work on the tasks that need to be worked on. If another daemon grabbed them already that's fine, but in the worst case we just gracefully degrade and do everything in process. So we get the best of both worlds: if we have parallelizable tasks and free daemons, things will execute in parallel. If we have nonparallelizable tasks or no free daemons, things will execute in process.
Test Plan: Ran `drydock_control.php --trace` and saw it perform cascading allocations without sleeping or special casing.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3861
Summary:
- Clean up a TODO about permanent failures.
- Clean up a TODO about failing tasks after too many retries.
- Clean up a TODO about testing for bad leases.
- Make the lease/retry implementation more flexible and natural.
- Make completely bogus tasks fail permanently.
- Make PhabricatorMetaMTAWorker use new `getWaitBeforeRetry()` (as intended), not hackily implement logic in `getRequiredLeaseTime()`.
- Document worker hooks for failures and retries.
- Provide coverage on everything.
Test Plan: Ran unit tests. Ran `bin/phd debug taskmaster`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3859
Summary: This simplifies the fairly thorny logic of leasing tasks a bit. I'm planning to introduce another callsite shortly for Drydock.
Test Plan: Ran `bin/phd debug taskmaster`, observed sensible queries and correct operation.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3855
Summary:
When a PhabricatorTestCase dies after creating storage fixtures, it leaves
those storage fixtures around. This doesn't happen often, but when it does
happen it's a pain to cleanup. The --unittest-fixtures option helps automate
that cleanup.
Test Plan:
Ran it with --dryrun, then for real. Became overwhelmed with a Zen like peace
after regarding the tidiness and beauty of SHOW DATABASES.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3858
Summary: Make mobile-friendly and provide UI to cancel/retry tasks. Remove display of task data to arbitrary users, as it may be sensitive.
Test Plan:
{F22502}
{F22503}
{F22504}
{F22505}
{F22506}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3854
Summary:
Currently, when taskmasters complete a task it is immediately deleted. This prevents us from doing some general things, like:
- Supporting the idea of permanent failure (e.g., after N failures just stop trying).
- Showing the user how fast taskmasters are completing tasks.
- Showing the user how long tasks took to complete.
Having better visibility into this is important to Drydock, which builds on the task system. Also, generally buff debug output for task execution.
Test Plan: Ran `bin/phd debug taskmaster`. Ran `bin/phd debug garbage`. Queued some tasks via various systems.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3852
Test Plan: Looked at diff with several different lint errors, saw correct messages in their inline comments.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3827
Summary: you can now add, edit, and delete status events. also added a "description" to status events and surface it in the big calendar view on mouse hover. some refactoring changes as well to make validation logic centralized within the storage class.
Test Plan: added, edited, deleted. yay.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T407
Differential Revision: https://secure.phabricator.com/D3810
Summary: Followup to D3804. Makes Diffusion main comments (not just inlines) render properly with the modern markup pipeline.
Test Plan: Created previews and inline previews. Edited inlines. Saved comment, viewed comment. Verified caches were read and written using "Services" tab.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3805
Summary:
See T1963 for discussion of the Facebook-specific hack.
Differential currently uses a one-stage cache (render -> postprocess -> save in cache) rather than the two-stage cache (render -> save in cache -> postprocess) offered by `PhabricatorMarkupInteface`. This breaks Differential comments coming out of cache for the lightbox, and makes various other things suboptimal (status of handles like @mentions and embeds are not displayed accurately).
Instead, use the modern stuff.
Test Plan:
- Created preview comments and inlines in Differential.
- Edited a Differential inline.
- Submitted main and inline Differential comments.
- Viewed and edited Differential summary and test plan.
- Created preview comments and inlines in Diffusion.
- Submitted comments and inlines in Diffusion.
- Verified Differential now loads and saves to the generalized markup cache (Diffusion is close, but main comments still hold a single-stage cache).
- Verified old Differential comments work correctly with the lightbox.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1963
Differential Revision: https://secure.phabricator.com/D3804
Summary:
Provides a simple way for policy-aware queries to pre-filter results without needing to maintain separate cursors, and fixes a bunch of filter-related edge cases.
- For reverse-paged cursor queries, we previously reversed each individual set of results. If the final result set is built out of multiple pages, it's in the wrong order overall, with each page in the correct order in sequence. Instead, reverse everything at the end. This also simplifies construction of queries.
- `AphrontCursorPagerView` would always render a "<< First" link when paging backward, even if we were on the first page of results.
- Add a filtering hook to let queries perform in-application pre-policy filtering as simply as possible (i.e., without maintaing their own cursors over the result sets).
Test Plan: Made feed randomly prefilter half the results, and paged forward and backward. Observed correct result ordering, pagination, and next/previous links.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3787
Summary:
images attached to maniphest tasks and mentioned in remarkup anywhere now invoke a lightbox control that lets the user page through all the images.
lightbox includes a download button, next / prev buttons, and if we're not at the tippy toppy of hte page an "X" or close button.
we also respond to left, right, and esc for navigating.
next time we should get non-images working in here...!
Test Plan:
played with maniphest - looks good
made comments with images. looks good.
made sure multiple image comments worked.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, chad
Maniphest Tasks: T1896
Differential Revision: https://secure.phabricator.com/D3705
Summary: The property is called 'actor', not 'user'. Extend from Phobject to catch this class of error automatically. Upgrade a couple of getActor() to requireActor().
Test Plan: Created new users.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3776
Summary: I need this shortly and it seems like something we're likely to need more of in the future now that fixtures work.
Test Plan: Ran unit tests. Used this productively in an upcoming diff.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3775
Summary:
Q1 to Q4 is used for parts of the year.
Also unlink all `[A-Z]0.*`, we don't route them anymore.
Test Plan:
Q0
Q1
Q10
Reviewers: epriestley, pieter
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3781
Summary: See comments.
Test Plan: Uploaded a small image in Safari via drag-and-drop.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3771
Summary:
We don't support this and say so in the documentation, but can check explicitly.
https://github.com/facebook/phabricator/issues/148
Test Plan: Set base-uri to stuff with/without paths, verified setup caught mistakes and gave useful errors.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3768
Summary:
See comment.
This can reveal some pretty bad bugs but HPHP handles this correctly so we already know about them.
Test Plan:
Added `phlog()` to `__call()` and observed what is defined for each method (under PHP). Also:
class C {
function __call($name, $args) {
static $class;
if (!$class) {
$class = get_class($this);
}
return $class;
}
}
class D extends C {
}
class E extends C {
}
$d = new D;
$e = new E;
var_dump($d->x());
var_dump($e->x()); // Prints D under PHP!
See also D3754.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1261
Differential Revision: https://secure.phabricator.com/D3753
Summary:
Allow skins to serve arbitrary resources without needing to be mapped, so we can have a vibrant community of amateur skinners.
For "basic" skins, just put all the "css/" on the page always.
Includes an image to prove that works.
@vrana, pretty sure this has no impact outside of Phame but it does change Celerity so it might be to blame if there's any weirdness with static resources.
Test Plan:
{F21341}
{F21340}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3719
Summary: Restore summarization. Use the remarkup cache, and try to do it somewhat-intelligently (pick the first paragraph that looks like it's text).
Test Plan:
{F21323}
{F21324}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3715
Summary:
Two high-level things happening here:
- We no longer ever need to put meta-UI (content creation, editing, notices, etc.) on live blog views, since this is all in Phame now. I pulled this out.
- On the other hand, I pushed more routing/control logic into Skins and made the root skin a Controller instead of a View. This simplifies some of the code above skins, and the theory behind this is that it gives us greater flexibility to, e.g., put a glue layer between Phame and Wordpress templates or whatever else, and allows skins to handle routing and thus add pages like "About" or "Bio".
- I added a basic skin below the root skin which is more like the old root skin and has standard rendering hooks.
- "Ten Eleven" is a play on the popular (default?) Wordpress themes called "Twenty Ten", "Twenty Eleven" and "Twenty Twelve".
Test Plan: Viewed live blog and live posts. They aren't pretty, but they don't have extraneous resources.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3714
Summary: See discussion in D3722. Good catch -- I'd definitely say this is a bug.
Test Plan: Rendered two <div />s, one with mustcapture set to `true` and one to `false`. The first got `data-mustcapture`; the second did not.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3723
Summary: I think this is simpler and better than making them conditional. In properly configured installs this should have no impact (they already use a CDN URI). In not-quite-properly configured installs this will add a trivial, highly-compressible number of bytes to the source. In all cases we have less code.
Test Plan: Loaded some pages, everything worked.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3709
Summary:
This leaves the UI in a pretty rough state, but implements blog policy controls and queries, and 1:1 relationships between posts and blogs. Needs a bunch more cleanup but seemed like an okayish breaking point in terms of cohesiveness.
Posts have these rules:
- Drafts are visible only to the author.
- Published posts are visible to anyone who can see the blog they appear on.
- Posts are only editable by the author.
...so we don't need any special policy UI or state to accommodate these rules.
Posts may have no blog if they're grandfathered in or you write a post to a blog and then lose the ability to see the blog. This is the messiest edge case -- specifically:
- You write a post to blog A.
- You publish the post.
- I edit the "Visible To:" for blog A and set it to exclude you.
What we do in this case is let you see the post in "My Posts", but you can no longer see the blog and you'll see the post as not being part of a blog. We can maybe give you some UI to let you move it later or something.
Test Plan: Hit all (I think?) of the interfaces without issues. Definitely some UI problems still right now.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3694
Summary:
Adds "can view" and "can edit" policies to blogs. Replaces "bloggers" with "can join".
This doesn't fully remove "bloggers" because I didn't want this to get too crazy/huge.
Test Plan: Created, edited, deleted blogs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3693
Summary: Provide array() default so we don't foreach() over null in the case of a missing config (from @dctrwatson).
Test Plan: Will verify with @dctrwatson.
Reviewers: vrana, btrahan, dctrwatson
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3686
Summary: When we receive an email, figure out if any of the other tos and ccs are users. If they are, pass their phids through the stach as "exclude phids" and exclude them from getting the email.
Test Plan: used the various applications (audit, differential, maniphest) and noted emails were sent as expected.
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin, vrana
Maniphest Tasks: T1676
Differential Revision: https://secure.phabricator.com/D3645
Summary:
Blames to D3659.
It's not that old (< 1 day) to fix it properly (add patch for dropping xhpast DB).
Test Plan:
$ arc unit src/applications/calendar/storage/__tests__/PhabricatorCalendarHolidayTestCase.php
Reviewers: btrahan, epriestley
Reviewed By: btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3670
Summary: ...and use 'em in the phame blog case.
Test Plan: viewed blog.phabricator.dev and it actually looked right!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3666
Summary:
We need to go slightly farther to stub reply handler functionality for Ponder in at least some configurations, where we rely on the presence of a unique random key to generate per-object or per-object+user reply addresses.
This should probably be formalized in an interface since it's currently pretty ad-hoc.
Test Plan:
- Made comments in Ponder under a per-user email configuration.
- Ran migration, verified mail keys were generated.
- Ran migration again (with --apply), verified existing questions were skipped.
- Created a new question, verified mail key generation.
Reviewers: pieter
Reviewed By: pieter
CC: aran
Maniphest Tasks: T1873
Differential Revision: https://secure.phabricator.com/D3665
Summary:
It isn't deleted by `storage destroy`.
This should be a no-op on current storage because we execute `CREATE DATABASE IF NOT EXISTS`.
Test Plan:
$ bin/storage destroy --dryrun
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3659
Summary: For immutable objects, just use the ID as a cursor.
Test Plan:
- Analyzed commits from an empty cursor.
- Checked that cursor was good.
- Pulled some more commits.
- Analyzed commits again, verified it only hit the new ones.
- Verified the graph of "Count of CMIT" looked reasonable.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1866
Differential Revision: https://secure.phabricator.com/D3656
Summary:
Alternate proposal for D3635.
- Works better with small images.
- Produces a predictable thumbnail size.
- Somewhat reasonable output on 3000x10 images.
- Increase the size of Macro thumbnails to 240px.
Test Plan: {F20497}
Reviewers: vrana, chad
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3638
Summary:
Basic infrastructure for generalizing subscriptions/CCs for T1808, T1514 and T1663.
- Implement `PhabricatorSubscribableInterface` and you'll get a subscribe/unsubscribe button for free.
- If there are any auto-subscribed users (like the question author) you can specify them; this makes more sense for Tasks and Revisions than Ponder probably, but maybe the author should be auto-subscribed.
- Subscriptions are either "explicit" (the user clicked 'subscribe') or "implicit" (the user did something which causes them to become subscribed naturally). If a user unsubscribes, they'll no longer be added by implicit subscriptions. This may or may not be relevant to Ponder but is an existing Herald feature in Differential.
- Helper method on PhabricatorSubscribersQuery to load subscribers.
- This doesn't handle actually sending email, etc. I think that's all so application-specific that it doesn't belong here.
- Now seems to work.
Test Plan:
{F20552}
{F20553}
Reviewers: pieter, btrahan
Reviewed By: pieter
CC: aran
Maniphest Tasks: T1663, T1514, T1808
Differential Revision: https://secure.phabricator.com/D3637
Summary: We never use this and almost certainly never will. It's been in Lisk for ~7 years but is a solution in search of a problem. It causes a conflict with any DAO that has a `version` column.
Test Plan: Browsed around, performed inserts and updates. Edited a Phriction document.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, leslie.chong
Differential Revision: https://secure.phabricator.com/D3625
Summary:
Fixes a TODO, and silences a warning introduced by D3601.
There are several cases where we load data like:
SELECT *, ... AS extraData FROM ...
...and then pass it to `loadAllFromArray()`. Currently, this causes us to set an `extraData` property on the object.
This idiom seems fairly useful and non-dangerous, so I made `loadFromArray()` just drop extra keys.
Since we hit this loop a potentially huge number of times (10,000+ for full Maniphest pages) I did some microoptimization. Lisk is hot enough that it's one of the few places where it's worthwhile (see D1291).
Test Plan: Loaded homepage, no longer got warnings about `viewerIsMember` from Project queries. Browsed ~10 apps, didn't see any issues.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3606
Summary:
I make this error quite often: I forget to declare a property I am writing to or I make a typo in it.
PHP implicitly creates a public property which I don't like.
I would much rather see a linter warning me against this than this runtime check but writing it is very difficult:
- We need to explore all parents of the class we are checking.
- It is even possible that children will declare that property but it's OK to treat this as error anyway.
- We can extend also builtin or external classes.
- It's somewhat doable for `$this` but even more complex for any `$obj` because we don't know the class of it.
This should catch significant part of these errors and I'm fine with that.
I don't plan escalating to exception because this error is not fatal and should not stop the application from working.
Test Plan: Loaded homepage, checked log.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3601
Summary:
Calling `->setPHID()` or other common Lisk setters creates an implicit public property `$phid`.
I don't like implicit properties and I see them as errors.
Its public visibility also makes me nervous and is vulnerable to bypassing any setters we may create.
Test Plan: Loaded homepage, checked log.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3600
Summary: this then enables people to create blog.theircompany.com. And for us, blog.phacility.com...!
Test Plan:
- created custom URIs of various goodness and verified the error messages were sensical.
- verified if "false" in configuration then custom uri stuff disappears
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3542
Summary: We want to allow a broader access to our installation but we need to check the request in that case.
Test Plan:
Created a simple `PhabricatorRequestChecker` returning a custom controller.
Verified that this controller is used when accessing any page.
Returned `null` from this checker and verified that all 209 Phabricator pages are accessible.
Reviewers: epriestley
Reviewed By: epriestley
CC: scottmac, aran, Korvin, btrahan
Differential Revision: https://secure.phabricator.com/D2488
Summary:
It happens to me quite often that I leave the window with revision (by closing it or by visiting a link from it).
When I return then the comment draft is there so I clowncopterize it but forget that I wanted to take some other action than Comment.
Test Plan: Selected "Add Reviewers", added some reviewers, closed the window, opened it - the action and reviewers were still there.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3530
Summary:
We have two troubles with this script:
# Our revisions and commits don't fit in the memory. (Our tasks do :-).)
# Reindexing revisions is slow.
Test Plan: Ran it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3483
Summary:
People have occasionally complained about phabricator being slow. We have
the access log to look at to see when slowness happens, but it doesn't tell
us much about why it happened. Since it's usually a sporadic issue that's
reported, it's hard to reproduce and then profile. This change will allow us
to collect sampled profiles so we can look at them when slowness occurs.
Test Plan:
checking that sampling works correctly:
- set rate to 0; do several page loads; check no new entries in table
- set rate to 1; check that there's a new row in the table for each page load
- set rate to 10; check that some requests write to table and some don't
check new ui for samples:
- load /xhprof/list/all/, see a list with a lot of samples
- load /xhprof/list/sampled/, see only sampled runs
- load /xhprof/list/manual/, see only non-sampled runs
- load /xhprof/list/my-runs/, se only my manual runs
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3458
Summary:
- Renames `PhabricatorPolicyQuery` to `PhabricatorPolicyAwareQuery` (a query which respects policy settings).
- Introduces `PhabricatorPolicyQuery`, which loads available policies (e.g., "member of project X").
- Introduces `PhabricatorPolicy`, which describes a policy.
- Allows projects to be set as policies.
- Allows Paste policies to be edited.
- Covers crazy cases where you make projects depend on themselves or each other because you are a dastardly villan.
Test Plan: Set paste and project policies, including crazy policies like A -> B -> A, A -> A, etc.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3476
Summary: Add storage to Pastes for view policies.
Test Plan: Set policies on pastes, see next diff.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3474
Summary: This is pretty spartan, but it does the job.
Test Plan:
Patch, update storage, add some comment
to your favorite question or answer.
Reviewers: nh, vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin, starruler, syrneus, me.here, victorzarate7
Maniphest Tasks: T1645
Differential Revision: https://secure.phabricator.com/D3471
Summary:
I want to implement a `{P123}` rule to embed pastes, but we need viewers everywhere before it will work with privacy.
This is not exhaustive; many Remarkup callsites haven't been converted to `PhabricatorMarkupInterface` yet.
Test Plan: Looked at Maniphest, Differential, Diffusion, Phriction; added markup, made edits and hit previews.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3428
Summary:
I've replaced all `id(new PhabricatorObjectHandleData(...))->loadHandles()` by `$this->loadViewerHandles(...)`.
Lint caught one usage in a static method.
Test Plan: Displayed revision with sporadic author.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3432
Summary: Previously, the identification string was thrown at the server long before you were connected, I've moved this to the end of the motd raw, and now errthangz gud
Test Plan: Register an account for your bot to use, give your bot the correct nick and password, then watch
Reviewers: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D3410
Summary:
Currently, CelerityController extends AphrontController, not PhabricatorController. (I think I imagined Celerity being somewhat stand-alone and didn't want to create a dependency.)
This creates a concrete problem if a static resource is missing, since we throw an exception, but the higher-level exception handlers depend on the User existing in order to show an appropriate response page. This is the only controller which doesn't extend PhabricatorController, and it doesn't seem worthwhile to make a weird edge case out of it.
Specific repro case is:
- Remove `externals/javelin/` (or forget to run `git submodule update --init`).
- Load a static resource.
- Get "[Rendering Exception] Argument 1 passed to PhabricatorMainMenuView::setUser() must be an instance of PhabricatorUser, null given, called in /services/apache/phabricator/phabricator/src/view/page/PhabricatorStandardPageView.php on line 435 and defined"
Test Plan:
- Followed above steps, no more fataling.
- Verified this is the only weird controller.
Reviewers: voldern, vrana, btrahan
Reviewed By: voldern
CC: aran
Differential Revision: https://secure.phabricator.com/D3389
Summary:
I like systems that just work. It is possible to store files larger than max_allowed_packet in MySQL and we shouldn't demand it.
It also fixes a problem when file was smaller than `storage.mysql-engine.max-size` but its escaped version was larger than `max_allowed_packet`.
Test Plan: Reduced the size to 5e4, uploaded 90 kB file, checked the queries in DarkConsole, downloaded the file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3392
Summary: In my haste, I forgot a trailing ?
Test Plan: Try both "Where is Derp?" and "Where in the world is Derp?"
Reviewers: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D3387
Summary:
This does a few things:
- Allows you to flag pastes. This is straightforward.
- Allows Applications to register event listeners.
- Makes object action lists emit a 'didrenderactions' event, so other applications can add more actions. The Flags application injects its action in this way. This should generally make it much easier to add actions to objects when we add new applications, with less code duplication and better modularity. We have a really hacky version of this in Differential that I want to get rid of in lieu of this more general approach. I'm going to make object lists do the same thing, so any application can jump in and add stuff.
Test Plan: Flagged and unflagged pastes. Viewed home page, differential, flags list.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3377
Summary:
More and more relations are going under edges and I can't work with them from Relatives framework.
This doesn't have the nice transitive property of normal relatives (loading relative objects from relatives loads all of them at once) but I can add it when I need it.
I plan to use it in D3085 (after converting relationships to edges).
Test Plan:
$task = id(new ManiphestTask())
->loadOneWhere('phid = %s', $phid);
print_r($task->loadRelativeEdges(4));
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3344
Summary:
Currently, when a user runs "arc diff" and the diff exceeds PHP's 'post_max_size', they get a very confusing and irrelevant error about a missing Conduit session token. The reason for this is that 'post_max_size' doesn't build $_POST, so //all// the data is missing.
We try to detect this, but currently only do so effectively for specific file upload forms. Broaden the detection to cover all cases.
Previously, we ran into an issue where Firefox + HTML5 drag-and-drop uploads would get a false positive on this detection. I dug into this and added the Content-Type checks, which correctly handle that case.
Test Plan: With small and large 'post_max_size', ran small and large normal, HTML5 and multipart/form-data POST requests against Phabricator in Safari and Firefox. Got desired beahviors.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: tido, aran
Differential Revision: https://secure.phabricator.com/D3320
Summary: We use numbers here and I see no reason for strings.
Test Plan:
$ bin/storage upgrade
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3303
Summary: Added match to the novel statement: Where in the world is derp?
Test Plan: Say something like "Where in the world is CarmenSandiego?"
Reviewers: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D3318
Summary:
See D3126, T1667, T1658. Prior to D3126, `phd` did not use MySQL directly. Now that it does, there are at least two specific problems (see inline comment).
In the long term, we should probably break this dependency and use Conduit. However, we don't currently have access to the daemon log ID and getting it is a mess (the overseer generates it), and I think I want to rewrite how all this works at some point anyway (the daemon calls are currently completely unauthenticated, which is silly -- we should move them to an authenticated channel at some point, I think).
Test Plan: Ran `phd stop` with a bad MySQL config against a non-running daemon, didn't get a query error.
Reviewers: nh, vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1667, T1658
Differential Revision: https://secure.phabricator.com/D3314
Summary: We need to open the envelope here.
Test Plan: Ran `bin/storage dump` without errors.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3315
Summary:
You can now embed countdowns in Remarkup! Not sure what it's
useful for, but there you have it.
Also I may have made a hash of the markup code; I don't really know what
I'm doing.
Test Plan: Make a new countdown, put `{C###}` in a Differential comment.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1053
Differential Revision: https://secure.phabricator.com/D3290
Summary: This is arguably a more useful view than listing all daemons.
Test Plan: Looked at list, only saw daemons that haven't exited
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3286
Summary:
Fact engines loading dependent objects are super slow because they load them one by one.
This diff put each page in a Lisk set allowing engines to use `loadRelatives()`.
It also introduces `clearSet()` method which is somewhat neccessary in PHP < 5.3 or with disabled cyclic [[ http://php.net/gc | GC ]].
Test Plan:
$iterator = new PhabricatorFactUpdateIterator(new DifferentialRevision());
foreach ($iterator as $revision) {
$diffs = $revision->loadRelatives(new DifferentialDiff(), 'revisionID');
echo memory_get_usage() . "\n";
}
Experienced not-steadily-increasing memory usage and much faster loading.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3247
Summary:
Currently, we're showing projets in reverse order (Z..A) because most cursor pagers go from high IDs to low IDs.
Allow sequence to be reversed; reverse it.
Also simplify some query/paging stuff.
Test Plan: Set page size to 1, paged back and forth.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3221
Summary:
- In ProjectQuery, always load the viewer's membership in the project because we need it to perform a CAN_VIEW test.
- Add storage for the view, edit and join policies.
- A user can always view a project if they are a member.
- A user can always join a project if they can edit it.
- Editing a project requires both "view" and "edit" permissions, and edit does not imply view.
- This has no effect on the application yet.
Test Plan: See next diff.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3219
Summary:
- Allow PolicyQuery to require specific sets of capabilities other than "CAN_VIEW", like edit, etc. The default set is "view".
- Add some convenience methods to PolicyFilter to test for capabilities.
Test Plan: Viewed pastes, projects, etc. Used other stuff in future diff.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3212
Summary:
We currently cache all connections in LiskDAO so we can roll back transactions when fixtured unit tests complete.
Since we establish a new connection wrapper each time we establish a global lock, this cache currently grows without bound.
Instead, pool global lock connections so we never have more than the largest number of locks we've held open at once (in PullLocalDaemon, always 1).
Another way to solve this is probably to add an "onclose" callback to `AphrontDatabaseConnection` so that it can notify any caches that it been closed. However, we currently allow a connection to be later reopened (which seeems reasonable) so we'd need a callback for that too. This is much simpler, and this use case is unusual, so I'd like to wait for more use cases before pursing a more complicated fix.
Test Plan:
Ran this in a loop:
while (true) {
for ($ii = 0; $ii < 100; $ii++) {
$lock = PhabricatorGlobalLock::newLock('derp');
$lock->lock();
$lock->unlock();
}
$this->sleep(1);
}
Previously it leaked ~100KB/sec, now has stable memory usage.
Reviewers: vrana, nh, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1636
Differential Revision: https://secure.phabricator.com/D3239
Summary:
Ponder is similar in spirit to the Wiki tool, but uses a Q&A
format and up/downvotes to signal user sentiment. Popular
questions are moved to the top of the feed on a 5-minute
cycle based on age (younger is better) and vote count (higher
is better).
Pre-apologies for noob diff.
Test Plan:
- `./bin/phd list` Should include `PonderHeatDaemon`; phd launch it
if necessary.
- Navigate to /ponder/ ; observe sanity when adding questions,
voting on them, and adding answers.
- Confirm that questions and answers are linkable using Q5 / Q5#A5 formatted object links.
- Confirm that searching for Ponder Questions works using built-in
search.
Feedback on code / schema / whatever organization very welcome.
Reviewers: nh, vrana, epriestley
Reviewed By: epriestley
CC: gmarcotte, aran, Korvin, starruler
Differential Revision: https://secure.phabricator.com/D3136
Summary: Apparently I am not qualified to do basic math.
Test Plan: Unit test.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3218
Summary: See discussion in D3078 for why I've separated this. Pretty sure it's not quite ready yet -- I want to build a couple of things on it so we have a better idea of what we need (autoincrement ID? <factType, objectA, epoch> primary key? objectB column? valueZ?) and don't need to do a ton of schema patches.
Test Plan: Applied patches, ran D3078.
Reviewers: vrana, btrahan, majak
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1581, T1562
Differential Revision: https://secure.phabricator.com/D3088
Summary: I think this is simpler? Includes test cases.
Test Plan: Ran tests. Loaded /paste/.
Reviewers: vrana, nh
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3209
Summary: See title. Adds features needed for D3136.
Test Plan:
Observe sanity (or run D3136 in a sandbox
and observe that voting works).
Reviewers: epriestley
Reviewed By: epriestley
CC: gmarcotte, aran, Korvin
Differential Revision: https://secure.phabricator.com/D3205
Summary:
A few goals here:
- Slightly simplify the Query classtree -- it's now linear: `Query` -> `OffsetPagedQuery` (adds offset/limit) -> `PolicyQuery` (adds policy filtering) -> `CursorPagedPolicyQuery` (adds cursors).
- Allow us to move from non-policy queries to policy queries without any backward compatibility breaks, e.g. Conduit methods which accept 'offset'.
- Separate the client limit ("limit") from the datafetch hint limit ("rawresultlimit") so we can make the heurstic smarter in the future if we want. Some discussion inline.
Test Plan: Expanded unit tests to cover offset behaviors.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3192
Summary:
- Commit detail view
- List of projects
- "edit" action which takes the user to a simple form where they can only add / remove projects.
- Integrated the project relationship into the commit search indexer
- fixed a bug from D790; it seems you must select the column if you're going to join against it later. Without this change searching for author or projectfails 100% for me.
Test Plan: added and removed projects. verified appropriate projects showed up in detail and edit view. searched for commits by project and found the ones I was supposed to...!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1614
Differential Revision: https://secure.phabricator.com/D3189
Summary:
- Store project members in edges.
- Migrate existing members to edge storage.
- Delete PhabricatorProjectAffiliation.
- I left the actual underlying data around just in case something goes wrong; we can delete it evenutally.
Test Plan:
- Ran migration.
- Created a new project.
- Joined and left a project.
- Added and removed project members.
- Manually called PhabricatorOwnersOwner::loadAffiliatedUserPHIDs() to verify its behavior.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3186
Summary:
I'm trying to make progress on the policy/visibility stuff since it's a blocker for Wikimedia.
First, I want to improve Projects so they can serve as policy groups (e.g., an object can have a visibility policy like "Visible to: members of project 'security'"). However, doing this without breaking anything or snowballing into a bigger change is a bit awkward because Projects are name-ordered and we have a Conduit API which does offset paging. Rather than breaking or rewriting this stuff, I want to just continue offset paging them for now.
So I'm going to make PhabricatorPolicyQuery extend PhabricatorOffsetPagedQuery, but can't currently since the `executeWithPager` methods would clash. These methods do different things anyway and are probably better with different names.
This also generally improves the names of these classes, since cursors are not necessarily IDs (in the feed case, they're "chronlogicalKeys", for example). I did leave some of the interals as "ID" since calling them "Cursor"s (e.g., `setAfterCursor()`) seemed a little wrong -- it should maybe be `setAfterCursorPosition()`. These APIs have very limited use and can easily be made more consistent later.
Test Plan: Browsed around various affected tools; any issues here should throw/fail in a loud/obvious way.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3177
Summary:
See T1602.
This is just the minimal functional patch; the scripts will continue
working because of the `DEFAULT ''`.
Test Plan:
Can't fully test this until I get more code working, but
nothing broke horribly yet.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran, Korvin
Maniphest Tasks: T1602
Differential Revision: https://secure.phabricator.com/D3147
Summary: The Log and PID directory should be separable in the config file
Test Plan: Start the daemons, and check if the pid and log files are stored in directories that were specified in the config file.
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3149
Summary:
- Adds a new "Applications" application.
- Builds an application list via application config instead of via hard-coding, so we can move toward better concepts of installing/uninstalling applications, etc.
- Applications indicate that they need attention with notice counts and brief status messages rathern than 50 giant tables of all sorts of app data.
I want to try replacing the home screen with this screen, pretty much. Not sure if this is totally crazy or not. What does everyone else think?
Test Plan: Will add screenshots.
Reviewers: btrahan, chad, vrana, alanh
Reviewed By: vrana
CC: aran, davidreuss, champo
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3129
Summary:
To make it easier to monitor daemons, let's store their current state
(running, died, exited, or unknown) to the db. The purpose of this is to
provide more information on the daemon console about the status of daemons,
especially when they are running on multiple machines. This is mostly backend
work, with only a few frontend changes. (It is also dependent on a change
to libphutil.)
These changes will make dead or stuck daemons more obvious, and will allow
more work on the frontend to hide daemons (and logs) that have exited cleanly,
i.e. ones we don't care about any more.
Test Plan:
- run db migration, check in db that all daemons were marked as exited
- start up a daemon, check in db that it is marked as running
- open web interface, check that daemon is listed as running
- after daemon has been running for a little bit, check in db that dateModified
is being updated (indicating daemon is properly sending heartbeat)
- kill -9 daemon (but don't run bin/phd yet), and check that db still shows it
as running
- edit daemon db entry to show it as being on a different host, and backdate
dateModified field by 3 minutes, and check the web ui to show that the status
is unknown.
- change db entry to have proper host, check in web ui that daemon status is
displayed as dead. Check db to see that the status was saved.
- run bin/phd stop, and see that the formerly dead daemon is now exited.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3126
Summary: You need to use -- to separate arguments for phd and the daemon.
Test Plan: Ran with the extra --.
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3106
Summary:
Some facts are aggregations of other facts. For example, we may compute how many times each macro is used in each object as a "raw fact":
Dnnn uses macro "psyduck" 6 times.
But we want to present this data in aggregate form, e.g. "order macros by popularity". We can do this at runtime and it probably won't be too awful a query, but we can also aggregate it cheaply:
Macro "psyduck" is used 3920 times across all objects.
...and then do a query like "select macros ordered by usage".
"Aggregate" facts support facts like this. The aggregate facts I've implemented are:
- Count of all objects.
- Count of objects of type X.
- Last time facts were updated.
These clearly fit the "aggregate" facts template well. I'm not 100% sure macros do. We can use this table to answer a question like "What are the most popular macros, ordered by use?" We can also use it to answer a question like "What are the most popular macros in the last 6 months?", if we build a specific fact for that. But we can't use it to answer a question like "What are the most popular macros between times X and Y?". Maybe that's important; maybe not.
This seems like a good fit for at least some types of facts.
I'll de-magic the keys a bit in the next diff.
Test Plan: Ran the engines and got some aggregated facts about other facts.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1562
Differential Revision: https://secure.phabricator.com/D3089
Summary:
Basic "Fact" application with some storage, part of a daemon, and a control binary.
= Goals =
The general idea is that we have various statistics we'd like to compute, like the frequency of image macros, reviewer responsiveness, task close rates, etc. Computing these on page load is expensive and messy. By building an ETL pipeline and running it in a daemon, we can precompute statistics and just pull them out of "stats" tables.
One way to do this is just to completely hard-code everything, e.g. have a daemon that runs every hour which issues a big-ass query and dumps results into a table per-fact or per fact-group. But this has a bunch of drawbacks: adding new stuff to the pipeline is a pain, various fact aggregators can't share much code, updates are slow and expensive, we can never build generic graphs on top of it, etc.
I'm hoping to build an ETL pipeline which is generic enough that we can use it for most things we're interested in without needing schema changes, and so that installs can use it also without needing schema changes, while still being specific enough that it's fast and we can build useful stuff on top of it. I'm not sure if this will actually work, but it would be cool if it does so I'm starting pretty generally and we'll see how far I get. I haven't built this exact sort of thing before so I might be way off.
I'm basing the whole thing on analyzing entire objects, not analyzing changes to objects. So each part of the pipeline is handed an object and told "analyze this", not handed a change. It pretty much deletes all the old data about that thing and then writes new data. I think this is simpler to implement and understand, and it protects us from all sorts of weird issues where we end up with some kind of garbage in the DB and have to wipe the whole thing.
= Facts =
The general idea is that we extract "facts" out of objects, and then the various view interfaces just report those facts. This change has on type of fact, a "raw fact", which is directly derived from an object. These facts are concerete and relate specifically to the object they are derived from. Some examples of such facts might be:
D123 has 9 comments.
D123 uses macro "psyduck" 15 times.
D123 adds 35 lines.
D123 has 5 files.
D123 has 1 object.
D123 has 1 object of type "DREV".
D123 was created at epoch timestamp 89812351235.
D123 was accepted by @alincoln at epoch timestamp 8397981839.
The fact storage looks like this:
<factType, objectPHID, objectA, valueX, valueY, epoch>
Currently, we supprot one optional secondary key (like a user PHID or macro PHID), two optional integer values, and an optional timestamp. We might add more later. Each fact type can use these fields if it wants. Some facts use them, others don't. For instance, this diff adds a "N:*" fact, which is just the count of total objects in the system. These facts just look like:
<"N:*", "PHID-xxxx-yyyy", ...>
...where all other fields are ignored. But some of the more complex facts might look like:
<"DREV:accept", "PHID-DREV-xxxx", "PHID-USER-yyyy", ..., ..., nnnn> # User 'yyyy' accepted at epoch 'nnnn'.
<"FILE:macro", "PHID-DREV-xxxx", "PHID-MACR-yyyy", 17, ..., ...> # Object 'xxxx' uses macro 'yyyy' 17 times.
Facts have no uniqueness constraints. For @vrana's reviewer responsiveness stuff, we can insert multiple rows for each reviewer, e.g.
<"DREV:reviewed", "PHID-DREV-xxxx", "PHID-USER-yyyy", nnnn, ..., mmmm> # User 'yyyy' reviewed revision 'xxxx' after 'nnnn' seconds at 'mmmm'.
The second value (valueY) is mostly because we need it if we sample anything (valueX = observed value, valueY = sample rate) but there might be other uses. We might need to add "objectB" at some point too -- currently we can't represent a fact like "User X used macro Y on revision Z", so it would be impossible to compute macro use rates //for a specific user// based on this schema. I think we can start here though and see how far we get.
= Aggregated Facts =
These aren't implemented yet, but the idea is that we can then take the "raw facts" and compute derived/aggregated/rollup facts based on the raw fact table. For example, the "count" fact can be aggregated to arrive at a count of all objects in the system. This stuff will live in a separate table which does have uniqueness constraints, and come in the next diff.
We might need some kind of time series facts too, not sure about that. I think most of our use cases today are covered by raw facts + aggregated facts.
Test Plan: Ran `bin/fact` commands and verified they seemed to do reasonable things.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, majak
Maniphest Tasks: T1562
Differential Revision: https://secure.phabricator.com/D3078
Summary: PhutilBufferedIterator now implements all the nonspecific logic here.
Test Plan:
Created a test script like this:
$iterator = new LiskMigrationIterator(new DifferentialRevision());
$iterator->setPageSize(3);
foreach ($iterator as $key => $rev) {
echo "{$key}: ".$rev->getID()."\n";
}
Ran it and verified sensible iteration results.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1562
Differential Revision: https://secure.phabricator.com/D3076
Summary: Also move the other tests up so they'll trigger when this stuff is touched.
Test Plan: liberate
Reviewers: nh, btrahan, vrana
Reviewed By: nh
CC: aran
Maniphest Tasks: T1283
Differential Revision: https://secure.phabricator.com/D3074
Summary:
Currently, we have this cumbersome `PhabricatorRepositoryCommitMessageDetailParser` hook. This is really old and outdated; I want to just use the Differential custom field parser. See T945 for a specific application.
However, it allows installs to override author/committer association. Instead, provide an event hook for doing this.
Test Plan: Added a listener, made every commit resolve to "turtle", parsed some commits, verified the events looked sane and they now correctly were all attributed to "turtle".
Reviewers: btrahan, vrana, nh
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1337
Differential Revision: https://secure.phabricator.com/D3040
Summary: Currently, MySQL/MySQLi connections store passwords in plain text on the object. Allow them to be stored in PhutilOpaqueEnvelopes instead. See D3053.
Test Plan: Loaded site.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3054
Summary: We pull "retries" and a doc link from PhabricatorEnv directly. Break these dependencies so the classes can move to libphutil.
Test Plan: Browsed site, triggered a schema exception and verified I still got the useful footer text.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1283
Differential Revision: https://secure.phabricator.com/D3053
Summary: Add an explicit close() method to connections and call it in GlobalLock.
Test Plan:
Wrote a script like this:
$lock = PhabricatorGlobalLock::newLock('test');
echo "LOCK";
$lock->lock();
sleep(10);
echo "UNLOCK";
$lock->unlock();
sleep(9999);
Using `SHOW FULL PROCESSLIST`, verified the connection closed after 10 seconds with both the "MySQL" and "MySQLi" implementations.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1470
Differential Revision: https://secure.phabricator.com/D3035
Summary: created a PhabricatorInlineCommentPreviewController so controllers in Diffusion and Differential respectively just have to handle the URI mapping and data loading like good little controllers.
Test Plan:
left inline comments on commits, deleted inline commits, submitted inline comments -- all worked well
did the same on some diffs
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1176
Differential Revision: https://secure.phabricator.com/D3034
Summary:
- Add edges for this relationship.
- Use edges to store this data.
- Migrate old data.
- Fix some warnings with generating feed stories about Aux and Edge transactions.
- Fix a task-task edge issue with "Create Subtask".
Test Plan:
- Migrated data, verified reivsions showed up.
- Attached and detached tasks to revisions and vice versa.
- Created a new revision with attached tasks.
- Created a subtask.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3018
Summary: Theses are sort of silly anyway since they should all have the actor in them rather than being sentence fragments, but make them work OK for English at least. See D3013.
Test Plan:
Ran:
echo pht('added %d dependencie(s): %s', 1, 'derp')."\n";
echo pht('added %d dependencie(s): %s', 2, 'derp, derp')."\n";
Got:
added dependency: derp
added dependencies: derp, derp
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3015
Summary:
blogs are collections of posts. a blog also has metadata like a name, description and "bloggers" that can edit the metadata of the blog and contribute posts.
changes include the post edit flow where bloggers can now select which blogs to publish to. also made various small tweaks throughout the UI to make things sensical and clean as the concept of blogs is introduced.
there's edges powering this stuff. bloggers <=> blogs and posts <=> blogs in particular.
Test Plan:
made blogs, deleted blogs, tried to make blogs with no bloggers. all went well.
verified ui to publish only showed up for public posts, published posts to blogs, un-published posts to blogs, re-published posts to blogs, deleted posts and verified they disappeared from blogs.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3003
Summary: See D3006. Move this data to the edge store.
Test Plan:
- Created dependencies, migrated, verified dependencies were preserved.
- Added new dependencies, they worked.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D3007
Summary:
- Use edges to store "X depends on Y" information in Maniphest.
- Show both "Depends On" and "Dependent Tasks".
- Migrate all the old edges.
Test Plan:
- Added some relationships, migrated, verified they were preserved.
- Added some new valid relationships, verified tasks got updated with sensible transactions and sent reasonable emails.
- Tried to add a cycle, got an ugly but effective error.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D3006
Summary: This should simplify a bunch of stuff in D3006 and D3003.
Test Plan: Will update D3006.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3010
Summary: See D3006, D3007. Make it easier to do migrations like that without holding all results in memory.
Test Plan:
Ran this code with an artificially small page size (2):
foreach (new LiskMigrationIterator(new DifferentialRevision()) as $rev) {
echo "Revision ".$rev->getID()."\n";
}
Verified each revision as loaded and processed.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D3008
'phd status' should have a stable result when invoked multiple times.
Automatically removing PID files for dead daemons every time 'phd status' is
invoked prevents tools from noticing that a daemon has died if something
happens to invoke 'phd status' before the tool looks. This affects Puppet
noticably, since it probably runs the status command every half hour.
'phd status' may be invoked by tools (such as puppet) which need to make
automated decisions about whether to start/restart the daemon. To enable this,
'phd status' now exits with 0 if all daemons are running, 1 if no daemons are
running, and 2 if some (but not all) daemons are running.
Summary:
Lisk currently behaves in two different ways if you call it like `load("cow")` (throws) versus `load(99999999)` (returns null), where neither ID exists.
This was intended to catch programming errors as distinct from missing data, but in practice the former is very rare and you have to handle the latter in most cases anyway. The case where you pass "0" is particularly confusing. See D2971 for an example.
On the balance, I think this ends up being far more confusing than helpful. Instead, just return NULL if we're sure there's no such object.
Test Plan: Reasoned about program behavior.
Reviewers: alanh, btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2977
Summary: http://www.networksorcery.com/enp/protocol/irc.htm
Test Plan: augment code with an additional debug line (phlog('hi');) so I can see my case was trigged and it will fall through. setup an ill-configured IRC server with ngircd. Configure an ircbot to connect to said ill-configured IRC server. verify ircbot connected to channel. verify in irc bot logs that debug line was invoked.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1452
Differential Revision: https://secure.phabricator.com/D2962
Summary: Allow the GC daemon to collect the new markup cache.
Test Plan: Ran gc daemon in "debug" mode, saw it collect cache entries.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2947
Summary:
- See D2945.
- Drop `cache` field from ManiphestTransaction.
- Render task descriptions and transactions through PhabricatorMarkupEngine.
- Also pull the list of macros more lazily.
Test Plan:
- Verified transactions and transaction preview work correctly and interact with cache correctly.
- Verified tasks descriptions and task preview work correctly.
- Verified we don't hit the imagemacro table when we're rendering everything from cache anymore.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2946
Summary:
The immediate issue this addresses is T1366, adding a rendering cache to Phriction. For wiki pages with code blocks especially, rerendering them each time is expensive.
The broader issue is that out markup caches aren't very good right now. They have three major problems:
**Problem 1: the data is stored in the wrong place.** We currently store remarkup caches on objects. This means we're always loading it and passing it around even when we don't need it, can't genericize cache management code (e.g., have one simple script to drop/GC caches), need to update authoritative rows to clear caches, and can't genericize rendering code since each object is different.
To solve this, I created a dedicated cache database that I plan to move all markup caches to use.
**Problem 2: time-variant rules break when cached.** Some rules like `**bold**` are time-invariant and always produce the same output, but some rules like `{Tnnn}` and `@username` are variant and may render differently (because a task was closed or a user is on vacation). Currently, we cache the raw output, so these time-variant rules get locked at whatever values they had when they were first rendered. This is the main reason Phriction doesn't have a cache right now -- I wanted `{Tnnn}` rules to reflect open/closed tasks.
To solve this, I split markup into a "preprocessing" phase (which does all the parsing and evaluates all time-invariant rules) and a "postprocessing" phase (which evaluates time-variant rules only). The preprocessing phase is most of the expense (and, notably, includes syntax highlighting) so this is nearly as good as caching the final output. I did most of the work here in D737 / D738, but we never moved to use it in Phabricator -- we currently just do the two operations serially in all cases.
This diff splits them apart and caches the output of preprocessing only, so we benefit from caching but also get accurate time-variant rendering.
**Problem 3: cache access isn't batched/pipelined optimally.** When we're rendering a list of markup blocks, we should be able to batch datafetching better than we do. D738 helped with this (fetching is batched within a single hunk of markup) and this improves batching on cache access. We could still do better here, but this is at least a step forward.
Also fixes a bug with generating a link in the Phriction history interface ($uri gets clobbered).
I'm using PHP serialization instead of JSON serialization because Remarkup does some stuff with non-ascii characters that might not survive JSON.
Test Plan:
- Created a Phriction document and verified that previews don't go to cache (no rows appear in the cache table).
- Verified that published documents come out of cache.
- Verified that caches generate/regenerate correctly, time-variant rules render properly and old documents hit the right caches.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1366
Differential Revision: https://secure.phabricator.com/D2945
Summary:
Certain types of things we should be storing in edges (notably, Task X depends on Task Y) should always be acyclic. Allow `PhabricatorEdgeEditor` to enforce this, since we can't correctly enforce it outside of the editor without being vulnerable to races.
Each edge type can be marked acyclic. If an edge type is acyclic, we perform additional steps when writing new edges of that type:
- We acquire a global lock on the edge type before performing any reads or writes. This ensures we can't produce a cycle as a result of a race where two edits add edges which independently do not produce a cycle, but do produce a cycle when combined.
- After performing writes but before committing transactions, we load the edge graph for each acyclic type and verify that it is, in fact, acyclic. If we detect cycles, we abort the edit.
- When we're done, we release the edge type locks.
This is a relatively high-complexity change, but gives us a simple way to flag an edge type as acyclic in a robust way.
Test Plan: Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D2940
Summary:
Currently, multiple unit tests that acquire global locks will interfere with each other. Namespace the locks so they don't.
(Possibly we should also rename this to PhabricatorStorageNamespaceLock or something since it's not really global any more, but that's kind of unwieldy...)
Test Plan: Acquired locks with --trace and verified they were namespaced properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D2939
Summary:
In unit tests which use fixtures, we open transactions on every connection we establish. However, since we don't track connections that are established with "$force_new" (currently, only GlobalLock connections) we never close these transactions normally.
Instead of not tracking these connections, track them using unique keys so we'll never get a cache hit on them.
Test Plan: Built unit tests on top of this, had them stop dying from unclosed transactions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D2938
Summary:
A later diff adds unit tests against edges, but we need real objects to connect with edges. Add some trivial objects to the Harbormaster database to compliment the similar HarbormasterScratchTable.
On its own, this does nothing interesting.
Test Plan: Built unit tests on this in a followup.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D2937
Summary:
See discussion here:
https://secure.phabricator.com/chatlog/channel/%23phabricator/?at=21186
Basically, MySQL usually raises a good error if we exceed "max_allowed_packet":
EXCEPTION: (AphrontQueryException) #1153: Got a packet bigger than 'max_allowed_packet' bytes
But sometimes it gives us a #2006 instead. This is documented, at least:
>"With some clients, you may also get a Lost connection to MySQL server during query error if the communication packet is too large."
http://dev.mysql.com/doc/refman//5.5/en/packet-too-large.html
Try to improve the error message to point at this as a possible explanation.
Test Plan: Faked an error, had it throw, read exception message. See also chatlog.
Reviewers: btrahan, skrul
Reviewed By: skrul
CC: aran
Differential Revision: https://secure.phabricator.com/D2923
Summary: See D2924.
Test Plan: Ran locks with blocking timeouts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2925
Summary: See D2906. This just adds text so they render pretty.
Test Plan:
Got pretty emails and rendered transactions.
{F13706}
Reviewers: btrahan, davidreuss
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2907
Summary:
- See D2741.
- When EdgeEditor performs edits, emit events.
- Listen for Maniphest edge events and save the changes as transactions.
- Do all this in a reasonably generic way that won't take too much rewriting as we use edges more generally.
Test Plan: Attached and detached commits from tasks, saw reasonable-looking transactions spring into existence.
Reviewers: btrahan, davidreuss
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2906
Summary: Simplify FeedQuery by making it extend from PhabricatorIDPagedPolicyQuery
Test Plan: Looked at feed on home, projects, user profile, and called `feed.query`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2905
Summary: Implementation is a little crazy but this seems to work as advertised.
Test Plan: Acquired locks with "lock.php". Verified they held as long as the process reamined open and released properly on kill -9, ^C, etc.
Reviewers: nh, jungejason, vrana, btrahan, Girish, edward
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1400
Differential Revision: https://secure.phabricator.com/D2864
Summary: Related to D2873.
Test Plan: Specified it and verified that highlighting still works.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2874
Summary:
Support the `--verbose` flag added in D2795 in `phd`. See T1389.
Also simplify argument generation a little bit.
Test Plan: Ran "nice" daemon with debug, daemon + verbose, daemon + no verbose.
Reviewers: vrana, jungejason, edward, aurelijus
Reviewed By: aurelijus
CC: aran
Maniphest Tasks: T1389
Differential Revision: https://secure.phabricator.com/D2797
Summary: I tried also filling the column by empty space but this looks better.
Test Plan: Displayed a comment both with visible and hidden inlines.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1216
Differential Revision: https://secure.phabricator.com/D2789
Summary: It requires `allow_url_fopen` which we don't check in setup and our installation is about to disable it.
Test Plan:
Login with OAuth.
/oauth/facebook/diagnose/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2787
Test Plan:
Altered database.
Wrote a custom translation and selected it in preferences.
Verified that the text is custom translated.
Set language back to default.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D2757
Summary:
This is the first step in Phabricator internationalization.
It adds a translation selector and calls it at startup.
Installations can add custom selectors to override some texts.
We can add official translations in future.
Next step is to allow user to choose his translation which will override the global one.
This is currently used only for English plurals.
Test Plan: Displayed a diff with unit test error, verified that it says 'Detail' or 'Details' and not 'Detail(s)'.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D2753
Summary:
Some e-mail clients display this header and it needs to be constant.
This is somehow involved but I doubt that there is a simpler solution.
Test Plan:
Applied SQL patch.
Commented on revision, commented on commit, changed package.
Verified that the `Thread-Topic` has constant and human readable value.
Reviewers: epriestley
Reviewed By: epriestley
CC: ola, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2745
Summary: This is //extremely// basic but dead simple and should cover us for v1, I think. Let me know what features you need.
Test Plan: Used UI example page.
Reviewers: allenjohnashton, ddfisher, keebuhm
Reviewed By: ddfisher
CC: aran, ender
Maniphest Tasks: T944
Differential Revision: https://secure.phabricator.com/D2732
Summary: Made it possible to link and unlink LDAP accounts with Phabricator accounts.
Test Plan:
I've tested this code locally and in production where I work.
I've tried creating an account from scratch by logging in with LDAP and linking and unlinking an LDAP account with an existing account. I've tried to associate the same LDAP account with different Phabricator accounts and it failed as expected.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, auduny, svemir
Maniphest Tasks: T742
Differential Revision: https://secure.phabricator.com/D2722
Summary:
HPHP doesn't like resolved symlinks.
Also I like this code better.
Test Plan: Used and not used custom Celerity map.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2694
Summary: Added `PhabricatorBuiltinPatchList` entry so that "storage upgrade" will update the database. Renamed and numbered the notification.sql patch.
Test Plan: Drop phabricator_feed.feed_storynotification table if it exists and run bin/storage upgrade to check if the patch is correctly applied.
Reviewers: epriestley, btrahan, allenjohnashton
Reviewed By: epriestley
CC: ddfisher, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2687
Summary:
- The config is called "resource-path" and the script references "resource-path", but the actual value checked for is "resource-map".
- Use nonempty(), since defaulting with getEnvConfig() will give you null if the setting exists but is set to null. This default is nearly useless so maybe we should change it to use coalesce().
- Remove Celerity map initialization from warmup. We don't currently initialize the environment in warmup, and Celerity initialization now depends on the environment.
Test Plan: Ran patch locally and on FPM-Warmup.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: hsb, aran
Differential Revision: https://secure.phabricator.com/D2662
Summary:
See T1303, which presents a reasonable case for inclusion of these characters in valid usernames.
Also, unify username validity handling.
Test Plan: Created a new user with a valid name. Tried to create a new user with an invalid name. Ran unit tests.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1303
Differential Revision: https://secure.phabricator.com/D2651
Summary: replace differential.find with differential.query and display in requested order
Test Plan: say D444D222D443D442 and ensure they are in the correct order
Reviewers: epriestley
Reviewed By: epriestley
CC: Mnkras, aran
Differential Revision: https://secure.phabricator.com/D2656
Summary:
We have custom static resources.
We currently include them in Phabricator's celerity resource map which is causing some pain - we need to regenerate the file without our custom resources before pushing upstream, we need to discard our changes before pulling from upstream and we need to rebuild with our changes to run Phabricator.
This diff allows writing and reading the map in other location.
The plan is this - I will run `celerity_mapper.php` twice - once to build Phabricator-only resources (to push to upstream) and once to build Phabricator + ours resoruces to put in our directory.
Better solution would be to create a map just with our resources and read and combine it with Phabricator resources.
But it is complicated because we have dependencies on Phabricator resources.
Test Plan:
`celerity_mapper.php webroot`
`celerity_mapper.php webroot ../facebook/src/__celerity_resource_map__.php`
Delete Phabricator's celerity map, set 'celerity.resource-path' and successfully load Phabricator.
Reviewers: epriestley, edward
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T721
Differential Revision: https://secure.phabricator.com/D2630
Summary:
- `kill_init.php` said "Moving 1000 files" - I hope that this is not some limit in `FileFinder`.
- [src/infrastructure/celerity] `git mv utils.php map.php; git mv api/utils.php api.php`
- Comment `phutil_libraries` in `.arcconfig` and run `arc liberate`.
NOTE: `arc diff` timed out so I'm pushing it without review.
Test Plan:
/D1234
Browsed around, especially in `applications/repository/worker/commitchangeparser` and `applications/` in general.
Auditors: epriestley
Maniphest Tasks: T1103
Summary:
Unittest databases are not always destroyed in our setup.
It could be caused by `__destruct()` not called in case of a fatal error.
Test Plan:
arc unit src/applications/calendar/storage/holiday
Reviewers: edward, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2568
Summary:
- We currently write every PHID we generate to a table. This was motivated by two concerns:
- **Understanding Data**: At Facebook, the data was sometimes kind of a mess. You could look at a random user in the ID tool and see 9000 assocs with random binary data attached to them, pointing at a zillion other objects with no idea how any of it got there. I originally created this table to have a canonical source of truth about PHID basics, at least. In practice, our data model has been really tidy and consistent, and we don't use any of the auxiliary data in this table (or even write it). The handle abstraction is powerful and covers essentially all of the useful data in the app, and we have human-readable types in the keys. So I don't think we have a real need here, and this table isn't serving it if we do.
- **Uniqueness**: With a unique key, we can be sure they're unique, even if we get astronomically unlucky and get a collision. But every table we use them in has a unique key anyway. So we actually get pretty much nothing here, except maybe some vague guarantee that we won't reallocate a key later if the original object is deleted. But it's hard to imagine any install will ever have a collision, given that the key space is 36^20 per object type.
- We also currently use PHIDs and Users in tests sometimes. This is silly and can break (see D2461).
- Drop the PHID database.
- Introduce a "Harbormaster" database (the eventual CI tool, after Drydock).
- Add a scratch table to the Harbormaster database for doing unit test meta-tests.
- Now, PHID generation does no writes, and unit tests are isolated from the application.
- @csilvers: This should slightly improve the performance of the large query-bound tail in D2457.
Test Plan: Ran unit tests. Ran storage upgrade.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: csilvers, aran, nh, edward
Differential Revision: https://secure.phabricator.com/D2466
Summary: D2490 was not my finest hour and I incorrectly thought it was a null value from error. In reality this error is impossible and its just a valid empty array so instead use the empty predicate to bail.
Test Plan: with our logic combined, this be tested
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2503
Summary: 'cuz github issue 114 came into existence. instead, just return false early here. note i am not sure if I should phlog that this is happening or not but its not exception worthy IMO.
Test Plan: lint-only 'cuz i don't want to setup an IRC server locally / somehow get my local phabricator instance accessible out there. happy to test end to end if there's an easier way...!
Reviewers: vrana, epriestley
Reviewed By: vrana
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2490
Summary:
D2470 added Package mailhandler, which was configured incorrectly in the getRequiredClasses function. this makes it like the other mail handlers
Reported at https://github.com/facebook/phabricator/issues/112
Test Plan: setup mode no longer fails
Reviewers: epriestley, jungejason, royklopper
Reviewed By: royklopper
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2476
Summary:
For package creation and deletion, send email to all the owners For
package modification, detect important fields such as owners and paths, and then
send out emails to all owners (including deleted owners and current owners)
Also start using transaction for package creation/deletion/modification.
Test Plan:
- tested mail creation and deletion
- tested modification to auditing enabled, primary owners, owners, paths
Reviewers: epriestley, nh, vrana
Reviewed By: epriestley
CC: prithvi, aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2470
Summary:
Required in order to run tests successfully in the HipHop interpreter.
Similar to D2362.
Test Plan: Run the tests in an HipHop runtime.
Reviewers: epriestley
Reviewed By: epriestley
CC: jungejason, aran, Koolvin, vrana
Differential Revision: https://secure.phabricator.com/D2365
Summary:
- Merge CommitTask daemon into PullLocal daemon. This is another artifact of past instability (and order-dependent parsers). We still publish to the timeline, although this was the last consumer. Long term we'll probably delete timeline and move to webhooks, since everyone who has asked about this stuff has been eager to trade away the durability and ordering of the timeline for the ease of use of webhooks. There's also no reason to timeline this anymore since parsing is no longer order-dependent.
- Add `phd start` to start all the daemons you need. Add `phd restart` to restart all the daemons you need. So cool~
- Simplify and improve phd and Diffusion daemon documentation.
Test Plan:
- Ran `phd start`.
- Ran `phd restart`.
- Generated/read documentation.
- Imported some stuff, got clean parses.
Reviewers: btrahan, csilvers
Reviewed By: csilvers
CC: aran, jungejason, nh
Differential Revision: https://secure.phabricator.com/D2433
Summary:
- Move email to a separate table.
- Migrate existing email to new storage.
- Allow users to add and remove email addresses.
- Allow users to verify email addresses.
- Allow users to change their primary email address.
- Convert all the registration/reset/login code to understand these changes.
- There are a few security considerations here but I think I've addressed them. Principally, it is important to never let a user acquire a verified email address they don't actually own. We ensure this by tightening the scoping of token generation rules to be (user, email) specific.
- This should have essentially zero impact on Facebook, but may require some minor changes in the registration code -- I don't exactly remember how it is set up.
Not included here (next steps):
- Allow configuration to restrict email to certain domains.
- Allow configuration to require validated email.
Test Plan:
This is a fairly extensive, difficult-to-test change.
- From "Email Addresses" interface:
- Added new email (verified email verifications sent).
- Changed primary email (verified old/new notificactions sent).
- Resent verification emails (verified they sent).
- Removed email.
- Tried to add already-owned email.
- Created new users with "accountadmin". Edited existing users with "accountadmin".
- Created new users with "add_user.php".
- Created new users with web interface.
- Clicked welcome email link, verified it verified email.
- Reset password.
- Linked/unlinked oauth accounts.
- Logged in with oauth account.
- Logged in with email.
- Registered with Oauth account.
- Tried to register with OAuth account with duplicate email.
- Verified errors for email verification with bad tokens, etc.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2393
Summary:
I will use it for highlighting users which are not currently available.
Maybe I will also use it in the nagging tool.
I don't plan creating a UI for it as API is currently enough for us.
Maybe I will visualize it at /calendar/ later.
I plan creating `user.deletestatus` method when this one will be done.
Test Plan:
`storage upgrade`
Call Conduit `user.addstatus`.
Verify DB.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2382
Summary:
We will need it for two purposes:
- Status tool.
- Nagging tool - @aran suggested using "3 business days" and I don't want it to fall on New Year's Eve or such.
I don't plan working on any interface for editing this as this kind of data should be always imported.
Test Plan:
`bin/storage upgrade`
`scripts/calendar/import_us_holidays.php`
/calendar/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2375
Summary:
- Unit tests can request storage fixtures.
- We build one fixture across all tests in the process, which can quickstart (takes roughly 1s to build, 200ms to destroy for me). This is a one-time cost for running an arbitrary number of fixture-based tests.
- We isolate all the connections inside transactions for each test, so individual tests don't affect one another.
Test Plan: Ran unit tests, which cover the important properties of fixtures.
Reviewers: btrahan, vrana, jungejason, edward
Reviewed By: btrahan
CC: aran, davidreuss
Maniphest Tasks: T140
Differential Revision: https://secure.phabricator.com/D2345
Summary: Allow the default namespace to be set in configuration, so you can juggle multiple copies of sandbox test data or whatever.
Test Plan: Changed default namespace, verified web UI and "storage" script respect it.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T345
Differential Revision: https://secure.phabricator.com/D2341
Summary:
This addresses three issues with the current patch management system:
# Two people developing at the same time often pick the same SQL patch number, and then have to go rename it. The system catches this, but it's silly.
# Second/third-party developers can't use the same system to manage auxiliary storage they may want to add.
# There's no way to build mock databases for unit tests that need to do reads.
To resolve these things, you can now name your patches whatever you want and conflicts are just merge conflicts, which are less of a pain to fix than filename conflicts.
Dependencies are now a DAG, with implicit dependencies created on the prior patch if no dependencies are specified. Developers can add new concrete subclasses of `PhabricatorSQLPatchList` to add storage management, and define the dependency branchpoint of their patches so they apply in the correct order (although, generally, they should not depend on the mainline patches, presumably).
The commands `storage upgrade --namespace test1234` and `storage destroy --namespace test1234` will allow unit tests to build and destroy MySQL storage.
A "quickstart" mode allows an upgrade from scratch in ~1200ms. Destruction takes about 200ms. These seem like fairily reasonable costs to actually use in tests. Building from scratch patch-by-patch takes about 6000ms.
Test Plan:
- Created new databases from scratch with and without quickstart in a separate test namespace. Pointed the webapp at the test namespaces, browsed around, everything looked good.
- Compared quickstart and no-quickstart dump states, they're identical except for mysqldump timestamps and a few similar things.
- Upgraded a legacy database to the new storage format.
- Destroyed / dumped storage.
Reviewers: edward, vrana, btrahan, jungejason
Reviewed By: btrahan
CC: aran, nh
Maniphest Tasks: T140, T345
Differential Revision: https://secure.phabricator.com/D2323
Summary: "Committed" is SVN-specific language, and confusing in Git and Mercurial. Use neutral language instead.
Test Plan: Inspection.
Reviewers: btrahan, Makinde, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T909
Differential Revision: https://secure.phabricator.com/D2087
Summary:
Couple of small improvements:
- Delete `randomon` macro.
- Make name unique (deleting current conflicts randomly).
- Image macro must be alone on the line.
- Filter by name.
Test Plan:
Run SQL.
/file/macro/
/file/macro/?name=imagemacro
Try to create conflicting name.
Write this comment:
Test imagemacro.
imagemacro
Reviewers: aran, epriestley
Reviewed By: epriestley
CC: epriestley, Koolvin
Differential Revision: https://secure.phabricator.com/D2230
Summary:
This event is fired after a task is created and assigned with an id.
Use case is sending an email notification to everyone in a project when a new task is
submitted to said project.
Test Plan:
Implement the event listener, submit a new task to a project, see if the project members
receive an email notification. I will submit the event handler in a separate diff once it's a bit
prettier and tested more thoroughly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D2159
Summary: Introduces a scope-guarded way to override the env config, for unit tests which are sensitive to config values.
Test Plan: Ran unit tests.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2237
Summary:
Provides a basic start for access policies. Objects expose various capabilities, like CAN_VIEW, CAN_EDIT, etc., and set a policy for each capability. We currently implement three policies, PUBLIC (anyone, including logged-out), USERS (any logged-in) and NOONE (nobody). There's also a way to provide automatic capability grants (e.g., the owner of an object can always see it, even if some capability is set to "NOONE"), but I'm not sure how great the implementation feels and it might change.
Most of the code here is providing a primitive for efficient policy-aware list queries. The problem with doing queries naively is that you have to do crazy amounts of filtering, e.g. to show the user page 6, you need to filter at least 600 objects (and likely more) before you can figure out which ones are 500-600 for them. You can't just do "LIMIT 500, 100" because that might have only 50 results, or no results. Instead, the query looks like "WHERE id > last_visible_id", and then we fetch additional pages as necessary to satisfy the request.
The general idea is that we move all data access to Query classes and have them do object filtering. The ID paging primitive allows efficient paging in most cases, and the executeOne() method provides a concise way to do policy checks for edit/view screens.
We'll probably end up with mostly broader policy UIs or configuration-based policies, but there are at least a few cases for per-object privacy (e.g., marking tasks as "Security", and restricting things to the members of projects) so I figured we'd start with a flexible primitive and the simplify it in the UI where we can.
Test Plan: Unit tests, played around in the UI with various policy settings.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D2210
Summary: See T993. MySQL fails very very softly if you request an engine which does not exist. Detect and fail if the InnoDB engine is missing or broken.
Test Plan: Faked InnoDB missing, got a failure. Ran normally, got success.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T993
Differential Revision: https://secure.phabricator.com/D2160
Summary:
We don't use versioned URIs for images, so when they change users may get old versions.
This was a particular issue with the recent logo change, which several users reported cache-related issues from.
Instead, use Celerity to manage image URI versions in addition to CSS/JS.
This is complicated, because we need to rewrite image URIs inside of CSS, which means the hash of a CSS file has to be derived from the current image data. Otherwise, when we updated an image the CSS wouldn't update, so we wouldn't be any better off.
So basically we:
- Find all the "raw" files, and put them into the map.
- Find all the CSS/JS, perform content-altering transformations on it (i.e., not minification) based on the partial map, and then put it into the map based on transformed hashes.
(If we wanted, we could now do CSS variables or whatever for "free", more or less.)
Test Plan:
- Regenerated celerity map, browsed site, verified images generated with versioned URIs.
- Moved "blue" flag image over "green" flag image, regenerated map, verified "green" flag image and the associated CSS changed hashes.
- Added transformation unit tests; ran unit tests.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1073
Differential Revision: https://secure.phabricator.com/D2146
Summary: This separates common MySQL stuff (identifiers and comments escaping, error codes, connection retries) from PHP extension specific stuff (connect, query, fetch, errors, escape string).
Test Plan:
/
Use `AphrontMySQLiDatabaseConnection` in `PhabricatorLiskDAO`, load homepage, edit task, save task.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran
Differential Revision: https://secure.phabricator.com/D2113
Summary: Use Edges to attach Commits and Tasks. Note, no "edit attached commits" interface from tasks yet since the search backend needs a little work to list commits in a sensible way.
Test Plan: Attached commits to tasks. Looked at commits, looked at tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D2105
Summary:
We have a lot of cases where we store object relationships, but it's all kind of messy and custom. Some particular problems:
- We go to great lengths to enforce order stability in Differential revisions, but the implementation is complex and inelegant.
- Some relationships are stored on-object, so we can't pull the inverses easily. For example, Maniphest shows child tasks but not parent tasks.
- I want to add more of these and don't want to continue building custom stuff.
- UIs like the "attach stuff to other stuff" UI need custom branches for each object type.
- Stuff like "allow commits to close tasks" is notrivial because of nonstandard metadata storage.
Provide an association-like "edge" framework to fix these problems. This is nearly identical to associations, with a few differences:
- I put edge metadata in a separate table and don't load it by default, to keep edge rows small and allow large metadata if necessary. The on-edge metadata seemed to get abused a lot at Facebook.
- I put a 'seq' column on the edges to ensure they have an explicit, stable ordering within a source and type.
This isn't actually used anywhere yet, but my first target is attaching commits to tasks for T904.
Test Plan: Made a mock page that used Editor and Query. Verified adding and removing edges, overwriting edges, writing and loading edge data, sequence number generation.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, 20after4
Differential Revision: https://secure.phabricator.com/D2088
Summary:
Most setters returns `$this` but some don't.
I guess it's not by purpose.
Test Plan:
arc lint
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2085
Summary:
These are the issues identified by the linter in D2052. I don't think any cause bugs, but they are all reasonable errors to raise and the linter correctly
detected that they are suspicious.
Test Plan: Mostly inspection.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2053
Summary: For production servers, minify CSS and JS by stripping comments, whitespace, etc.
Test Plan: Looked at CSS/JS, it was much smaller.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T264
Differential Revision: https://secure.phabricator.com/D2034
Summary:
I'd like to use this regex elsewhere and copying and pasting is
bad.
Test Plan: none
Reviewers: casey, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2038
Summary:
- Still really really rough.
- Adds a full synchronous mode for debugging.
- Adds some logging.
- It can now allocate EC2 machines and put webroots on them in a hacky, terrible way.
- Adds a base query class.
Test Plan: oh hey look a test page? http://ec2-50-18-65-151.us-west-1.compute.amazonaws.com:2011/
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D2026
Summary: D2023 adds a new '*' token to javelinsymbols (indicating that a behavior is 'installed'). This fixes a sanity-check regex in PhabricatorJavelinLinter that validates the output of javelinsymbols so that it is aware of this new token type.
Test Plan:
Patched javelinsymbols.cpp from D2023 to externals/javelin/support/javelinsymbols, build the new javelinsymbols binary, then ran
arc lint --lintall webroot/rsrc/js/application/core/behavior-drag-and-drop-textarea.js
(before this diff, that throws an error -- after it works with no lint)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2025
Summary:
We render a huge picture of a PDF for PDFs right now, etc. This is hella dumb.
Also allow users to force this rendering style, and change the link name.
Test Plan: Uploaded image and non-image files, used layout=link and name=....
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1040
Differential Revision: https://secure.phabricator.com/D2006
Test Plan:
Run setup with 'differential.attach-task-class' set to:
- ''
- 'FacebookTasksAttacher'
- 'X'
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1999
Summary: I'll mark this one up inline since it's all separate bugs.
Test Plan:
- Created a diff with eight changes: (newline absent -> newline present, newline present -> newline absent, newline present -> newline present, newline absent -> newline absent) x (short file with change near end, long file with change near middle).
- Viewed diff in Ignore All, Ignore Most, Ignore Trailing and Show All whitespace modes.
- All 32 results seemed sensible.
- Really wish this stuff was better factored and testable. Need to fix it. :(
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1030
Differential Revision: https://secure.phabricator.com/D1992
Summary:
It is now possible to set config setting requiring class of certain implementation to something completely else.
The consequence is that your Phabricator may stop working after update because you didn't implement some new method.
This diff validates the class upon usage.
It throws exception which is better than fatal thrown currently after calling undefined method.
Better solution would be to validate classes when setting the config but it would be too expensive - respective class definitions would have to be loaded and checked by reflection.
I was also thinking about some check script but nobody would run it after changing config.
The same behavior should be implemented for these settings:
- metamta.mail-adapter
- metamta.maniphest.reply-handler
- metamta.differential.reply-handler
- metamta.diffusion.reply-handler
- storage.engine-selector
- search.engine-selector
- differential.field-selector
- maniphest.custom-task-extensions-class
- aphront.default-application-configuration-class
- controller.oauth-registration
Test Plan:
Send comment, verify that it pass.
Change `metamta.differential.reply-handler` to incompatible class, verify that sending comment shows nice red exception.
Set `metamta.differential.reply-handler` to empty string, verify that it throws.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1919
Summary:
- Affects the "Inline Comments" summary table which appears in comments that have attached inlines in the discussion threads in Differential.
- Prepares for inclusion in Diffusion.
- No application changes (minor CSS), just factors code better.
- Simplify/separate CSS.
Test Plan: Looked at on-diff and off-diff comment summaries in Differential, display looked correct.
Reviewers: davidreuss, nh, btrahan
Reviewed By: davidreuss
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1928
Summary:
It is currently not possible to select source code covered by reticle when creating comment.
This diff hides reticle on mouseout from reply area.
Test Plan:
Hover inline comment, verify that reticle is displayed.
Reply, verify that reticle is displayed when mouseover reply, hidden otherwise.
Repeat for create.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1851
Summary:
- Add inline comments to Audits, like Differential.
- Creates new storage for the comments in the Audits database.
- Creates a new PhabricatorAuditInlineComment class, similar to DifferentialInlineComment.
- Defines an Interface which Differential and Audit comments conform to.
- Makes consumers of DifferentialInlineComments consume objects which implement that interface instead.
- Adds save
NOTE: Some features are still missing! Wanted to cut this off before it got crazy:
- Inline comments aren't shown in the main comment list.
- Inline comments aren't shown in the emails.
- Inline comments aren't previewed.
I'll followup with those but this was getting pretty big.
@vrana, does the SQL change look correct?
Test Plan:
- Created, edited, deleted, replied to, reloaded and saved inline comments in Diffusion, on the left and right side of diffs.
- Created, edited, deleted, replied to, reloaded and saved inline comments in Differentila, on the left and right side of primary and diff-versus-diff diffs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1898
Summary:
1. The setup flow complains if you haven't updated your schema, so that section
should be moved above the setup flow.
2. The setup flow tells you to lower your timeout, but it doesn't tell you how
low will make it stop complaining.
Test Plan:
Didn't test the setup.
Regenerated the docs and saw the change.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1888
Summary: Last of the big final patches. Left a few debatable classes (12 out of about 400) that I'll deal with individually eventually.
Test Plan: Ran testEverythingImplemented.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1881
Summary:
These are all unambiguously unextensible. Issues I hit:
- Maniphest Change/Diff controllers, just consolidated them.
- Some search controllers incorrectly extend from "Search" but should extend from "SearchBase". This has no runtime effects.
- D1836 introduced a closure, which we don't handle correctly (somewhat on purpose; we target PHP 5.2). See T962.
Test Plan: Ran "testEverythingImplemented" unit test to identify classes extending from `final` classes. Resolved issues.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1843
Summary:
Adds a macro handler that spams your channel with macros. Config is:
- macro.size: scale macros to this size before rasterizing
- macro.sleep: sleep this many seconds between lines (evade flood protection)
Test Plan: derpderp
Reviewers: kdeggelman, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1838
Summary:
This adds a new configuration setting:
"notification.actions" : [
"commit",
"abandon",
"actions"
]
if not set, displays all actions, if is set, display only what is set to display
Test Plan: add the notification.actions settings and set accordingly
Reviewers: epriestley, zeeg
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1820
Summary:
A user reported that their install (on an unusual piece of hardware) was hitting this timeout. We don't need to be quite so stingy; just use the default 30s timeout.
Also remove some kind of sentence fragment since I no longer remember what it meant and it doesn't make sense.
Test Plan: This change resolved the issue.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1805
Summary: The docs say "http://www.domain.com/" but if you don't put "/api/" it fails. GOTCHA!
Test Plan: Removed "/api/", launched bot, it worked.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T935
Differential Revision: https://secure.phabricator.com/D1763
Summary: Added support for audit comment, concern, accept
Test Plan: Comment / Concern / Accept audit, and say "What's new?" in IRC
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1730
Summary:
Added phabot irc command to directly message a user rather than outputting in a
channel.
Syntax:
ex:
````Korvin, D1717```
results in phabot private messaging me the info on D1717
Test Plan: ##nick##, [DTPVF]n
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1717
Test Plan:
Type ##@makinde## to comment, verify that it is converted to ##@Makinde##.
Verify that ##@NonExistent## stays ##@NonExistent##.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1725
Summary:
Added what's new flood protection and fixed array_push issues.
Also added rhetoric for "Commit"
Test Plan: say "What's new?" twice within one minute
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1684
Summary:
Added "What's new?" to the ircbot
====Matches
```What is new?
What's new?
Whats new```
Test Plan:
<`Korvin> what is new?
<korvinbot-local> Derpen created D1: Herped the derp - http://phabricator.net/D1
It shows five.
Reviewers: epriestley
Reviewed By: epriestley
CC: epriestley, aran
Differential Revision: https://secure.phabricator.com/D1666
Summary:
I typed up like 30 pages here and then got my soul crushed by T895, but here's
the summary:
I looked at several charting libraries. There aren't very many that seem to be
any good and have an open-source license.
I also want the charts to be scriptable in JS so we can add good interactivity
where appropriate.
Raphael is an SVG drawing library which seems very solid. gRaphael is a charting
library on top of Raphael that is a lot less solid, but seems kind of OK.
Overall, I think this selection gives us a lot of flexibility, although we'll
have to pay some costs up front. I'd rather do that then get limited later,
though.
That said, I'm open to other suggestions here if anyone has experience or wants
to take a different stab at researching things.
This is largely for @vii and D1643.
Test Plan: Created a basic, fairly OK chart (see next revision).
Reviewers: btrahan, vii
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1654
Summary:
This is pretty simple and unpolished, but it's getting pretty big and it seems
like a reasonable starting point.
- Log chat in various "channels".
- Conduit record and query methods.
- IRCBot integration for IRC logging
Major TODO:
- Web UI is really unpolished and has no search, paging, anchor-linking, etc.
Basically all presentation stuff, though.
- I think the bot should have a map of channels to log with channel aliases?
- The "channels" should probably be in a separate table.
- The "authors" should probably be correlated to Phabricator accounts somehow,
where possible.
Test Plan: Used phabotlocal to log #phabricator.
Reviewers: kdeggelman, btrahan, Koolvin
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T837
Differential Revision: https://secure.phabricator.com/D1625
Summary:
We sometimes call PhabricatorEnv::getProductionURI($file->getBestURI()) or
similar, but this may currently cause us to construct a URI like this:
http://domain.com/http://cdn-domain.com/file/data/xxx/yyy/name.jpg
Instead, if the provided URI has a domain already, leave it unmodified.
Test Plan: Attached a file to a task; got an email with a valid URI instead of
an invalid URI.
Reviewers: btrahan
Reviewed By: btrahan
CC: Makinde, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1622
Summary:
I accidentally added two "104" patches. This actually works OK for the most part
but is fundamentally bad and wrong.
Merge the patches (installs applied both as "104", so we can't move one to
"105") and add a safeguard.
Test Plan: Ran upgrade_schema.php with two "104" patches, got error'd. Ran
without, got successs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1614
"Content-Disposition: attachment"
Summary:
We currently serve some files off the primary domain (with "Content-Disposition:
attachment" + a CSRF check) and some files off the alternate domain (without
either).
This is not sufficient, because some UAs (like the iPad) ignore
"Content-Disposition: attachment". So there's an attack that goes like this:
- Alice uploads xss.html
- Alice says to Bob "hey download this file on your iPad"
- Bob clicks "Download" on Phabricator on his iPad, gets XSS'd.
NOTE: This removes the CSRF check for downloading files. The check is nice to
have but only raises the barrier to entry slightly. Between iPad / sniffing /
flash bytecode attacks, single-domain installs are simply insecure. We could
restore the check at some point in conjunction with a derived authentication
cookie (i.e., a mini-session-token which is only useful for downloading files),
but that's a lot of complexity to drop all at once.
(Because files are now authenticated only by knowing the PHID and secret key,
this also fixes the "no profile pictures in public feed while logged out"
issue.)
Test Plan: Viewed, info'd, and downloaded files
Reviewers: btrahan, arice, alok
Reviewed By: arice
CC: aran, epriestley
Maniphest Tasks: T843
Differential Revision: https://secure.phabricator.com/D1608
Summary:
Some browsers will still sniff content types even with "Content-Type" and
"X-Content-Type-Options: nosniff". Encode "<" and ">" to prevent them from
sniffing the content as HTML.
See T865.
Also unified some of the code on this pathway.
Test Plan: Verified Opera no longer sniffs the Conduit response into HTML for
the test case in T865. Unit tests pass.
Reviewers: cbg, btrahan
Reviewed By: cbg
CC: aran, epriestley
Maniphest Tasks: T139, T865
Differential Revision: https://secure.phabricator.com/D1606
Summary: Remarkup object names require #1 for linking to comments which is not
very intuitive.
Test Plan:
D1558#4e01328c
D1558#1
D1558#comment-1
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1565
Summary:
Sending CSRF token in GET forms is dangerous because if there are external links
on the target page then the token could leak through Referer header.
The token is not required for anything because GET forms are used only to
display data, not to perform operations.
Sending CSRF tokens to external URLs leaks the token immediately.
Please note that <form action> defaults to GET.
PhabricatorUserOAuthSettingsPanelController suffered from this problem for both
reasons.
Test Plan: Save my settings (POST form).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1558
Summary: posix may not be loaded on the web/cgi SAPI but we call posix functions
on this pathway, which we hit on /daemon/. Fall back to exec if we don't have
posix.
Test Plan: Added "&& false" and verified the page executed a bunch of "ps"
tests.
Reviewers: Koolvin, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T821
Differential Revision: https://secure.phabricator.com/D1540
Summary: We currently allow you to launch abstract daemons; use
setConcreteOnly() to only list/launch concrete daemons.
Test Plan: Ran "phd list" (no abstract daemons listed), "phd launch
PhabricatorRepositoryCommitDiscoveryDaemon" (reasonable error message).
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T801
Differential Revision: https://secure.phabricator.com/D1487
Summary: When we try to kill a daemon but discover it isn't running, we should
remove the PID file. We can also simplify the logic here.
Test Plan: Ran "phd stop" a couple of times, subsequent runs did not try to stop
a legion of dead daemons.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T781
Differential Revision: https://secure.phabricator.com/D1421
Summary:
See D1416. Add options to file-embed syntax, and document new code and
embed options.
Test Plan: Used new options in markup blocks.
Reviewers: davidreuss, btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T336
Differential Revision: https://secure.phabricator.com/D1417
Summary:
By default, PHP-FMP (an alternate PHP FCGI SAPI) cleans the entire environment
for child processes. This means we have no $PATH.
This causes some confusing failures for reasons I don't fully understand. If you
do these things:
exec_manual('env');
exec_manual('export');
...they show no $PATH, as expected. If you do this:
exec_manual('echo $PATH');
...it shows a path. And this works (i.e., it finds the executable):
exec_manual('ls');
...but this fails (it says "no ls in ((null))"):
exec_manual('which ls');
So, basically, the sh -c process itself gets a default PATH somehow, but its
children don't. I don't realllly get why this happens, but clearly an empty
$PATH is a misconfiguration, and can easily be remedied.
See discussion here: https://github.com/facebook/libphutil/issues/7
Test Plan: Applied patch to Centos6 + nginx + PHP-FPM machine, ran setup, the
configuration issue was detected and I was given information on resolving it.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1413
Summary:
I locked this down a little bit recently, but make
double-extra-super-sure that we aren't sending the user anywhere suspicious or
open-redirecty. This also locks down protocol-relative URIs (//evil.com/path)
although I don't think any browsers do bad stuff with them in this context, and
header injection URIs (although I don't think any of the modern PHP runtimes are
vulnerable).
Test Plan:
- Ran tests.
- Hit redirect page with valid and invalid next URIs; was punted to / for
invalid ones and to the right place for valid ones.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: arice, aran, epriestley, btrahan
Differential Revision: https://secure.phabricator.com/D1369
Summary:
- PHP uses a SAPI ("server API") to determine how it interacts with the caller
(e.g., how to read the environment, how to read flags, what code to execute).
- There are several different SAPIs: cli, cgi, cgi-fcgi, apache, etc.
- Each SAPI has different behavior -- for instance, the "cgi" SAPI emits some
CGI headers unless told not to, so a script like 'echo "x"' actually echoes some
headers and then 'x' as an HTTP body.
- In some setups, "php" may be php-cgi.
- If you run php-cgi as "php scriptname.php" and your ENV has an existing CGI
request in it, it runs that CGI request instead of the script. This causes an
infinite loop.
- Add checks to verify that "php" is the "cli" SAPI binary, not some other
SAPI.
- In particular, cPanel uses suphp and is affected by this configuration
issue. See this thread:
https://lists.marsching.com/pipermail/suphp/2008-September/002036.html
Test Plan:
- On a cPanel + suphp machine, ran setup and was stopped for having the
"cgi-fcgi" SAPI instead of throw into an infinite loop.
- Applied the suggested remedy, setup now runs fine.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Differential Revision: https://secure.phabricator.com/D1390
Summary:
With T764, http://localhost doesn't work anymore. So add instructions
about how to support it by modifying the hosts file.
Test Plan:
- turned on setup mode and the error message did show up
- turned off the setup mode and the error message also showed up
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T764
Differential Revision: https://secure.phabricator.com/D1370
Summary:
Chrome/Chromium won't set cookies on these domains, at least under
Ubuntu. See T754. Detect brokenness and explode.
Test Plan:
Logged into phabricator as "http://derps/" (failed) and
"http://derps.com/" (worked) in Chromium. Set config to "http://derps/" (config
exploded) and "http://local.aphront.com/" (config OK).
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T754
Differential Revision: https://secure.phabricator.com/D1355
Summary:
we used to need this function for security purposes, but no longer need
it. remove it so that some call sites can be optimized via smarter data
fetching, and so the whole codebase can have one less thing in it.
Test Plan:
verified the images displayed properly for each of the following
- viewed a diff with added images.
- viewed a user feed
- viewed a user profile
- viewed all image macros
- viewed a paste and clicked through "raw link"
weakness in testing around proxy files and transformed files. not sure what
these are. changes here are very programmatic however.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Maniphest Tasks: T672
Differential Revision: https://secure.phabricator.com/D1354
Summary: When private messaged, the bot responds via private message to the
sender, instead of sending a private message to itself.
Test Plan: Mentioned tasks in public channels and private messages.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T274
Differential Revision: https://secure.phabricator.com/D1350
Summary: Simple notificaiton handler that reads the difx event timeline and
posts notifications to IRC.
Test Plan: Ran it in #phabricator.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1337
Summary: Preview of Add Reviewers looks silly without actually showing them
Test Plan:
Go to any diff
Leap into action: Add Reviewers
Add some reviewers
Write some comment
Preview including Added reviewers should be displayed
Change action to Comment
Added reviewers should disappear
Repeat with Add CCs
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, vrana
Differential Revision: https://secure.phabricator.com/D1276
Summary:
It used to be more useful for daemons to spew random debugging information, but
features like "phd debug" and some fixes to error reporting like D1101 provide
better ways to debug, test, develop and diagnose daemons.
- Stop writing "." every time MetaMTA sends a message.
- Stop spewing the entire IRC protocol from the IRC bot unless in debug mode.
- Stop writing GC daemon log entries about collecting daemon logs (DURRR)
unless in debug mode.
Test Plan: Ran daemons in debug and non-debug modes, got expected level of
noisiness.
Reviewers: jungejason, nh, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D1268
Summary: Allow the bot to answer the question "where is X?", where X is a
symbol.
Test Plan:
phabotlocal joined the chat room.
epriestley: phabotlocal: where is DarkConsole?
phabotlocal left the chat room. (Remote host closed the connection)
phabotlocal joined the chat room.
epriestley: phabotlocal: where is DarkConsole?
phabotlocal left the chat room. (Remote host closed the connection)
phabotlocal joined the chat room.
epriestley: phabotlocal: where is DarkConsole?
phabotlocal: class DarkConsole (php):
http://local.aphront.com/diffusion/SUBC/browse/src/aphront/console/api/DarkConsole.php$22
epriestley: thanks phabotlocal that is vastly more useful
phabotlocal left the chat room. (Remote host closed the connection)
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T315
Differential Revision: https://secure.phabricator.com/D1261
Summary:
- For context, see T547. This is the last (maybe?) in a series of diffs that
moves us off raw sha1() calls in order to make it easier to audit the codebase
for correct use of hash functions.
- This breaks CSRF tokens. Any open forms will generate an error when
submitted, so maybe upgrade off-peak.
- We now generate HMAC mail keys but accept MAC or HMAC. In a few months, we
can remove the MAC version.
- The only remaining callsite is Conduit. We can't use HMAC since Arcanist
would need to know the key. {T550} provides a better solution to this, anyway.
Test Plan:
- Verified CSRF tokens generate properly.
- Manually changed CSRF to an incorrect value and got an error.
- Verified mail generates with a new mail hash.
- Verified Phabricator accepts both old and new mail hashes.
- Verified Phabricator rejects bad mail hashes.
- Checked user log, things look OK.
Reviewers: btrahan, jungejason, benmathews
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T547
Differential Revision: 1237
Test Plan:
Created a listener that adds some patterns to $matches array, reloaded
Differential, some changesets were not shown as generated.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, mareksapota
Differential Revision: 1200
Summary: See T632. When we miss a @mention, preserve the original case. This
approach is slightly unwieldy, but preserves backward compatibility (remarkup is
cached in Differential and Maniphest).
Test Plan:
https://secure.phabricator.com/file/view/PHID-FILE-u7z5j73dxrr4vuwkdcy3/
Reviewers: aran, btrahan
Reviewed By: aran
CC: aran, epriestley
Differential Revision: 1141
Summary: create CelerityResourceGraph, which extends AbstractDirectedGraph.
since we've done a bunch of work already to load the resource graph into memory
CelerityResourceGraph simply stores a copy and makes loadEdges work off that
stored copy.
Test Plan:
made phabricator-prefab require herald-rule-editor
~/code/phabricator> ./scripts/celerity_mapper.php webroot
Finding static resources...
Processing 154
files..........................................................................................................................................................
[2011-11-22 11:28:29] EXCEPTION: (Exception) Cycle detected in resource graph:
phabricator-prefab => herald-rule-editor => phabricator-prefab at
[/Users/btrahan/Dropbox/code/phabricator/scripts/celerity_mapper.php:173]
fixed phabricator-prefab requiring herald-rule-editor. re-ran celerity_mapper
and no errors!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: 1132
Summary: a well-titled diff this be. i feel 'meh' about the change; doesn't
seem to help too much imo.
Test Plan:
edited my custom conf file to have errors -
127.0.0.1 => 127.0.0.2
mysql_user => mysql_users
and for phabricator to be in setup mode. for each error i verified that i liked
the display.
Reviewers: epriestley, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 1129
Summary:
Move event framework from Phabricator to libphutil so it can be used in other
phutil projects, such as Arcanist.
Test plan:
Use along with path to libphutil, events should work as expected.
Reviewers: epriestley
Differential Revision: 1098
Summary: Allow tweaking Differential mail before sending.
Test Plan:
Wrote a listener renaming Differential attachments and it worked without
problems.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, mareksapota, davidreuss
Differential Revision: 1091
Summary:
make sure all symboles can be loaded to avoid issues like missing
methods in descendants of abstract base class.
Test Plan:
ran it and verified it passes; remove a method in a descendant class
and verified that the test failed.
Reviewers: epriestley, nh
Reviewed By: nh
CC: aran, nh, jungejason
Differential Revision: 1023
Summary: See T548 and D996. Makes Phabricator configure the remarkup engine so
http:// and https:// get linked. Also make the "named link" syntax respect the
whitelist.
Test Plan:
- Whitelisted URIs (they get linked).
- Other URIs (not linked).
- Whitelisted, named URIs (linked).
- Other, named URIs (treated as phriction links).
- Actual phriction links (work correctly).
Reviewers: jungejason, nh, tuomaspelkonen, aran, benmathews
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 997
Summary: There are currently two files, but all scripts require both of them,
which is clearly silly. In the longer term I want to rewrite all of this init
stuff to be more structured (e.g., merge webroot/index.php and __init_script__
better) but this reduces the surface area of the ad-hoc "include files" API we
have now, at least.
Test Plan:
- Grepped for __init_env__.php (no hits)
- Ran a unit test (to test unit changes)
- Ran a daemon (to test daemon changes)
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 976
Summary:
This is a pretty bad, but working implmentation of a way to kill one particular PID that
is controlled by Phabricator. Also remove the second 'stop' from the ##phd help## list.
Test Plan:
[ricky@rhelpad01 phabricator] (phd-stop-twice)$ ./bin/phd status
PID Started Daemon
30154 Oct 1 2011, 2:38:08 AM PhabricatorMetaMTADaemon
30172 Oct 1 2011, 2:38:09 AM PhabricatorMetaMTADaemon
30190 Oct 1 2011, 2:38:09 AM PhabricatorMetaMTADaemon
30210 Oct 1 2011, 2:38:09 AM PhabricatorMetaMTADaemon
[ricky@rhelpad01 phabricator] (phd-stop-twice)$ ./bin/phd stop 30190
Stopping daemon 'PhabricatorMetaMTADaemon' (30190)...
Daemon 30190 exited normally.
[ricky@rhelpad01 phabricator] (phd-stop-twice)$ ./bin/phd stop 123456
123456 is not controlled by Phabricator. Not killing.
[ricky@rhelpad01 phabricator] (phd-stop-twice)$ ./bin/phd stop
Stopping daemon 'PhabricatorMetaMTADaemon' (30154)...
Stopping daemon 'PhabricatorMetaMTADaemon' (30172)...
Stopping daemon 'PhabricatorMetaMTADaemon' (30210)...
Daemon 30210 exited normally.
Daemon 30154 exited normally.
Daemon 30172 exited normally.
Reviewers: epriestley
CC:
Differential Revision: 975
Summary: Shows events which a page dispatched, plus all the registered
listeners.
Test Plan:
Pretty basic for now, but works OK:
https://secure.phabricator.com/file/view/PHID-FILE-49fcd23081ce55cf9369/
(I also made it dispatch some dummy events to verify they show up.)
Reviewers: aran
Reviewed By: aran
CC: aran
Differential Revision: 973
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
setup
Summary: See T481. We'll fail the pcntl test if we don't have this, in a
potentially confusing way. Test and detect missing 'php' explicitly before we
try the pcntl test, so we can give the user a better error message.
Test Plan: In setup mode, did a good run and then faked it to execute 'phpx'
instead to get a failure.
Reviewers: johnduhart, jungejason, tuomaspelkonen, aran
Reviewed By: tuomaspelkonen
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 878
Summary: Allow Conduit method so they stop raising lint warnings. See D874.
Test Plan: Ran "arc lint" on conduit files and was no longer given frivolous
warnings.
Reviewers: nh, jungejason, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh, epriestley
Differential Revision: 875
Summary: If the link text is a URI, just treat it as a nameable (and possibly
relative) URI link. See tasks.
Test Plan: Copy/pasted the doc example into Phriction, links worked.
Reviewers: skrul, hunterbridges, jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 882
Summary: This may make it easier to debug problems with CLI + pcntl
Test Plan: Added a syntax error to the script and got more useful output
Reviewers: johnduhart, jungejason, tuomaspelkonen, aran
Reviewed By: johnduhart
CC: aran, johnduhart
Differential Revision: 869
Summary: Do JOIN in the protocol handler, after we receive 376 ("end of motd").
Test Plan: Ran bot, it joined a channel after receieving a 376 command.
Reviewers: moos3, codeblock, aran, jungejason, tuomaspelkonen
Reviewed By: moos3
CC: aran, moos3
Differential Revision: 855
"user":"authenticationusername",
"pass":"thisuserspassowrd",
This will allow people with internal irc servers to use this if they control access from ldap for irc.
domain
Summary:
See D758, D759.
- Provide a strongly recommended setting which permits configuration of an
alternate domain.
- Lock cookies down better: set them on the exact domain, and use SSL-only if
the configuration is HTTPS.
- Prevent Phabriator from setting cookies on other domains.
This assumes D759 will land, it is not effective without that change.
Test Plan:
- Attempted to login from a different domain and was rejected.
- Logged out, logged back in normally.
- Put install in setup mode and verified it revealed a warning.
- Configured an alterate domain.
- Tried to view an image with an old URI, got a 400.
- Went to /files/ and verified links rendered to the alternate domain.
- Viewed an alternate domain file.
- Tried to view an alternate domain file without the secret key, got a 404.
Reviewers: andrewjcg, erling, aran, tuomaspelkonen, jungejason, codeblock
CC: aran
Differential Revision: 760
Summary:
We don't currently validate CSRF tokens on this workflow. This allows an
attacker to upload arbitrary files on the user's behalf. Although I believe the
tight list of servable mime-types means that's more or less the end of the
attack, this is still a vulnerability.
In the long term, the right solution is probably to pass CSRF tokens on all Ajax
requests in an HTTP header (or just a GET param) or something like that.
However, this endpoint is unique and this is the quickest and most direct way to
close the hole.
Test Plan:
- Drop-uploaded files to Files, Maniphest, Phriction and Differential.
- Modified CSRF vaidator to use __csrf__.'x' and verified uploads and form
submissions don't work.
Reviewers: andrewjcg, aran, jungejason, tuomaspelkonen, erling
Commenters: andrewjcg, pedram
CC: aran, epriestley, andrewjcg, pedram
Differential Revision: 758
Summary:
Provide a {T123} syntax which pulls in the entire name of an object, not just a
link to it. A major use for this is organizing projects using wiki pages. Since
handle links show object status now, this lets you organize stuff in an ad-hoc
way and get a reasonable overview of it. We can make handles richer in the
future, too.
The performance on this isn't perfect (it adds some potential single gets) but I
think it's okay for now and I don't want to make remarkup engine even more
complex until the preprocess/postprocess stuff has had a chance to settle and
I'm more confident it works.
In Differential and Maniphest we'll also incorrectly cache the object
state/name, but that'll fix itself once I move the cache code to use
preprocess/postprocess correctly.
Test Plan:
- See https://secure.phabricator.com/file/view/PHID-FILE-5f9ca32407bec20899b9/
for an example.
- Generated and looked over the documentation.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran, hunterbridges
CC: skrul, aran, jungejason, epriestley
Differential Revision: 784
Summary:
This accomplishes two goals:
- Data fetching is now grouped across blocks.
- Demonstrates that D737 actually works.
Test Plan: Used @mentions in Phriction preview, they rendered properly. Verified
only one service call was being made across blocks.
Reviewed By: jungejason
Reviewers: hunterbridges, jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 738
query
Summary:
- Provide an example unit test, and document it.
- Document database isolation better.
- When we issue an unsimulated query to the isolated connection, throw a
helpful message.
- Pygments is complaining about my madeup "lang=demo", change it to
"lang=text".
Test Plan:
- Ran the unit test (sanity check).
- Ran all other unit tests (verify I didn't break isolation).
- Added a queryfx(..., 'SELECT 1') to a test and verified it throws.
- Read the documentation.
Reviewed By: edward
Reviewers: edward, jungejason, tuomaspelkonen, aran
CC: aran, edward
Differential Revision: 773
Summary:
This gets all the major pieces working. Allows you to drag-and-drop files in
Differential and Phriction, and embed files in remarkup with {Fxxx} references.
See also task.
I'm explicitly not documenting this yet since it's still pretty rough.
Test Plan: Dragged and dropped stuff into Differential and Phriction.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran, tomo
Commenters: tomo
CC: aran, tomo, jungejason
Differential Revision: 674
Summary: This has to table scan a ginormous table right now, give it a fighting
chance with a more usable key.
Test Plan:
- Launched GC daemon, no errors.
- Used test console to create a new transcript.
- Viewed some old transcripts.
- Ran EXPLAIN on the SELECT and verified it was utilizing the garbageCollected
key.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 735
Summary: This didn't end up getting used but I neglected to delete it.
Test Plan: git grep
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, jungejason
Differential Revision: 721
problems
Summary: Detect more PHP misconfigurations in setup.
Test Plan: Broke my configuration, ran setup, it seemed to detect all the
problems and issue meaningful error messages.
Reviewed By: jungejason
Reviewers: hunterbridges, 10098, jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 717
Summary:
See T344. Currently, there's a hard-coded 12MB filesize limit and some awkward
interactions with MySQL's max_allowed_packet. Make this system generally more
robust:
- Move the upload limit to configuration.
- Add setup steps which reconcile max_allowed_packet vs MySQL file storage
limits.
- Add a layer of indirection between uploading files and storage engines.
- Allow the definition of new storage engines.
- Define a local disk storage engine.
- Add a "storage engine selector" class which manages choosing which storage
engines to put files in.
- Document storage engines.
- Document file storage classes.
Test Plan:
Setup mode:
- Disabled MySQL storage engine, misconfigured it, configured it correctly.
- Disabled file storage engine, set it to something invalid, set it to
something valid.
- Verified max_allowed_packet is read correctly.
Application mode:
- Configured local file storage.
- Uploaded large and small files.
- Verified larger files were written to local storage.
- Verified smaller files were written to MySQL blob storage.
Documentation:
- Read documentation.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 695
Summary:
Share code with the new PhabricatorDifferenceEngine, which handles diffs with no
changes correctly.
(This isn't the same issue as file moves, but I ran into it while generating a
repro case.)
Test Plan: Previously, changes which didn't change file content (e.g., property
changes) would throw. Now they work.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 698
Summary:
Single brackets are getting some troublesome false positives in Facebook's
install. Particularly, there's a weird convention at Facebook of tagging diffs
by putting stuff like "[perf]" or "[chat]" in the title, although this isn't
turned into structured data at any stage. When commits appear in Diffusion, we
currently link such ad-hoc tags to Phriction.
Wikipedia uses double-bracket sytnax, as do many other wikis, so this seems like
a reasonable burden to place on the lightweightness of the markup. The
alternative is selectively disabling Phriction markup in some interfaces, but
I'd rather allow integration in commit messages and just guard the syntax more
closely.
(I'm not providing any sort of migration plan since this landed less than a week
ago and I'm pretty confident no one has built a huge wiki yet, but I added a
CHANGELOG note.)
Test Plan: Edited a wiki document and added some links. Verified single brackets
were unlinked and double brackets were linked.
Reviewed By: jungejason
Reviewers: hsb, aran, jungejason, tuomaspelkonen
CC: aran, jungejason, epriestley
Differential Revision: 689
Summary:
This is really rough and needs work (particularly, there's some diff code I
really need to refactor since I sort-of-copy-pasted it) but basically
functional.
Show text changes between diffs and allow users to revert to earlier versions.
Differential's line-oriented diff style isn't ideal for large blocks of text but
I'm betting this is probably good enough in most cases. We can see how bad it is
in practice and then fix it if needbe.
I added a bunch of support for "description" but didn't add the feature in this
diff, I'll either follow up or task it out since it should be a pretty
straightforward change.
Test Plan: Looked at history for several Phriction documents, clicked "previous
change" / "next change", clicked revert buttons.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen
CC: aran, hsb, epriestley
Differential Revision: 687
Summary:
- Exceptions on the rendering pathway currently go uncaught and result in a
blank page. Commonly, this is a bad require_celerity_resource() call. Although
we can't safely render a page if the rendering pathway is broken, we can show a
useful message.
- When PHP exits because of a fatal error, there is an opportunity to run code
in the shutdown handler. This allows us to show messages at least some of the
time, e.g. "call to unknown function derp() in somefile.php at line 99"
- flip dem tables
Test Plan: Added fatals ("derp();") and rendering exceptions
("require_celerity_resource('does-not-exist')") to a controller and verified
that the error handling behavior is now more useful.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley
Differential Revision: 680
Summary: Document linking and some general layout improvements. I'd like to
eventually do more meta-dataey things with links (like store them separately and
check them for 404s) but this is a decent start.
Test Plan:
https://secure.phabricator.com/file/view/PHID-FILE-d756b94a06b69c273fce/
Reviewed By: jungejason
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 650
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:
If we're going to hardcode a path, at least let's do it in a way that works on RHEL too.
Test Plan:
Successfully ran the setup script on RHEL.
Reviewers:
epriestley
CC:
Differential Revision: 623
Summary:
Add the differential parse cache to the GC. This is the largest object in the
system by a wide margin, I think.
This table is potentially gigantic which is why the script truncates it before
doing a schema change.
Test Plan: Ran the GC daemon, it cleaned up some parse caches.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: tuomaspelkonen
CC: aran, jungejason, tuomaspelkonen, epriestley
Differential Revision: 620
Summary:
Now that we store language with pastes, we can include this in Conduit.
Test Plan:
Tried it on a paste with a blank language, and one with a specified language.
16:14:50 <@CodeBlock> P1
16:14:51 <@codeblock-phabot> P1: http://phabricator.local/P1 - test.php
16:15:05 <@CodeBlock> P43
16:15:06 <@codeblock-phabot> P43: http://phabricator.local/P43 - sadoijfoisaf (php)
Reviewers:
epriestley, Ttech
CC:
Differential Revision: 616
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: When the remote closes the connection, phabot goes into a busy loop
because of PHP's "nothing should ever be an error" semantics. Instead, detect
connection termination.
Test Plan: Disabled the "PONG" response in the protocol handler and let freenode
disconnect phabot. It spun out of control before, now it detects the issue and
exits to await automatic restart.
Reviewed By: jungejason
Reviewers: codeblock, jungejason, aran, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 582
Summary:
Summarize (or at least link to) pastes when P<id> is given in a channel. This depends on D552!
Test Plan:
04:34:03 <@codeblock-phabot> P15 - http://phabricator.local/P15
04:35:22 <@CodeBlock> P1 asdf foo Bar Baz Diet Sprite P4
04:35:24 <@codeblock-phabot> P1 - http://phabricator.local/P1
04:35:24 <@codeblock-phabot> P4 - http://phabricator.local/P4
Reviewers:
epriestley
CC:
Differential Revision: 553
Summary:
Maniphest tasks can now be summarized by referencing their respective T<id> in irc.
Test Plan:
03:29:28 <@CodeBlock> T218
03:29:29 <@codeblock-phabot> T218: NYANCAT (Priority: Wishlist) - https://secure.phabricator.com/T218
03:30:14 <@CodeBlock> T58 asdf foo bar baz mcFoo T163
03:30:15 <@codeblock-phabot> T58: Allow AphrontFormView to render as a <div /> instead of a form (Priority: Wishlist) - https://secure.phabricator.com/T58
03:30:15 <@codeblock-phabot> T163: arc amend is slow (Priority: Needs Triage) - https://secure.phabricator.com/T163
...
03:28:23 <@CodeBlock> Oh. Hah, I'm testing against production Conduit. Ah well.
03:28:40 <@CodeBlock> I don't always test my code...but when I do, I do it in prod! :D
Reviewers:
epriestley
CC:
Differential Revision: 551
Summary:
When a user gets @mentioned in Differential, add them as a CC.
No Maniphest hookup yet since I want to make that one a little more formal.
Depends on D518.
Test Plan:
@mentioned a user and they were added as a CC.
Reviewed By: jungejason
Reviewers: tomo, mroch, jsp, jungejason, aran, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 519
Summary:Make a new directory, src/infrastructure/markup/remarkup/markuprule/paste/
Make a new class called PhabricatorRemarkupRulePaste in that directory.
Add the rule to DifferentialMarkupEngineFactory.
Test Plan: Created a task in maniphest. Put P1 and P2 in the content.
Created P1 and P2 in Paste. Verified P1 and P2 were highlighted and
linked correctly.
Reviewers:epriestley, codeblock
CC:jungejason
Differential Revision: 539
Summary:
The web console already correctly checks for processes which are running but
can't be signaled. Share this check in the CLI.
Test Plan:
Looked at the web console. Poked at the CLI.
Reviewed By: fratrik
Reviewers: fratrik
CC: aran, fratrik
Differential Revision: 544
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:
Provides basic Remarkup support for @mentions. No application integration yet so
these aren't terribly useful until that happens.
Test Plan:
https://secure.phabricator.com/file/view/PHID-FILE-83d68e7af6085ae928df/
Reviewers: tomo, mroch, jsp
Commenters: tomo
CC: aran, tomo, epriestley
Differential Revision: 517
Summary:
The MySQL MyISAM fulltext backend defaults to a minimum length of 4, but many
3-letter words are useful search terms. Provide instructions for configuring
MySQL to reduce the auto-stopword length.
Test Plan:
Followed instructions, searched for a 3-letter term and got a hit.
Reviewed By: jungejason
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran, jungejason
Differential Revision: 471
Summary:
Tweaks to the paste app:
- I realized that unlike all the other apps, it makes more sense for the
default view of this one to be "create paste" instead of "list pastes" since
when you access the application directly you are most often wanting to share
something. Swap list out of the default slot and make edit the default.
- Make the textarea bigger (usability).
- Allow you to copy an existing paste.
- Implement 'raw view'.
- Tweak/adjust list view (usability, formatting).
- Tweak page titles.
Test Plan:
Created, copied, and listed pastes. Viewed raw paste. Created an invalid paste.
Tried to create a copy of a nonexistant paste.
Reviewed By: codeblock
Reviewers: codeblock, jungejason, aran, tuomaspelkonen
CC: aran, epriestley, codeblock
Differential Revision: 456
Summary:
Nicks can contain more characters than were allowed. The new regex is ugly,
but should include most of them.
Test Plan:
Joined the bot as phabot-c0d`eb1^ock successfully.
Reviewers:
epriestley
CC:
Differential Revision: 453
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 'gd' an explicit optional dependency, test for it in setup, and make the
software behave correctly if it is not available.
When generating file thumnails, provide reasonable defaults and behavior for
non-image files.
Test Plan:
Uploaded text files, pdf files, etc., and got real thumbnails instead of a
broken image.
Simulated setup and gd failures and walked through setup process and image
fallback for thumbnails.
Reviewed By: aran
Reviewers: toulouse, jungejason, tuomaspelkonen, aran
CC: aran, epriestley
Differential Revision: 446
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:
There was a last-minute edit to this to fix a typo before rP089d8327 landed
which accidentally made it impossible to pass the check. :)
Test Plan:
Put install into setup mode, changed protocol to 'http', 'ftp'.
Reviewed By: cadamo
Reviewers: toulouse, codeblock, cadamo
Commenters: toulouse, codeblock
CC: aran, cadamo, toulouse, codeblock
Differential Revision: 434
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
Summary:
Well, since I couldn't regenerate my arcanist cert I figured out that this wass because "workflows" are unavailable there now. I really can not figure out why but it was.
I added in the setup script, the ability to check if is present the protocol of the host and if it has a trailing slash a the end of the line, since both are needed to generate the cert.
Users now only be able to upload valid image files with mimetype of jpg, jpeg,
png and gif.
Test Plan:
FIRST: DO NOT apply those changes! then
1- go to settings->arcanist certificate and the click on regenerate ... humm
2- On your config file, delete the trailing slash at the end and the protocol on "phabricator.base-uri", then go to setting->arcanist certificate. Here you
will see something like this "phabricator.example.comapi\/" instead of
"http:\/\/phabricator.example.com\/api\/".
SECOND: Now apply this changes:
1- Go to settings->arcanist certificate and the click on regenerate.
2- On your config file, delete the trailing slash at the end and the protocol
on "phabricator.base-uri", and setup "phabricator.setup" to true.
3- Then go to setting->arcanist certificate and you could see that this was successfully generated.
THIRD:
Go to settings->account and try to upload an invalid image file, and do the same on "youruserna"->edit profile.
Reviewed By: epriestley
Reviewers: epriestley jungejason
CC: epriestley jugesason cadamo aran
Differential Revision: 391
Summary:
Allows you to link to comments with "D123#3" or "T123#3", then adds a pile of JS
to try to make it not terrible. :/
The thing I'm trying to avoid here is when someone says "look at this!
http://blog.com/#comment-239291" and you click and your browser jumps somewhere
random and you have no idea which comment they meant. Since I really hate this,
I've tried to avoid it by making sure the comment is always highlighted.
Test Plan:
Put T1#1 and D1#1 in remarkup and verified they linked properly.
Clicked anchors on individual comments.
Faked all comments hidden in Differential and verified they expanded on anchor
or anchor change.
Reviewed By: aran
Reviewers: aran, tomo, mroch, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 383
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:
Sendmail is seriously difficult to configure; SendGrid is extremely easy. It's
also pretty expensive ($80/mo) but there are a bunch of startups that already
have plans so it's effectively free for them.
Test Plan:
Configured SendGrid and sent reply email through it.
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, epriestley
Differential Revision: 376
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:
Removes calling `which` and instead checks the file path that PHPMailerLite uses directly (/usr/bin/sendmail).
This fixes $PATH export issues which occur on certain platforms causing un-expected errors during setup.
Test Plan:
* Run setup on server without sendmail install & error should be presented.
* Install sendmail then re-run setup & no error should be presented.
Reviewers: epriestley
Differential Revision: 375
Summary:
After successfully installing phabricator on my Mac OS X 10.6.7, I was unable to
link my accounts to either Facebook or GitHub.
I diagnosed that file_get_contents() and fopen() were not working properly.
After installing the php openssl package I was able to get it linking
successfully.
Test Plan:
With php's openssl extension disabled, and phabricator installed. Try linking to
Facebook and GitHub and observe that it fails. You can visit the Auth
Diagnostics page and "Facebook Graph" and "App Login" should fail.
With php's openssl extension enabled, linking to Facebook and GitHub should be
successful.
Change the configuration to add "phabricator.setup = false".
Disable php's openssl extension. Visit the phabricator site and observe that it
requires you to install php's openssl extension.
Enable php's openssl extension. Visit the phabricator site and observe that it
installs fine.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: 352
Summary:
I need to add some more conduit methods / info to make this at all useful but
here's some basics.
Test Plan:
Hung out in #phabot-test
Reviewed By: mroch
Reviewers: mroch, codeblock, aran, jungejason, tuomaspelkonen
CC: aran, mroch, epriestley
Differential Revision: 300
Summary:
Allow construction of handlers which use Conduit.
Test Plan:
Made a bot that connects to local and runs conduit.ping.
Reviewed By: mroch
Reviewers: mroch, codeblock, aran, jungejason, tuomaspelkonen
CC: aran, mroch
Differential Revision: 299
Summary:
This is purely a prototype at the moment, but the basic functionality sort of
works.
I'm not sure how far I want to go with this but I think we might be able to get
somewhere without it being gross.
The idea here is to build a notification server WITHOUT using Comet, since Comet
is extremely difficult and complicated.
Instead, I use Flash on the client. LocalConnection allows flash instances to
talk to each other and connect() can be used as a locking primitive. This allows
all the instances to elect a master instance in a race-safe way. The master is
responsible for opening a single connnection to the server.
On the server, I use Node.js since PHP is pretty unsuitable for this task.
See Github Issue #3: https://github.com/facebook/phabricator/issues/3
One thing I need to figure out next is if I can reasonably do SSL/TSL over Flash
(it looks like I can, in theory, with the as3crypto library) or if the server
needs to just send down version information and trigger a separate Ajax call on
the client.
Test Plan:
Created a client pool and connected it to the server, with election and failover
apparently working correctly.
Reviewed By: aran
Reviewers: Girish, aran, jungejason, tuomaspelkonen, davidrecordon
Commenters: Girish, davidrecordon
CC: aran, epriestley, Girish, davidrecordon
Differential Revision: 284
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:
This is sort of a silly/fun project but I think there's some utility. For
example, mroch added some handlers to an eggdrop or something similar to look
for "D12345" and print out the title/link, which was actually pretty useful.
We could also add logging here and subsume the more-or-less unowned Facebook
tool that does the same thing, especially since we can get a bunch of good stuff
it doesn't support (like search) more or less for free.
This is also an easy way to provide some example code for writing Conduit system
agents.
This is a minimal implementation which creates a bot that connects to a
hard-coded server and sits there indefinitely. Next steps:
- Add conduit/sysagent support
- Write differential/maniphest/diffusion handlers
- Move configuration to the web interface (?) and integrate with phd
- Write a logging handler?
Test Plan:
Ran bot with "exec_daemon.php", it connected to the hard-coded server and sat
there indefinitely.
Reviewed By: aran
Reviewers: codeblock, mroch, tuomaspelkonen, aran, jungejason
CC: aran, epriestley
Differential Revision: 283
Summary:
ccheever did an install and gave me some feedback about issues he hit. This
tries to:
- properly document how to configure outbound email;
- test outbound email configuration in the setup mode;
- provide basic daemon documentation;
- document that phabricator.base-uri is required for all installs.
Test Plan:
read documentation, jumped through all the setup branches to test configuration
error detection
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran, rm
CC: ccheever, aran
Differential Revision: 276
Summary:
Currently, the Javelin linter fails completley if this binary is missing.
However, it's hard to build and not critical so just issue a warning.
Eventually we can document this better and make the build easier, but the
current behavior is pretty unfriendly so make it smoother until the state of the
world can be improved.
Test Plan:
Removed the binary and ran "arc lint --lintall" against multiple Javelin paths.
Received one warning. Restored the binary and ran with "--trace", got no
warnings and verified that the binary was running.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran, tomo
CC: aran, jungejason
Differential Revision: 265
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:
Previously, Remarkup allowed you to paste in an image URI and get an inline
image. However, it did this by hotlinking the image which isn't so hot in an
open source product.
Restore this feature, but use image proxying instead. The existing image macro
code does most of the work.
There is a mild security risk depending on the network setup so I've left this
default-disabled and made a note about it. It should be safe to enable for
Facebook.
Test Plan:
Pasted in image and non-image links, got reasonable behavior. Verified proxying
appears to work. Verified that file:// shenanigans produce 400.
Reviewed By: tuomaspelkonen
Reviewers: aran, jungejason, tuomaspelkonen
Commenters: cpiro
CC: aran, cpiro, tuomaspelkonen
Differential Revision: 214
Summary:
Allow Lisk to be put into process-isolated mode which establishes
only isolated connections. By default, put it into this mode when running
unit tests. Build some simple unit tests around object insertion and
updating.
NOTE: The one flaw in this is that $dao->establishConnection() still
punches through the isolation layer. I need to do an API change to fix this
though so I'm holding it for now. It will probably just rename getConnection()
to establishConnection() and then rename establishConnection() to something
scary like establishLiveExternalConnection().
Test Plan:
Ran unit tests.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran, epriestley
Differential Revision: 194
Summary:
This provides a new connection which doesn't connect to
anything, so effects can be isolated to the current process (for
unit testing).
Test Plan:
Ran unit tests.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran, epriestley
Differential Revision: 193
Summary:
The correct name of this key is 'github.application-secret', not
'github.secret'. Make DarkConsole check that all the masked keys exist to
prevent this from happening again. This isn't super important since this
is just intended to protected against casual security lapses (taking a
screenshot with DarkCnosole's "Config" tab open, for instance) but it's easy
to check for so it seems worthwhile to get right.
Test Plan:
Loaded page without the actual config file change, got an exception.
Fixed the config, reloaded the page, good news goats (really trying to get this
to catch on since goats are adorable).
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran
Differential Revision: 189
Summary:
Just minor bookkeeping, but the current regexp is too liberal and
will match things which can't possibly be revision hashes.
Test Plan:
Typed things which should and shouldn't be revision links, they
got handled properly.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 185
Summary:
Fixed the image macro regex not to use '-' as the separator.
Also minor improvement to randomon.
Test Plan:
Tried different image marcors.
Reviewed By: jungejason
Reviewers: jungejason
CC: epriestley, jungejason
Differential Revision: 153
Summary: add --load-phutil-library for libraries other than phabricator
and libphutil.
Test Plan: launch a daemon in facebook such as
PhabricatorDifferentialCommentDaemon.
Reviewers: epriestley, tuomaspelkonen
CC:
Differential Revision: 146
Summary: See D133. Workers can also be subject to the same race, invert the
row relationship in the same way.
Test Plan: Launched repository master daemons and some taskmasters and used
the Daemon console to veify that they were able to process tasks. Manually
checked the database to make sure data got linked correctly and that new data
was inserted correctly.
Reviewers: jungejason
CC: tuomaspelkonen
Differential Revision: 135
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:
Added long waited image macro support for differential and others.
Test Plan:
Tried a couple of different macros and made sure they appear nicely
in the comment preview. Made sure that the normal comments are shown
correctly.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, tuomaspelkonen, epriestley
Differential Revision: 129
Summary: Basic scaffolding for repository tracking, plus daemon infrastructure
(Timelines, Cursors) and some fixes (memory usage, mysql_connect() junk).
Test Plan: parsed Javelin git commit history via daemon
Reviewers:
CC:
Summary: Autolink Differential and Maniphest objects.
Test Plan: Typed "D12345" and "T12345" into the Differential comment preview,
got links. Typed "http://www.elsewhere.com/D12345" and got a single link to
that URI, not a mess where the D12345 part linked incorrectly.
Reviewers: aran
CC:
Differential Revision: 35
