Summary:
Ref T12020. Ref T11114. If we continue here on a mention, we try to generate `$old`, which requires reviewers to be attached. They won't be for simple codepaths like mentions.
Instead, just bail early: we don't need to do anything anyway since we can't possibly find any more errors with zero transactions.
Test Plan: Mentioned a revision on a task.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T11114, T12020
Differential Revision: https://secure.phabricator.com/D17059
Summary: Reorgaizes the CSS here a bit, by object list style, adds in a new drag ui class, which will be used in menu ordering.
Test Plan:
Workboards, Home Apps.
{F2126266}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17057
Summary: Fixes T12015. This is weird and probably got copy/pasted from something else that was also being weird, since the methods were empty and I previously removed them.
Test Plan: Edited a book in Diviner.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T12015
Differential Revision: https://secure.phabricator.com/D17056
Summary:
Ref T11114. This replaces the old edit controller with a new one based entirely on EditEngine.
This removes the CustomFieldEditEngineExtension hack for Differential, since remaining field types are fairly straightforward and work with existing EditEngine support, as far as I can tell.
Test Plan:
- Created a revision via web diffs.
- Updated a revision via web diffs.
- Edited a revision via web.
- Edited nonstandard custom fields ("Blame Revision", "JIRA Issues").
- Created a revision via CLI.
- Updated a revision via CLI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17054
Summary: Ref T11114. Much of this is around making the "comment-while-updating" flow work correctly.
Test Plan:
- Created new diffs by copy/pasting, then:
- used one to create a new revision;
- used one to update an existing revision, with a comment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17053
Summary: We're throwing here when we actually want to return `null` so we make it into custom field handling code. See Conpherence.
Test Plan: Found a failing task and re-executed it with `bin/worker execute --id <id>`; after this change, it didn't fatal.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17051
Summary: Ref T11114. This one is a bit more complex, but I think I covered everything.
Test Plan:
- Added reviewers.
- Removed reviewers.
- Made reviewers blocking.
- Made reviewers nonblocking.
- Tried to make the author a reviewer.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17050
Summary: Ref T11114. The only real trick here is that we respect configuration in `differential.fields`.
Test Plan: Turned plan on and off, tried to remove the plan, edited the plan.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17048
Summary: Ref T11114. These are unambiguous and always-enabled.
Test Plan: {F2117777}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17047
Summary:
Ref T11114. Currently, all of Differential is extremely custom CustomFields. I want to back away from that somewhat and leverage more EditEngine / ModularTransactions infrastructure.
This allows EditEngine, ModularTransactions, and CustomFields to coexist in an uneasy peace. The "EditPro" controller applies a //different edit// than the CustomFields do, but everything works out in the end. I think.
Hopefully the horrible mess I am creating here will be short-lived.
Test Plan:
- Edited a revision with the normal editor.
- Edited a revision with the pro editor.
- Created a revision with `arc diff`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17044
Summary: Ref T11114. This doesn't really support anything yet, but technically works if you manually go to `/editpro/`.
Test Plan: {F2117302}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17043
Summary: Ref T10967. This makes room for a `DifferentialReviewer` object which can be a real storage table.
Test Plan: Grepped for `DifferentialReviewer`, browsed Differential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17041
Summary: Ref T8475. This gets rid of most of the old "legacy hunk" code. I'll nuke the rest (and drop the old table) once we're more sure that we're in the clear.
Test Plan: Browsed Differential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8475
Differential Revision: https://secure.phabricator.com/D17040
Summary:
Fixes T7899. If you create or update a revision and type an enormously long first line, we currently fatal trying to insert it into the database.
This text is only used to show a single-line summary of the diff in the "History" tab, which should probably be updated anyway. For now, stop fataling.
Test Plan:
Uploaded a diff with the description "MMMM..." (thousands of them).
Before patch: fatal on description being too long.
After patch: beautiful "MMMM" summary.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7899
Differential Revision: https://secure.phabricator.com/D17038
Summary:
Fixes T12013. Send either "Content-Length" or enable output compression, but not both.
Prefer compression for static resources (CSS, JS, etc).
Test Plan: Ran `curl -v ...`, no longer saw responses with both compression and `Content-Length`.
Reviewers: chad, avivey
Reviewed By: avivey
Subscribers: avivey
Maniphest Tasks: T12013
Differential Revision: https://secure.phabricator.com/D17045
Summary: Allows users set an icon (for reuse on upcoming home) for their dashboard based on 16 descriminating choices.
Test Plan: Create a new dashboard, set new icon. Edit an existing dashboard, set icon.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17042
Summary: Adds authorPHID to panels so we can default to the panels you made.
Test Plan: Run upgrade, visit manage panels, see my panels. Create a new panel. Edit a panel.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17036
Summary: Adds an authorPHIDs, populates olds ones.
Test Plan: Make a new Dashboard, see that I created it.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17022
Summary: Ref T571. This was accidentally left behind in D12266.
Test Plan: Used {key command F} to search for "bulk".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T571
Differential Revision: https://secure.phabricator.com/D17034
Summary: Fixes T11999. These are actual panels (SettingsPanel) which are panelley so it's OK.
Test Plan: Clicked "Customize Menu..." on Home.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11999
Differential Revision: https://secure.phabricator.com/D17032
Summary: Cleans up the UI on the page here, uses two column layout, places actions as actionlist instead of dropdown. Changes edit pages to dialogs.
Test Plan: Add an application, divider, link, and facts to a menu page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17030
Summary:
I'm about 90% sure this fixes the intermittent test failure on `testObjectSubscribersPolicyRule()` or whatever.
We use `spl_object_hash()` to identify objects when passing hints about policy changes to policy rules. This is hacky, and I think it's the source of the unit test issue.
Specifically, `spl_object_hash()` is approximately just returning the memory address of the object, and two objects can occasionally use the same memory address (one gets garbage collected; another uses the same memory).
If I replace `spl_object_hash()` with a static value like "zebra", the test failure reproduces.
Instead, sneak an object ID onto a runtime property. This is at least as hacky but shouldn't suffer from the same intermittent failure.
Test Plan: Ran `arc unit --everything`, but I never got a reliable repro of the issue in the first place, so who knows.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17029
Summary: Ref T11957.
Test Plan:
- Viewed an existing project profile.
- Viewed a user profile.
- Created a new project.
- Edited a profile menu.
- Added new profile items.
- Grepped for renamed symbols.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11957
Differential Revision: https://secure.phabricator.com/D17028
Summary:
Ref T11957. This renames the Configuration storage, transaction, query, and PHID type.
No rename on the actual menu item types yet, that's next (and should be the end of this, I think).
Test Plan:
- Viewed projects.
- Viewed profiles.
- Edited a project menu.
- Grepped for all renamed symbols, I think?
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11957
Differential Revision: https://secure.phabricator.com/D17027
Summary:
- Fixes T11995. This got moved but I missed renaming this callsite.
- Fixes T11993. If you have valid credentials, but haven't run `storage upgrade` yet, we can hit this exception during setup. Just ignore it instead.
Test Plan:
- Saved global settings, no more fatal.
- Changed `storage-namespace` to junk, loaded web UI with valid database credentials.
{F2106358}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11993, T11995
Differential Revision: https://secure.phabricator.com/D17024
Summary: Allows applications to be added as profile menu items
Test Plan: Add an application to a project, see menu item, click on menu. Uninstall application, see menu without application.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17016
Summary: Fixes T11982. If an install is not public, the registering user may not be able to see the inviting user.
Test Plan: {F2097656}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11982
Differential Revision: https://secure.phabricator.com/D17015
Summary: Fixes T11544. Attempt to detect if we're on a tiny, burstable-CPU AWS instance and complain.
Test Plan:
- Completely faked this locally.
- Hit the URI on an EC2 instance to check that it's correct (got back "m3.large", since that was the instance class).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11544
Differential Revision: https://secure.phabricator.com/D17014
Summary:
Fixes T11955. Currently, milestones have an internal name of "Parent (Milestone) ...".
This makes them look like they're prefix matches for "Parent", but they're actually prefix matches for "Milestone".
Reorder the names so that the internal name is "Milestone Parent ...".
Test Plan: Created a project "AAA" with milestone "BBB". Searched for "AAA", found "AAA" and milestone "AAA (BBB)".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11955
Differential Revision: https://secure.phabricator.com/D17013
Summary:
Fixes T11955. Milestone names are currently tokenizing and indexing awkwardly. For example, "A (B C D)" becomes the tokens "A", "(B", "C" and "D)".
The token "(B" can't be searched for since "(" is tokenized on the client.
Instead, tokenize "A (B C D)" into "A", "B", "C", "D".
Test Plan:
- Added unit tests.
- Used `bin/search index --type project --force` to reindex.
- Searched for "A", "B", "C", "D", etc., for real examples.
- Now, found milestones more consistently.
- Also serached for `viewer()`, `members()`, etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11955
Differential Revision: https://secure.phabricator.com/D17012
Summary:
Ref T11553. With some regularity, users make various configuration mistakes which we can detect by making a request to ourselves.
I use a magical header to make this request because we want to test everything else (parameters, path).
- Fixes T4854, probably. Tries to detect mod_pagespeed by looking for a header. This is a documentation-based "fix", I didn't actually install mod_pagespeed or formally test this.
- Fixes T6866. We now test for parameters (e.g., user somehow lost "QSA").
- Ref T6709. We now test that stuff is decoded exactly once (e.g., user somehow lost "B").
- Fixes T4921. We now test that Authorization survives the request.
- Fixes T2226. Adds a setup check to determine whether gzip is enabled on the web server, and attempts to enable it at the PHP level.
- Fixes `<space space newline newline space><?php` in `preamble.php`.
Test Plan: Tested all of these setup warnings, although mostly by faking them.
Reviewers: joshuaspence, chad
Reviewed By: chad
Subscribers: Korvin
Maniphest Tasks: T4854, T4921, T6709, T6866, T11553, T2226
Differential Revision: https://secure.phabricator.com/D12622
Summary:
Ref T11954. In cluster configurations, we get repository information by making HTTP calls over Conduit.
These are slower than local calls, so clustering imposes a performance penalty. However, we can use futures and parallelize them so that clustering actually improves overall performance.
When not running in clustered mode, this just makes us run stuff inline.
Test Plan:
- Browsed Git, Mercurial and Subversion repositories.
- Locally, saw a 700ms wall time page drop to 200ms.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D17009
Summary:
Fixes T11960. In D16998 I removed some code which validated settings on read to improve performance, but lost this replacement validation in shuffling the patch stack.
This restores similar validation before we write the cache. This has the same effect, it's just faster.
Also, bump the cache key to wipe out anything that got bitten (like my account on `secure` rendering dates wrong).
Test Plan:
- Edited settings, verified the edits held.
- Faked invalid settings, saw the check throw exceptions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11960
Differential Revision: https://secure.phabricator.com/D17008
Summary:
Ref T11954. Especially with higher-latency file stores like S3, we can spend a lot of time reading README data and then pulling it out of file storage.
Instead, cache it.
Test Plan: Browsed a repostory with a README, saw faster pages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D17002
Summary:
Ref T11954. This is kind of complex and I'm not sure I want to actually land it, but it gives us a fairly good improvement for clustered repositories so I'm leaning toward moving forward.
When we make (or receive) clustered repository requests, we must first load a bunch of stuff out of Almanac to figure out where to send the request (or if we can handle the request ourselves).
This involves several round trip queries into Almanac (service, device, interfaces, bindings, properties) and generally is fairly slow/expensive. The actual data we get out of it is just a list of URIs.
Caching this would be very easy, except that invalidating the cache is difficult, since editing any binding, property, interface, or device may invalidate the cache for indirectly connected services and repositories.
To address this, introduce `PhabricatorCacheEngine`, which is an extensible engine like `PhabricatorDestructionEngine` for propagating cache updates. It has two modes:
- Discover linked objects (that is: find related objects which may need to have caches invalidated).
- Invalidate caches (that is: nuke any caches which need to be nuked).
Both modes are extensible, so third-party code can build repository-dependent caches or whatever. This may be overkill but even if Almanac is the only thing we use it for it feels like a fairly clean solution to the problem.
With `CacheEngine`, make any edit to Almanac stuff propagate up to the Service, and then from the Service to any linked Repositories.
Once we hit repositories, invalidate their caches when Almanac changes.
Test Plan:
- Observed a 20-30ms performance improvement with `ab -n 100`.
- (The main page making Conduit calls also gets a performance improvement, although that's a little trickier to measure directly.)
- Added debugging code to the cache engine stuff to observe the linking and invalidation phases.
- Made invalidation throw; verified that editing properties, bindings, etc, properly invalidates the cache of any indirectly linked repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D17000
Summary:
Ref T11954. This reduces how much work we need to do to load settings, particularly for Conduit (which currently can not benefit directly from the user cache, because it loads the user indirectly via a token).
Specifically:
- Cache builtin defaults in the runtime cache. This means Phabricator may need to be restarted if you change a global setting default, but this is exceptionally rare.
- Cache global defaults in the mutable cache. This means we do less work to load them.
- Avoid loading settings classes if we don't have to.
- If we missed the user cache for settings, try to read it from the cache table before we actually go regenerate it (we miss on Conduit pathways).
Test Plan: Used `ab -n100 ...` to observe a ~6-10ms performance improvement for `user.whoami`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D16998
Summary:
Ref T11954. I want to store some lists/arrays in the mutable (database) cache, but it only supports string storage.
Provide a serializing wrapper which flattens when values are written and expands them when they're read.
Test Plan: Used by D16997. See that revision.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D16999
Summary:
Ref T11954. When we query for Conduit tokens, we load the associated objects (users) by PHID.
Currently, querying objects by PHID requires us to load every PHIDType class, when we can know which specific classes we actually need (e.g., just `UserPHIDType`, if only user PHIDs are present in the query).
Use PhabricatorCachedClassMapQuery to reduce the number of classes we load on this pathway.
Test Plan:
- Used `ab -n100` to roughly measure a ~5% performance improvement?
- This measurement feels a little flimsy but the XHProf profile is cleaner, at least.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D16997
Summary: Ref T11954. This old query class can use slightly more modern code.
Test Plan: Ran Conduit methods, verified results are unchanged.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D16996
Summary: Ref T11954. Depends on D16994. This implements the Conduit method cache described in that revision for a small global Conduit performance improvement.
Test Plan: Verified Conduit has the same behavior at lower cost. See D16994 for details.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D16995
Summary:
Ref T11954. Depends on D16993. We have a couple of "look up the class for this key" queries which are costly enough to show up on a profile.
These aren't huge wins, but they're pretty easy. We currently do this like this:
```
$class_map = load_every_subclass();
return idx($class_map, $key);
```
However, we don't need to load EVERY subclass if we're only looking for, say, the Conduit method subclass which implements `user.whoami`. This allows us to cache that map and find the right class efficiently.
This cache is self-validating and completely safe even in development.
Test Plan:
- Used `curl` to make queries to `user.whoami`, verified that content was identical before and after the change.
- Used `ab -n100` to roughly measure 99th percentile time, which dropped from 74ms to 65ms. This is a small improvement (13% in the best case, here) but it benefits every Conduit method call.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D16994
Summary:
Ref T11954. Depends on D16992. We have some data which can be generated and cached at runtime. Three examples are:
- Class map from Conduit method names to implementing classes.
- Class map from PHID types to implementing classes.
- The main routing map.
None of these are huge wins but they impose global costs and can be shaved down through caching without introducing an enormous amount of new complexity.
The cost to these maps is that sometimes you'll need to restart your webserver, even in development mode if these caches are active. However, in some cases these changes are very rare, and in other cases we can just leave the cache disabled in development mode without a huge complexity cost.
Specifically, the Conduit/PHID type class maps are self-validating and can not go bad, even in development mode.
The routing map will be able to, but I plan to just disable it in development mode.
This provides a general-purpose pure APC cache stack for storing this data.
Test Plan: See future changes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11954
Differential Revision: https://secure.phabricator.com/D16993
Summary: Ref T929. When viewing a branch, show a few recent differences from the default branch (usually, "master").
Test Plan: {F2079220}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T929
Differential Revision: https://secure.phabricator.com/D16991
Summary: Ref T929. We've made some UI updates since D15330.
Test Plan: {F2079125}
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T929
Differential Revision: https://secure.phabricator.com/D16990
Summary:
This shows the commits list only (Actual `git diff` will show up at a later date).
The inputs are left as text-fields, to allow the form to accept anything that can be resolved. The form is GET, to allow sharing URIs.
The conduit method response array is compatible with that of `diffusion.historyquery`, to make it easy to build
the "history" table.
The hardest part here was, of course, Naming. I think "from" and "onto" are unconfusing, and I'm fairly confident that the "to merge"
instructions are in sync with the actual content of the page.
Test Plan: Look at several "compare" views, with various values of "from" and "onto".
Reviewers: #blessed_reviewers!, epriestley
Subscribers: caov297, 20after4, Sam2304, reardencode, baileyb, chad, Korvin
Maniphest Tasks: T929
Differential Revision: https://secure.phabricator.com/D15330
Summary:
Persona is going to be decommed November 30th, 2016.
It is highly unlikely that anyone is currently using persona as a real
login method at this point.
Test Plan: tried locally to add auth adapter.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D16371
