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
