Summary:
Depends on D19955. Ref T920. Ref T5969. Update Postmark to accept new Message objects. Also:
- Update the inbound whitelist.
- Add a little support for `media` configuration.
- Add a service call timeout (see T5969).
- Drop the needless word "Implementation" from the Adapter class tree. I could call these "Mailers" instead of "Adapters", but then we get "PhabricatorMailMailer" which feels questionable.
Test Plan: Used `bin/mail send-test` to send mail via Postmark with various options (mulitple recipients, text vs html, attachments).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5969, T920
Differential Revision: https://secure.phabricator.com/D19956
Summary:
Ref T920. Over time, mail has become much more complex and I think considering "mail", "sms", "postcards", "whatsapp", etc., to be mostly-the-same is now a more promising avenue than building separate stacks for each one.
Throw away all the standalone SMS code, including the Twilio config options. I have a separate diff that adds Twilio as a mail adapter and functions correctly, but it needs some more work to bring upstream.
This permanently destroys the `sms` table, which no real reachable code ever wrote to. I'll call this out in the changelog.
Test Plan:
- Grepped for `SMS` and `Twilio`.
- Ran storage upgrade.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19939
Summary:
Depends on D19906. Ref T13222. This isn't going to win any design awards, but make the "wait" and "answered" elements a little more clear.
Ideally, the icon parts could be animated Google Authenticator-style timers (but I think we'd need to draw them in a `<canvas />` unless there's some clever trick that I don't know) or maybe we could just have the background be like a "water level" that empties out. Not sure I'm going to actually write the JS for either of those, but the UI at least looks a little more intentional.
Test Plan:
{F6070914}
{F6070915}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19908
Summary:
Depends on D19903. Ref T13222. This was a Facebook-specific thing from D6202 that I believe no other install ever used, and I'm generally trying to move away from the old "event" system (the more modern modular/engine patterns generally replace it).
Just drop support for this. Since the constant is being removed, anything that's actually using it should break in an obvious way, and I'll note this in the changelog.
There's no explicit replacement but I don't think this hook is useful for anything except "being Facebook in 2013".
Test Plan:
- Grepped for `TYPE_AUTH_WILLLOGIN`.
- Logged in.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19904
Summary:
Depends on D19919. Ref T11351. This method appeared in D8802 (note that "get...Object" was renamed to "get...Transaction" there, so this method was actually "new" even though a method of the same name had existed before).
The goal at the time was to let Harbormaster post build results to Diffs and have them end up on Revisions, but this eventually got a better implementation (see below) where the Harbormaster-specific code can just specify a "publishable object" where build results should go.
The new `get...Object` semantics ultimately broke some stuff, and the actual implementation in Differential was removed in D10911, so this method hasn't really served a purpose since December 2014. I think that broke the Harbormaster thing by accident and we just lived with it for a bit, then Harbormaster got some more work and D17139 introduced "publishable" objects which was a better approach. This was later refined by D19281.
So: the original problem (sending build results to the right place) has a good solution now, this method hasn't done anything for 4 years, and it was probably a bad idea in the first place since it's pretty weird/surprising/fragile.
Note that `Comment` objects still have an unrelated method with the same name. In that case, the method ties the `Comment` storage object to the related `Transaction` storage object.
Test Plan: Grepped for `getApplicationTransactionObject`, verified that all remaining callsites are related to `Comment` objects.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19920
Summary:
Depends on D19918. Ref T11351. In D19918, I removed all calls to this method. Now, remove all implementations.
All of these implementations just `return $timeline`, only the three sites in D19918 did anything interesting.
Test Plan: Used `grep willRenderTimeline` to find callsites, found none.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19919
Summary:
Ref T13222. Ref T13225. We store a digest of the session key in the session table (not the session key itself) so that users with access to this table can't easily steal sessions by just setting their cookies to values from the table.
Users with access to the database can //probably// do plenty of other bad stuff (e.g., T13134 mentions digesting Conduit tokens) but there's very little cost to storing digests instead of live tokens.
We currently digest session keys with HMAC-SHA1. This is fine, but HMAC-SHA256 is better. Upgrade:
- Always write new digests.
- We still match sessions with either digest.
- When we read a session with an old digest, upgrade it to a new digest.
In a few months we can throw away the old code. When we do, installs that skip upgrades for a long time may suffer a one-time logout, but I'll note this in the changelog.
We could avoid this by storing `hmac256(hmac1(key))` instead and re-hashing in a migration, but I think the cost of a one-time logout for some tiny subset of users is very low, and worth keeping things simpler in the long run.
Test Plan:
- Hit a page with an old session, got a session upgrade.
- Reviewed sessions in Settings.
- Reviewed user logs.
- Logged out.
- Logged in.
- Terminated other sessions individually.
- Terminated all other sessions.
- Spot checked session table for general sanity.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13225, T13222
Differential Revision: https://secure.phabricator.com/D19883
Summary: Fixes T13218. We have no more callers to any of this and can get rid of it forever.
Test Plan: Grepped for all four API methods, `LiskDAOSet`, and `inSet`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13218
Differential Revision: https://secure.phabricator.com/D19879
Summary: Ref T13218. This is the last public-facing API call for `loadRelatives/loadOneRelative`. This just "primed" objects to make the other calls work and had no direct effects.
Test Plan:
- Ran `bin/fact analyze`.
- Used `bin/storage upgrade -f --apply` to apply `20181031.board.01.queryreset.php`, which uses `LiskMigrationIterator`.
- Browsed user list.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13218
Differential Revision: https://secure.phabricator.com/D19878
Summary:
Ref T13222. See PHI683. Currently, you can "Change subtype..." via Conduit and the bulk editor, but not via the comment action stack or edit forms.
In PHI683 an install is doing this often enough that they'd like it to become a first-class action. I've generally been cautious about pushing this action to become a first-class action (there are some inevitable rough edges and I don't want to add too much complexity if there isn't a use case for it) but since we have evidence that users would find it useful and nothing has exploded yet, I'm comfortable taking another step forward.
Currently, `EditEngine` has this sort of weird `setIsConduitOnly()` method. This actually means more like "this doesn't show up on forms". Make it better align with that. In particular, a "conduit only" field can already show up in the bulk editor, which is goofy. Change this to `setIsFormField()` and convert/simplify existing callsites.
Test Plan:
There are a lot of ways to reach EditEngine so this probably isn't entirely exhaustive, but I think I got pretty much anything which is likely to break:
- Searched for `setIsConduitOnly()` and `getIsConduitOnly()`, converted all callsites to `setIsFormField()`.
- Searched for `setIsLockable()`, `setIsReorderable()` and `setIsDefaultable()` and aligned these calls to intent where applicable.
- Created an Almanac binding.
- Edited an Almanac binding.
- Created an Almanac service.
- Edited an Almanac service.
- Edited a binding property.
- Deleted a binding property.
- Created and edited a badge.
- Awarded and revoked a badge.
- Created and edited an event.
- Made an event recurring.
- Created and edited a Conpherence thread.
- Edited and updated the diff for a revision.
- Created and edited a repository.
- Created and disabled repository URIs.
- Created and edited a blueprint.
- Created and edited tasks.
- Created a paste, edited/archived a paste.
- Created/edited/archived a package.
- Created/edited a project.
- Made comments.
- Moved tasks on workboards via comment action stack.
- Changed task subtype via comment action stack.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19842
Summary:
Ref T13216. See PHI916. Harbormaster builds may be long-running, particularly if they effectively wrap `ssh ... ./run-huge-build.sh`. If we spend more than a few seconds waiting for futures to resolve, close idle database connections.
The general goal here is to reduce the held connection load for installs with a very large number of test runners.
Test Plan: Added debugging code to `phlog()` closures, saw connections closed while running builds.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19824
Summary: Depends on D19810. Ref T13217. Ref T13216. I mostly used `grep implode | grep OR` and `grep implode | grep AND` to find these -- not totally exhaustive but should be a big chunk of the callsites that are missing `%LO` / `%LA`.
Test Plan:
These are tricky to test exhaustively, but I made an attempt to hit most of them:
- Browsed Almanac interfaces.
- Created/browsed Calendar events.
- Enabled/disabled/showed the lock log.
- Browsed repositories.
- Loaded Facts UI.
- Poked at Multimeter.
- Used typeahead for users and projects.
- Browsed Phriction.
- Ran various fulltext searches.
Not sure these are reachable:
- All the lint stuff might be dead/unreachable/nonfunctional?
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13217, T13216
Differential Revision: https://secure.phabricator.com/D19814
Summary:
Ref T13216. See PHI970. Ref T13054. See some discussion in T13216.
When a Harbormaster Buildable object is first created for a Diff, it has no `containerPHID` since the revision has not yet been created.
We later (after creating a revision) send the Buildable a message telling it that we've added a container and it should re-link the container object.
Currently, we send this message in `applyExternalEffects()`, which runs inside the Differential transaction. If Harbormaster races quickly enough, it can read the `Diff` object before the transaction commits, and not see the container update.
Add a `didCommitTransaction()` callback after the transactions commit, then move the message code there instead.
Test Plan:
- See T13216 for substantial evidence that this change is on the right track.
- Before change: added `sleep(15)`, reproduced the issue reliably.
- After change: unable to reproduce issue even with `sleep(15)` (the `containerPHID` always populates correctly).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216, T13054
Differential Revision: https://secure.phabricator.com/D19807
Summary:
Ref T13216. Ref T13217. Depends on D19800. This fixes all of the remaining query warnings that pop up when you run "arc unit --everything".
There's likely still quite a bit of stuff lurking around, but hopefully this covers a big set of the most common queries.
Test Plan: Ran `arc unit --everything`. Before change: lots of query warnings. After change: no query warnings.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217, T13216
Differential Revision: https://secure.phabricator.com/D19801
Summary: Depends on D19789. Ref T13217. Continue updating things to use the new %Q-flavored conversions instead of smushing a bunch of strings together.
Test Plan: Browsed around, far fewer errors. These changes are largely mechanical in nature.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19790
Summary: Depends on D19785. Ref T13217. This converts many of the most common clause construction pathways to the new %Q / %LQ / %LO / %LA / %LJ semantics.
Test Plan: Browsed around a bunch, saw fewer warnings and no obvious behavioral errors. The transformations here are generally mechanical (although I did them by hand).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: hach-que
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19789
Summary: Depends on D19784. Ref T13217. Reduce uses of unsafe `%Q` in SELECT construction.
Test Plan: This reduces the number of safety warnings when loading Phabricator home from ~900 to ~800.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19785
Summary:
Ref T13217. This method is slightly tricky:
- We can't safely return a string: return an array instead.
- It no longer makes sense to accept glue. All callers use `', '` as glue anyway, so hard-code that.
Then convert all callsites.
Test Plan: Browsed around, saw fewer "unsafe" errors in error log.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19784
Summary: Depends on D19796. Simplify some timing code by using phutil_microseconds_since() instead of duplicate casting and arithmetic.
Test Plan: Grepped for `1000000` to find these. Pulled, pushed, made a conduit call. This isn't exhaustive but it should be hard for these to break in a bad way since they're all just diagnostic.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19797
Summary:
Ref T13210. Ref T11908. Add some basic test coverage for the new "%R" introduced in D19764, then convert LiskDAO to implement the "Database + Table Ref" interface.
To move forward, we need to convert all of these (where `%T` is not a table alias):
```counterexample
qsprintf($conn, '... %T ...', $thing->getTableName());
```
...to these:
```
qsprintf($conn, '... %R ...', $thing);
```
The new code is a little simpler (no `->getTableName()` call) which is sort of nice. But we also have a //lot// of `%T` so this is probably going to take a while.
(I'll hold this until after the release cut.)
Test Plan:
- Ran unit tests.
- Browsed around and edited some objects without issues. This change causes a reasonably large percentage of our total queries to use the new code since the LiskDAO builtin queries are some of the most commonly-constructed queries, although there are still ~700 callsites which need to be examined for possible conversion.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210, T11908
Differential Revision: https://secure.phabricator.com/D19765
Summary:
Ref T13210. See PHI930. This translation is wrong: the parameter is a comma-separated list as a string, but the USEnglish translation provides alternatives. We can't select among alternatives based on a random string (it isn't a plurality value to let us select "chair" vs "chairs", and isn't a gender value to let us select "his profile" vs "her profile") so we get an error.
But the string itself is also misleading, since "bin/phd log --id A --id B --id C" will say "none of these are valid" if //any// of them are invalid.
Instead, just tell the user explicitly about the first problem.
Test Plan:
- Ran `bin/phd log --id` with good (got logs) and bad IDs (got sensible error).
- Ran `bin/phd log` with any logs (got logs) and (simluated) without any logs (got error).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19755
Summary:
Ref T13210. Minor usability improvements to "bin/bulk export":
- Allow `--class task` to work (previously, only `--class ManiphestTaskSearchEngine` worked).
- If you run `--query jXIlzQyOYHPU`, don't require `--class`, since the query identifies the class on its own.
- Allow users to call `--query A --query B --query C` and get a union of all results.
Test Plan:
- Ran `--class task`, `--query A --query B`, `--query X` (with no `--class`), got good results.
- Ran various flavors of bad combinations (queries from different engines, invalid engines, query and class differing, ambiguous/invalid `--class` name) and got sensible errors.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210
Differential Revision: https://secure.phabricator.com/D19738
Summary:
Ref T13202. See PHI889. If the lock log is enabled, we can try to offer more details about lock holders.
When we fail to acquire a lock:
- check for recent acquisitions and suggest that this is a bottleneck issue;
- if there are no recent acquisitions, check for the last acquisition and print details about it (what process, how long ago, whether or not we believe it was released).
Test Plan:
- Enabled the lock log.
- Changed the lock wait time to 1 second.
- Added a `sleep(10)` after grabbing the lock.
- In one window, ran a Conduit call or a `git fetch`.
- In another window, ran another operation.
- Got useful/sensible errors for both ssh and web lock holders, for example:
> PhutilProxyException: Failed to acquire read lock after waiting 1 second(s). You may be able to retry later. (This lock was most recently acquired by a process (pid=12609, host=orbital-3.local, sapi=apache2handler, controller=PhabricatorConduitAPIController, method=diffusion.rawdiffquery) 3 second(s) ago. There is no record of this lock being released.)
> PhutilProxyException: Failed to acquire read lock after waiting 1 second(s). You may be able to retry later. (This lock was most recently acquired by a process (pid=65251, host=orbital-3.local, sapi=cli, argv=/Users/epriestley/dev/core/lib/phabricator/bin/ssh-exec --phabricator-ssh-device local.phacility.net --phabricator-ssh-key 2) 2 second(s) ago. There is no record of this lock being released.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19702
Summary:
Ref T13195. Fixes T8573. When you're adding inlines to your own stuff, mark them "Done" by default. You can unmark them as "Done" if you're legitimately leaving TODOs for yourself, although I think this is unusual.
(If this turns out to be less unusual than I think, we could consider an alternate rule: mark replies by the author as "Done" by default.)
Test Plan: Added some inlines as an author and a non-author. Saw my author inlines marked as "Done" by default. Submitted them; unmarked and submittted them.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195, T8573
Differential Revision: https://secure.phabricator.com/D19635
Summary:
Ref T13195. Ref T8573. This allows reviewers to mark their own inline comments as "Done" before they submit them.
If you're leaving a non-actionable comment like "this is good", you can pre-check "Done" to give the author a hint that you don't expect any response.
Test Plan: On revisions and commits, added inlines as the author and a reviewer/auditor. Marked them done/not-done before submitting. As author, marked the not-done ones done after submitting. Checked preivews, toggled done/not done states.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195, T8573
Differential Revision: https://secure.phabricator.com/D19634
Summary:
Ref T13195. Ref T8573. The inline comment controllers currently use outdated `$user = $this->getRequest()->getUser()` calls.
Instead, use `$viewer = $this->getViewer()`.
This is just a small consistency update with no behavioral changes.
Test Plan: Viewed and added inlines in Differential and Diffusion.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195, T8573
Differential Revision: https://secure.phabricator.com/D19633
Summary:
Ref T13164. See PHI823. (See that issue for some more details and discussion.)
Add aural labels to various buttons which were missing reasonable aural labels.
The "Search" button (magnifying glass in the global search input) had an entire menu thing inside it. I moved that one level up and it doesn't look like it broke anything (?). All the other changes are pretty straightforward.
Test Plan:
{F5806497}
{F5806498}
- Will follow up on the issue to make sure things are in better shape for the reporting user.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19594
Summary:
Ref T13164. PHI805 incidentally includes some `bin/storage probe` output for 100GB+ tables which renders wrong.
We have the tools to render it properly, so stop doing this manually and let ConsoleTable figure out the alignment.
Test Plan:
Faked very large table sizes, ran `bin/storage probe`:
{F5785946}
(Then, un-faked the very large table sizes and ran it again, got sensible output.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19567
Summary:
See PHI785. Ref T13164. In this case, an install wants to receive mail via Mailgun, but not configure it (DKIM + SPF) for outbound mail.
Allow individual mailers to be marked as not supporting inbound or outbound mail.
Test Plan:
- Added and ran unit tests.
- Went through some mail pathways locally, but I don't have every inbound/outbound configured so this isn't totally conclusive.
- Hit `bin/mail send-test` with a no-outbound mailer.
- I'll hold this until after the release cut so it can soak on `secure` for a bit.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19546
Summary: I've pulled up this code probably three different times to make sure that the big scary warning does, in fact, still get printed even when passing `--unitest-fixtures` to `bin/storage destroy`. Make the warning message less scary if only removing test data.
Test Plan: Ran with and without `--unitest-fixtures` and saw expected warnings. After agreeing to warnings, test data was deleted as expected. Did not test `bin/storage destroy` without `--unittest-fixtures`.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19535
Summary:
Ref PHI778. In D18492, I added support for parsing this operator, but did not actually implement it in the query engine.
Implementation is fairly straightforward. This supports querying for objects by exact title with `title:="exact title"`. This is probably a bad idea, but sometimes maybe useful anyway.
Test Plan: Queried for `title:="xxx"`, found only exact matches.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: ahoffer2
Differential Revision: https://secure.phabricator.com/D19529
Summary:
Ref T13168. I'm not sure how this worked before, but I ran into this issue on my new laptop.
SiteSource accesses `PhabrictatorEnv::getEnvConfig('phabricator.base-uri')` when local, which may poison the cache and lock the value since we don't later discard the cache.
Specifically, when I access `http://locala.phacility.com`, I was getting an error like "You made a request for locala.phacility.com, but no configured site can serve this request.". This was because the base-uri was being incorrectly frozen as "local.phacility.com". The expectation is that it will match, so the standard PlatformSite will serve the request.
Test Plan:
- Before change: "no configured site" error.
- After change: local instance works properly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13168
Differential Revision: https://secure.phabricator.com/D19526
Summary:
See PHI746. See also T11833, perhaps. Ref T13151.
Long ago, parent revisions were called "dependent revisions". This was changed to "parent revisions" in the action UI to improve clarity, but not changed in the timeline stories.
Update the timeline stories to use the same language the actions in the UI use.
Test Plan:
{F5732876}
{F5732877}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19514
Summary: Ref T13152. The pager does a bit of magic here and doesn't populate `nextPageID` when it knows it got an exact final page. The logic misfired in this case and sent us back to the start.
Test Plan:
- Set page size to 1 to guarantee rows were an exact multiple of page size.
- Ran `rebuild-identities` (I no-op'd the actual logic to make it faster).
- Before: looped forever.
- After: clean exit after processing everything.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13152
Differential Revision: https://secure.phabricator.com/D19479
Summary:
Ref T13151. See PHI654. Depends on D19477. If you have long package names, the table of contents (e.g., in Differential) can end up expanding to be gigantic.
Getting tables to behave nicely is hard (or, at least, I can't figure it out after spending a decent amount of time on it; see also `AphrontTableView::renderSingleDisplayLine()`). I tried a bunch of things and Googled for a bit but didn't make any progress on finding a CSS solution. Just truncate the package names to get reasonable behavior without falling down any kind of CSS rabbit hole.
Test Plan:
- Created a package named "Very long package name...".
- Created a package named "MMMMMMMMMMMMMMMMMMMMMM...".
- Had them own a file in a Differential revision, viewed that revision.
- Before: table is pushed out to several times the browser window width and everything is kind of a mess.
- After: package names get truncated to something reasonable.
{F5652953}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19478
Summary:
Ref T13141. Currently, during first-time setup we don't surface all the details about connection exceptions that we could: the underlying exception is discarded inside cluster connection management.
This isn't a huge issue since the reason for connection problems is usually fairly obvious, but in at least one case (see T13141) we hit a less-than-obvious exception.
Instead, store the original exception and propagate the message up the stack so users have more information about the problem.
Test Plan:
- Configured an intentionally bad MySQL username.
- Restarted Apache and loaded Phabricator.
- Got a more helpful exception with a specific authentication error message.
{F5622361}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13141
Differential Revision: https://secure.phabricator.com/D19454
Summary:
See D19446. This should make it easier to process larger, more complex result sets in constant memory.
Today, `LiskMigrationIterator` takes constant memory but can't apply `needX()` reqeusts or `withY(...)` constraints.
Using a raw `Query` can handle this stuff, but requires memory proportional to the size of the result set.
Offer the best of both worlds: constant memory and full access to the power of `Query` classes.
Test Plan:
Used this script to iterate over every commit, saw sensible behavior:
```name=list-commits.php
<?php
require_once 'scripts/init/init-script.php';
$viewer = PhabricatorUser::getOmnipotentUser();
$query = id(new DiffusionCommitQuery())
->setViewer($viewer);
$iterator = new PhabricatorQueryIterator($query);
foreach ($iterator as $commit) {
echo $commit->getID()."\n";
}
```
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19450
Summary:
See <https://discourse.phabricator-community.org/t/maniphest-non-integer-point-values-in-csv-export/1443>.
We currently export the Maniphest "points" field as an integer, but allow it to accept decimal values (e.g. "6.25").
Also fix a bug where we wouldn't roll over from "..., X, Y, Z, AA, AB, ..." correctly for Excel column names if sheet had more than 26 columns.
Test Plan:
- Set a task point value to 6.25.
- Exported to text, JSON, XLS.
- Saw 6.25 represented accurately in exports.
- Exported an excel sheet with 27+ columns.
- Manually printed the first 200 column names to check that the algorithm looks correct.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19434
Summary:
Ref T13130. See PHI483. Currently, "Plan Changes + Draft" uses rules like "Plan Changes", not rules like "Draft", and allows "Accept".
This isn't consistent with how "Draft" and "Accept" work in other cases. Make "Plan Changes + Draft" more like "Draft" for consistency.
Also fix a string that didn't have a natural English version.
Test Plan:
- Added a failing build plan.
- Created a revision.
- Loaded the revision before builds completed, saw a nicer piece of text about "waiting for builds" instead of "waiting for 2 build(s)".
- Builds failed, which automatically demoted the reivsion to "Changes Planned + Draft".
- As the author and as a reviewer, verified all the actions available to me made sense (particularly, no "Accept").
- Abandoned the revision to test "Abandoned + Draft".
- As the author and as a reviewer, verified all the actions available to me made sense.
- Reclaimed the revision, then used "Request Review" to send it to "Needs Review". Verified that actions made sense and, e.g., reviewers could now "Accept" normally.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19398
Summary:
Ref T13124. Ref T13131. Fixes T8953. See PHI512.
When you receieve a notification about an object and then someone hides that object from you (or deletes it), you get a phantom notification which is very difficult to clear.
For now, test that notifications are visible when you open the menu and clear any that are not.
This could be a little more elegant than it is, but the current behavior is very clearly broken. This unbreaks it, at least.
Test Plan:
- As Alice, configured task stuff to notify me (instead of sending email).
- As Bailey, added Alice as a subscriber to a task, then commented on it.
- As Alice, loaded home and saw a notification count. Didn't click it yet.
- As Bailey, set the task to private.
- As Alice, clicked the notification bell menu icon.
- Before change: no unread notifications, bell menu is semi-stuck in a phantom state which you can't clear.
- After change: bad notifications automatically cleared.
{F5530005}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13131, T13124, T8953
Differential Revision: https://secure.phabricator.com/D19384
Summary: Ref T13116. See PHI526. Currently, the YouTube remarkup rule writes an `<iframe ...>` but does not adjust the Content-Security-Policy appropriately.
Test Plan: Pasted a YouTube link; viewed it in Safari, Chrome and Firefox.
Maniphest Tasks: T13116
Differential Revision: https://secure.phabricator.com/D19277
Summary:
Ref T13114. See PHI519. An install is interested in modifying a tokenizer custom field from the comment area. Provide this capability.
This patch is fairly narrow but should solve the immediate need.
Test Plan: Added, removed, and modified a tokenizer custom field using the comment action dropdown.
Maniphest Tasks: T13114
Differential Revision: https://secure.phabricator.com/D19270
Summary:
Depends on D19247. Ref T13109. When we receive an SSH request, generate a random unique ID for the request. Then thread it down through the process tree.
The immediate goal is to let the `ssh-exec` process coordinate with `commit-hook` process and log information about read and write lock wait times. Today, there's no way for `ssh-exec` to interact with the `PushEvent`, but this is the most helpful place to store this data for users.
Test Plan: Made pushes, saw the `PushEvent` table populate with a random request ID. Exported data and saw the ID preserved in the export.
Maniphest Tasks: T13109
Differential Revision: https://secure.phabricator.com/D19249
Summary:
Fixes T8845. Ref T13102. See PHI467. Currently, object monograms like `L1` which appear in Remarkup headers render incorrectly (with an internal placeholder "x") in the table of contents:
{F5475505}
Instead, render them down to just, e.g., `L1` in plain text.
For `{P123}` I just rendered it to `{P123}` since it's not really clear to me what users intend. This could be adjusted if there's some reasonable thing that someone is trying to do with this.
Test Plan: Wrote a Phriction document with several object references (like `L1` and `{P123}`) in headers. After patch, saw "x"-free, sensible-looking header names in the table of contents.
Maniphest Tasks: T13102, T8845
Differential Revision: https://secure.phabricator.com/D19234
Summary:
Ref T13101. This is a minimal change to make "{meme ...}" work with the new Content-Security-Policy by using an Ajax request to generate the image and then swapping the source on the client.
This could be much cleaner (see T5258, etc).
Test Plan: Used `{meme, src=cat6, above=i am, below=cat}`, chuckled completely unironically.
Maniphest Tasks: T13101
Differential Revision: https://secure.phabricator.com/D19196
Summary:
Depends on D19192. Ref T4190. Ref T13101. Instead of directly including the proxy endpoint with `<img src="..." />`, emit a placeholder and use AJAX to make the request. If the proxy fetch fails, replace the placeholder with an error message.
This isn't the most polished implementation imaginable, but it's much less mysterious about errors.
Test Plan: Used `{image ...}` for valid and invalid images, got images and useful error messages respectively.
Maniphest Tasks: T13101, T4190
Differential Revision: https://secure.phabricator.com/D19193
Summary: Depends on D19173. Ref T13096. Adds an optional, disabled-by-default lock log to make it easier to figure out what is acquiring and holding locks.
Test Plan: Ran `bin/lock log --enable`, `--disable`, `--name`, etc. Saw sensible-looking output with log enabled and daemons restarted. Saw no additional output with log disabled and daemons restarted.
Maniphest Tasks: T13096
Differential Revision: https://secure.phabricator.com/D19174
Summary:
Ref T13096. Currently, we do a fair amount of clever digesting and string manipulation to build lock names which are less than 64 characters long while still being reasonably readable.
Instead, do more of this automatically. This will let lock acquisition become simpler and make it more possible to build a useful lock log.
Test Plan: Ran `bin/repository update`, saw a reasonable lock acquire and release.
Maniphest Tasks: T13096
Differential Revision: https://secure.phabricator.com/D19173
Summary:
Depends on D19155. Ref T13094. Ref T4340.
We can't currently implement a strict `form-action 'self'` content security policy because some file downloads rely on a `<form />` which sometimes POSTs to the CDN domain.
Broadly, stop generating these forms. We just redirect instead, and show an interstitial confirm dialog if no CDN domain is configured. This makes the UX for installs with no CDN domain a little worse and the UX for everyone else better.
Then, implement the stricter Content-Security-Policy.
This also removes extra confirm dialogs for downloading Harbormaster build logs and data exports.
Test Plan:
- Went through the plain data export, data export with bulk jobs, ssh key generation, calendar ICS download, Diffusion data, Paste data, Harbormaster log data, and normal file data download workflows with a CDN domain.
- Went through all those workflows again without a CDN domain.
- Grepped for affected symbols (`getCDNURI()`, `getDownloadURI()`).
- Added an evil form to a page, tried to submit it, was rejected.
- Went through the ReCaptcha and Stripe flows again to see if they're submitting any forms.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13094, T4340
Differential Revision: https://secure.phabricator.com/D19156
Summary: Ref T13090. The doc string in "any()" wasn't specified correctly and the help page wasn't getting enough supporting data to build properly.
Test Plan: Viewed "Reference: Advanced Functions" for a custom datasource field and got more helpful help.
Maniphest Tasks: T13090
Differential Revision: https://secure.phabricator.com/D19128
Summary: Depends on D19126. Ref T13090. For datasource custom fields, this proxies the datasource and provides "none()" and "any()" functions to allow you to search for objects with no values or any values.
Test Plan:
- Created a custom "Owning Group" field in Maniphest using a Projects datasource.
- For a task with no owner assigned, searched for "none()" (hit) and "any()" (miss).
- Assigned the task to an owning project.
- Searched for "none()" (miss), "any()" (hit), the project it is now a member of (hit) and some random other project (miss).
Maniphest Tasks: T13090
Differential Revision: https://secure.phabricator.com/D19127
Summary: Ref T13090. Currently, it isn't possible to query custom fields for complex constraints. Lay the groundwork to implement some of the easy ones (none(), any()) for Datasource/PHID fields.
Test Plan: Hard-coded some constraints and queried with them; see next change for more substantial testing.
Maniphest Tasks: T13090
Differential Revision: https://secure.phabricator.com/D19126
Summary: See D19117. Instead of automatically figuring this out inside `phutil_tag()`, explicitly add rel="noreferrer" at the application level to all external links.
Test Plan:
- Grepped for `_blank`, `isValidRemoteURIForLink`, checked all callsites for user-controlled data.
- Created a link menu item, verified noreferrer in markup.
- Created a link custom field, verified no referrer in markup.
- Verified noreferrer for `{nav href=...}`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D19118
Summary:
Depends on D19092. Ref T13077. This modernizes markup rendering for PhrictionContent.
This is a little messy because table of contents generation isn't straightforward.
Test Plan: Viewed Phriction documents with and without 3+ headers, saw ToC vs no ToC. Edited/previewed documents. Grepped for affected symbols. Checked DarkConsole for sensible cache behavior.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19093
Summary:
Depends on D19073. Ref T13073. Give leases a normal header tag and try to wrangle their status constants a bit.
Also, try to capture the "status class" pattern a bit. Since we target PHP 5.2.3 we can't use `static::` so the actual subclass is kind of a mess. Not exactly sure if I want to stick with this or not. We could consider targeting PHP 5.3.0 instead to get `static::` / late static binding.
Test Plan: Viewed leases and lease lists, saw better and more conventional status information.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19074
Summary: Ref T12677. Skip these checks if we're doing the new stuff. Also, allow priority to be unspecified.
Test Plan: Will deploy.
Maniphest Tasks: T12677
Differential Revision: https://secure.phabricator.com/D19043
Summary:
Depends on D19031. Fixes T11389. Currently, we render `Dxxx` in a text context (plain text email) as just a URI.
Instead, render it like `Dxxx <uri>`. This is more faithful to the original intent and preserves `T123/T456` as two separate, usable links.
Test Plan: Wrote `T123/T234` in a task, pulled mail for it with `bin/mail show-outbound`, saw separate clickable links.
Maniphest Tasks: T11389
Differential Revision: https://secure.phabricator.com/D19032
Summary:
Depends on D19002. Ref T13053. Ref T12677. Adds a new option to allow configuration of multiple mailers.
Nothing actually uses this yet.
Test Plan: Tried to set it to various bad values, got reasonable error messages. Read documentation.
Reviewers: amckinley
Maniphest Tasks: T13053, T12677
Differential Revision: https://secure.phabricator.com/D19003
Summary:
Ref T13057. This makes "reverts" syntax more visible and useful. In particular, you can now `Reverts Dxx` in a revision or commit, and `Reverts <hash>` from a revision.
When you do, the corresponding object will get a more-visible cross-reference marker in its timeline:
{F5405517}
From here, we can look at surfacing revert information more heavily, since we can now query it on revision/commit pages via edges.
Test Plan: Used "reverts <hash>" and "reverts <revision>" in Differential and Diffusion, got sensible results in the timeline.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13057
Differential Revision: https://secure.phabricator.com/D18978
Summary:
Fixes T12800. See that task for discussion. When a cell in a CSV begins with "=", "+", "-", or "@", mangle the content to discourage Excel from executing it.
This is clumsy, but we support other formats (e.g., JSON) which preserve the data faithfully and you should probably be using JSON if you're going to do anything programmatic with it.
We could add two formats or a checkbox or a warning or something but cells with these symbols are fairly rare anyway.
Some possible exceptions I can think of are "user monograms" (but we don't export those right now) and "negative numbers" (but also no direct export today). We can add exceptions for those as they arise.
Test Plan: Exported a task named `=cmd|'/C evil.exe'!A0`, saw the title get mangled with "(!)" in front.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12800
Differential Revision: https://secure.phabricator.com/D18974
Summary:
Depends on D18961. Ref T13049. Currently, longer exports don't give the user any feedback, and exports that take longer than 30 seconds are likely to timeout.
For small exports (up to 1,000 rows) continue doing the export in the web process.
For large exports, queue a bulk job and do them in the workers instead. This sends the user through the bulk operation UI and is similar to bulk edits. It's a little clunky for now, but you get your data at the end, which is far better than hanging for 30 seconds and then fataling.
Test Plan: Exported small result sets, got the same workflow as before. Exported very large result sets, went through the bulk flow, got reasonable results out.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18962
Summary:
Depends on D18959. Ref T13049. Provide tags, subscribers, spaces, and created/modified as automatic extensions for all objects which support them.
(Also, for JSON export, be a little more consistent about exporting `null` instead of empty string when there's no value in a text field.)
Test Plan: Exported users and tasks, saw relevant fields in the export.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18960
Summary: Depends on D18958. Ref T13049. Support the new stuff. There are a couple more fields this needs to strictly improve on the old export, but I'll add them as extensions shortly.
Test Plan: Exported tasks to Excel, saw reasonble-looking data in the export.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18959
Summary:
Depends on D18957. Ref T13049. To do Excel exports, PHPExcel needs to be installed on the system somewhere.
This library is enormous (1K files, ~100K SLOC), which is why we don't just include it in `externals/`. This install process is a little weird and we could improve it, but users don't seem to have too much difficulty with it. This shouldn't be worse than the existing workflow in Maniphest, and I tried to make it at least slightly more clear.
Test Plan: Uninstalled PHPExcel, got it marked "Unavailable" and got reasonably-helpful-ish guidance on how to get it to work. Reinstalled, exported, got a sheet.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18958
Summary:
Depends on D18956. Ref T13049. Make the "Export Format" selector sticky.
This is partly selfish, since it makes testing format changes a bit easier.
It also seems like it's probably a good behavior in general: if you export to Excel once, that's probably what you're going to pick next time.
Test Plan: Exported to excel. Exported again, got excel as the default option.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18957
Summary:
Depends on D18955. Ref T13049. This directory was getting a little cluttered with different kinds of code.
Put the formats (csv, json, ...), the field types (int, string, epoch, ...) and the engine-related stuff in subdirectories.
Test Plan: wow so aesthetic
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18956
Summary:
Depends on D18954. Ref T13049. This brings over the existing Maniphest Excel export pipeline in a generic way.
The `<Type>ExportField` classes know directly that `PHPExcel` exists, which is a little sketchy, but writing an Excel indirection layer sounds like a lot of work and I don't anticipate us changing Excel backends anytime soon, so trying to abstract this feels YAGNI.
This doesn't bring over the install instructions for PHPExcel or the detection of whether or not it exists. I'll bring that over in a future change.
Test Plan: Exported users as Excel, opened them up, got a sensible-looking Excel sheet.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18955
Summary:
Depends on D18953. Ref T13049. Allow applications and infrastructure to supplement exportable fields for objects.
Then, implement an extension for custom fields. Only a couple field types (int, string) are supported for now.
Test Plan: Added some custom fields to Users, populated them, exported users. Saw custom fields in the export.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18954
Summary:
Depends on D18951. Ref T13049. When we export to CSV or plain text, add a header row in the first line of the file to explain what each column means. This often isn't obvious with PHIDs, etc.
JSON has keys and is essentially self-labeling, so don't do anything special.
Test Plan: Exported CSV and text, saw new headers. Exported JSON, no changes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18952
Summary: Depends on D18948. Ref T13051. The actual logic ended up so simple that this doesn't really feel terribly valuable, but maybe it'll catch something later on.
Test Plan: Ran test.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13051
Differential Revision: https://secure.phabricator.com/D18949
Summary:
Depends on D18947. Ref T13051. This goes through transaction tables and compacts the edge storage into the slim format.
I put this on `bin/garbage` instead of `bin/storage` because `bin/storage` has a lot of weird stuff about how it manages databases so that it can run before configuration (e.g., all the `--user`, `--password` type flags for configuring DB connections).
Test Plan:
Loaded an object with a bunch of transactions. Ran migration. Spot checked table for sanity. Loaded another copy of the object in the web UI, compared the two pages, saw no user-visible changes.
Here's a concrete example of the migration effect -- old row:
```
*************************** 44. row ***************************
id: 757
phid: PHID-XACT-PSTE-5gnaaway2vnyen5
authorPHID: PHID-USER-cvfydnwadpdj7vdon36z
objectPHID: PHID-PSTE-5uj6oqv4kmhtr6ctwcq7
viewPolicy: public
editPolicy: PHID-USER-cvfydnwadpdj7vdon36z
commentPHID: NULL
commentVersion: 0
transactionType: core:edge
oldValue: {"PHID-PROJ-wh32nih7q5scvc5lvipv":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-wh32nih7q5scvc5lvipv","dateCreated":"1449170691","seq":"0","dataID":null,"data":[]},"PHID-PROJ-5r2ed5v27xrgltvou5or":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-5r2ed5v27xrgltvou5or","dateCreated":"1449170683","seq":"0","dataID":null,"data":[]},"PHID-PROJ-zfp44q7loir643b5i4v4":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-zfp44q7loir643b5i4v4","dateCreated":"1449170668","seq":"0","dataID":null,"data":[]},"PHID-PROJ-okljqs7prifhajtvia3t":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-okljqs7prifhajtvia3t","dateCreated":"1448902756","seq":"0","dataID":null,"data":[]},"PHID-PROJ-3cuwfuuh4pwqyuof2hhr":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-3cuwfuuh4pwqyuof2hhr","dateCreated":"1448899367","seq":"0","dataID":null,"data":[]},"PHID-PROJ-amvkc5zw2gsy7tyvocug":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-amvkc5zw2gsy7tyvocug","dateCreated":"1448833330","seq":"0","dataID":null,"data":[]}}
newValue: {"PHID-PROJ-wh32nih7q5scvc5lvipv":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-wh32nih7q5scvc5lvipv","dateCreated":"1449170691","seq":"0","dataID":null,"data":[]},"PHID-PROJ-5r2ed5v27xrgltvou5or":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-5r2ed5v27xrgltvou5or","dateCreated":"1449170683","seq":"0","dataID":null,"data":[]},"PHID-PROJ-zfp44q7loir643b5i4v4":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-zfp44q7loir643b5i4v4","dateCreated":"1449170668","seq":"0","dataID":null,"data":[]},"PHID-PROJ-okljqs7prifhajtvia3t":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-okljqs7prifhajtvia3t","dateCreated":"1448902756","seq":"0","dataID":null,"data":[]},"PHID-PROJ-3cuwfuuh4pwqyuof2hhr":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-3cuwfuuh4pwqyuof2hhr","dateCreated":"1448899367","seq":"0","dataID":null,"data":[]},"PHID-PROJ-amvkc5zw2gsy7tyvocug":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-amvkc5zw2gsy7tyvocug","dateCreated":"1448833330","seq":"0","dataID":null,"data":[]},"PHID-PROJ-tbowhnwinujwhb346q36":{"dst":"PHID-PROJ-tbowhnwinujwhb346q36","type":41,"data":[]},"PHID-PROJ-izrto7uflimduo6uw2tp":{"dst":"PHID-PROJ-izrto7uflimduo6uw2tp","type":41,"data":[]}}
contentSource: {"source":"web","params":[]}
metadata: {"edge:type":41}
dateCreated: 1450197571
dateModified: 1450197571
```
New row:
```
*************************** 44. row ***************************
id: 757
phid: PHID-XACT-PSTE-5gnaaway2vnyen5
authorPHID: PHID-USER-cvfydnwadpdj7vdon36z
objectPHID: PHID-PSTE-5uj6oqv4kmhtr6ctwcq7
viewPolicy: public
editPolicy: PHID-USER-cvfydnwadpdj7vdon36z
commentPHID: NULL
commentVersion: 0
transactionType: core:edge
oldValue: []
newValue: ["PHID-PROJ-tbowhnwinujwhb346q36","PHID-PROJ-izrto7uflimduo6uw2tp"]
contentSource: {"source":"web","params":[]}
metadata: {"edge:type":41}
dateCreated: 1450197571
dateModified: 1450197571
```
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13051
Differential Revision: https://secure.phabricator.com/D18948
Summary: Depends on D18946. Ref T13051. Begins writing edge transactions as just a list of changed PHIDs.
Test Plan: Added, edited, and removed projects. Reviewed transaction record and database. Saw no user-facing changes but a far more compact database representation.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13051
Differential Revision: https://secure.phabricator.com/D18947
Summary:
Ref T13051. This puts a translation layer between the raw edge data in the transaction table and the UI that uses it.
The intent is to start writing new, more compact data soon. This class give us a consistent API for interacting with either the new or old data format, so we don't have to migrate everything upfront.
Test Plan: Browsed around, saw existing edge transactions render properly in transactions and feed. Added and removed subscribers and projects, saw good transaction rendering.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13051
Differential Revision: https://secure.phabricator.com/D18946
Summary:
Depends on D18918. Ref T13046. Ref T5954. Pull logs can currently be browsed in the web UI, but this isn't very powerful, especially if you have thousands of them.
Allow SearchEngine implementations to define exportable fields so that users can "Use Results > Export Data" on any query. In particular, they can use this workflow to download a file with pull logs.
In the future, this can replace the existing "Export to Excel" feature in Maniphest.
For now, we hard-code JSON as the only supported datatype and don't actually make any effort to format the data properly, but this leaves room to add more exporters (CSV, Excel) and data type awareness (integer casting, date formatting, etc) in the future.
For sufficiently large result sets, this will probably time out. At some point, I'll make this use the job queue (like bulk editing) when the export is "large" (affects more than 1K rows?).
Test Plan: Downloaded pull logs in JSON format.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046, T5954
Differential Revision: https://secure.phabricator.com/D18919
Summary:
See PHI305. Ref T13046.
The SSH workflows currently extend `PhabricatorManagementWorkflow` to benefit from sharing all the standard argument parsing code. Sharing the parsing code is good, but it also means they inherit a `getViewer()` method which returns the ommnipotent viewer.
This is appropriate for everything else which extends `ManagementWorkflow` (like `bin/storage`, `bin/auth`, etc.) but not appropriate for SSH workflows, which have a real user.
This caused a bug with the pull logs where `pullerPHID` was not recorded properly. We used `$this->getViewer()->getPHID()` but the correct code was `$this->getUser()->getPHID()`.
To harden this against future mistakes:
- Don't extend `ManagementWorkflow`. Extend `PhutilArgumentWorkflow` instead. We **only** want the argument parsing code.
- Rename `get/setUser()` to `get/setSSHUser()` to make them explicit.
Then, fix the pull log bug by calling `getSSHUser()` instead of `getViewer()`.
Test Plan:
- Pulled and pushed to a repository over SSH.
- Grepped all the SSH stuff for the altered symbols.
- Saw pulls record a valid `pullerPHID` in the pull log.
- Used `echo {} | ssh ... conduit conduit.ping` to test conduit over SSH.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046
Differential Revision: https://secure.phabricator.com/D18912
Summary:
Ref T13045. See that task for discussion.
This replaces `digestForIndex()` with a "clever" algorithm in `digestForAnchor()`. The new digest is the same as `digestForIndex()` except when the original output was "." or "_". In those cases, a replacement character is selected based on entropy accumulated by the digest function as it iterates through the string.
Test Plan: Added unit tests.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13045
Differential Revision: https://secure.phabricator.com/D18909
Summary:
Ref T13043. In D18898 I moved VCS passwords to the new shared infrastructure.
Before account passwords can move, we need to make two changes:
- For legacy reasons, VCS passwords and Account passwords have different "digest" algorithms. Both are more complicated than they should be, but we can't easily fix it without breaking existing passwords. Add a `PasswordHashInterface` so that objects which can have passwords hashes can implement custom digest logic for each password type.
- Account passwords have a dedicated external salt (`PhabricatorUser->passwordSalt`). This is a generally reasonable thing to support (since not all hashers are self-salting) and we need to keep it around so existing passwords still work. Add salt support to `AuthPassword` and make it generate/regenerate when passwords are updated.
Then add a nice story about password digestion.
Test Plan: Ran migrations. Used an existing VCS password; changed VCS password. Tried to use a revoked password. Unit tests still pass. Grepped for callers to legacy `PhabricatorHash::digestPassword()`, found none.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18900
Summary: Ref T13025. This makes limits (for fields like "Assign To") work in the bulk editor, so you can't type "Assign to: x, y, z" anymore.
Test Plan: Hit limit for "Assign to" and a custom project field. No limit for "Add subscribers".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13025
Differential Revision: https://secure.phabricator.com/D18888
Summary:
See PHI173. Currently, Herald has an "Assign to" action for tasks, and you can specify custom fields with datasource values (like users or projects) that have a limit (like 1 "Owner", or 12 "Jury Members").
Herald doesn't support these limits right now, so you can write `[ Assign to ][ X, Y, Z ]`. This just means "Assign to X", but make it more clear by actually enforcing the limit in the UI.
Test Plan:
- Created a "projects" custom field with limit 1.
- Tried to create actions that 'assign to' or 'set custom field to' more than one thing, got helpfully rebuffed by the UI.
- Created an "add subscribers" action with more than one value.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18887
Summary:
Fixes T13042. This hooks up the new "silent" mode from D18882 and makes it actually work.
The UI (where we tell you to go run some command and then reload the page) is pretty clumsy, but should solve some problems for now and can be cleaned up eventually. The actual mechanics (timeline aggregation, Herald interaction, etc.) are on firmer ground.
Test Plan:
- Made a normal bulk edit, got mail and feed stories.
- Made a silent bulk edit, no mail and no feed.
- Saw "Silent Edit" marker in timeline for silent edits:
{F5386245}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13042
Differential Revision: https://secure.phabricator.com/D18883
Summary: Ref T13025. We're getting kind of a lot of actions, so put them in nice groups so they're easier to work with.
Test Plan: {F5386038}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13025
Differential Revision: https://secure.phabricator.com/D18880
Summary: Ref T13025. Fixes T5689. A straightforward change!
Test Plan: Used the bulk editor to modify a custom "select" field like the one in T5689.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13025, T5689
Differential Revision: https://secure.phabricator.com/D18879
Summary:
Ref T13025. This allows custom tokenizer fields, like a "Owning Group" field, to be edited with the bulk editor.
See PHI173 for some context.
Test Plan: Edited a custom "Owner" field (a project tokenizer) with the bulk editor.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13025
Differential Revision: https://secure.phabricator.com/D18877
Summary:
Ref T13025. Custom field transactions work somewhat unusually: the values sometimes need to be encoded. We currently do not apply this encoding correctly via Conduit.
For example, setting some custom PHID field to `["PHID-X-Y"]` fails with a bunch of JSON errors.
Add an extra hook callback so that EditTypes can apply processing to transaction values, then apply the correct CustomField processing.
This only affects Conduit. In a future diff, this also allows bulk edit of custom fields to work correctly.
Test Plan: Added a custom field to Maniphest with a list of projects. Used Conduit to bulk edit it (which now works, but did not before). Used the web UI to bulk edit it.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13025
Differential Revision: https://secure.phabricator.com/D18876
Summary:
We have one production instance with failing database backups since they recently uploaded a 52MB hunk. The production configuration specifies a 64MB "max_allowed_packet" in `[mysqld]`, but this doesn't apply to `mysqldump` (we'd need to specify it in a separate `[mysqldump]` section) and `mysqldump` runs with an effective limit of the default (16MB).
We could change our production config to specify a value in `[mysqldump]`, but just change it unconditionally at execution time since there's no reason for any user to ever want this command to fail because they have too much data.
Test Plan: Dumped locally, will verify production backup goes through cleanly.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18834
Summary:
See PHI173. Adds custom field support for Herald actions, and implements actions for "Datasource/Tokenizer" fields.
The only action available for now is "set field to...". Other actions ("Add values", "Remove values") might make sense in the future for these fields, but there's currently no use case. For most other field types (text, select, checkbox, etc) only "Set to" makes sense.
Test Plan:
- Added a "datasource" custom field to the custom field definition in Config.
- Added a "if field is empty, set field to default value X" rule to Herald.
- Created a task with a nonempty field: no Herald trigger.
- Created a task with an empty field: Herald fired.
- Reviewed rule and transcripts for text strings.
{F5297615}
{F5297616}
{F5297617}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18784
Summary:
Ref T12974. Ferret object queries SELECT a virtual "_ft_rank" column for relevance ordering.
Currently, they always SELECT this column. That's fine and doesn't hurt anything, but makes developing and debugging things kind of a pain since every query has this `, blah blah _ft_rank` junk.
Instead, construct this column only if we're actually going to use it.
Mostly, this cleans up DarkConsole / query logs a bit.
Test Plan:
Viewed normal query results on various pages, viewed global search results, ordered Maniphest tasks by normal stuff and by "Relevance".
Viewed DarkConsole, saw no more "_ft_rank" junk on normal pages.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12974
Differential Revision: https://secure.phabricator.com/D18728
Summary:
Ref T12680. See PHI167. See that task for discussion.
Rewrite `DiffusionCommitQuery` to work more like `DifferentialRevisionQuery`, and use a UNION to find "all revisions you need to audit OR respond to".
I tried to get this working a little more cleanly than RevisionQuery does, and can probably simplify that now.
Test Plan: Poked at the UI locally without hitting any apparent issues, but my local data is pretty garbage at this point. I'll take a look at how the query plans work on `secure`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12680
Differential Revision: https://secure.phabricator.com/D18722
Summary:
Ref T13000. The new approach for dumping database-by-database means that we don't get CREATE DATABASE or USE statements, which makes importing the dump again inconvenient.
Manually stitch these into the dump.
Test Plan:
- Used `bin/storage dump --namespace ...` to dump a smaller local instance.
- Used `bin/storage destroy --namespace ...`, to destroy the namespace, then inported the dump cleanly.
- Verified that each CREATE DATABASE statement appears only once.
- Verified that `bin/storage renamespace --live` can correctly process this file.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18707
Summary: Noticed a couple of typos in the docs, and then things got out of hand.
Test Plan:
- Stared at the words until my eyes watered and the letters began to swim on the screen.
- Consulted a dictionary.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D18693
Summary:
Ref T13000. This marks each table as either "data" (normal data), "cache" (automatically rebuilt, no need to ever dump) or "index" (can be manually rebuilt).
By default, `bin/storage dump` dumps data and index tables, but not cache tables.
With `--no-indexes`, it dumps only data tables. Indexes can be rebuilt after a restore with `bin/search index --all ...`.
Test Plan:
- Ran `--no-indexes` and normal dumps with `--trace`, verified that cache and index (former case) or cache only (latter case) tables were dumped with `--no-data`.
- Verified dump has the same number of `CREATE TABLE` statements as before the changes.
- Reviewed persistence tags in the web UI (note Ferret engine tables are "Index"):
{F5210886}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18682
Summary:
Ref T13000. This allows us to be more selective about which tables we dump data for, to reduce the size of backups and exports. The immediate goal is to make large `ngrams` tables more manageable in the cluster, but this generally makes all backups and exports faster and easier.
Here, tables are dumped one at a time. A followup change will sometimes add the `--no-data` flag, to skip dumping readthrough caches and (optionally) rebuildable indexes.
Test Plan: Compared a dump from `master` and from this branch, found them to be essentially identical. The new dump has a little more header information in each section. Verified each contains the same number of `CREATE TABLE` statements.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18679
Summary: See PHI118. Enables hovercards to support peeking at tags and other details if you, e.g., create numerous identical subtasks of each task.
Test Plan: {F5210816}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18681
Summary:
Ref T13000. This adds support for tracking "common" ngrams, which occur in too many documents to be useful as part of the ngram index.
If an ngram is listed in the "common" table, it won't be written when indexing documents, or queried for when searching for them.
In this change, nothing actually writes to the "common" table. I'll start writing to the table in a followup change.
Specifically, I plan to do this:
- A new GC process updates the "common" table periodically, by writing ngrams which appear in more than X% of documents to it, for some value of X, if there are at least a minimum number of documents (maybe like 4,000).
- A new GC process deletes ngrams that have been added to the common table from the existing indexes.
Hopefully, this will pare down the ngrams index to something reasonable over time without requiring any manual tuning.
Test Plan:
- Ran some queries and indexes.
- Manually inserted ngrams `xxx` and `yyy` into the ngrams table, searched and indexed, saw them ignored as viable ngrams for search/index.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18672
Summary: Despite how I (and everyone else?) pronounce it, it is spelled with an "a". See PHI38.
Test Plan: Googled both spellings.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18622
Summary:
Ref T12819. In D18581, I corrected one bug (ngram selection for terms) but introduced a minor new bug. We now pass `' query '` (term corpus with boundary spaces) to the stemmer, but it bails out on this since English words don't start with spaces.
Trim these extra boundary spaces off before invoking the stemmer.
The practical effect of this is that searching for non-stem variations of a word ("detection") now finds stemmed variations again ("detect"). Prior to fixing this bug, the stem could find longer variations but not the other way around.
Test Plan: Searched for "detection", found results matching "detect" after patch (and saw same results for "detect" and "detection").
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18593
Summary:
Ref T12819. These render the little "Searched For: X, Y, U V" hint about how something was parsed.
(This might get a "substring" color or "title only" color or something in the future.)
Test Plan: {F5178807}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18589
Summary:
Ref T12819. For queries like `v0.2`, we would incorrectly search for ngrams including `0.2`, but this is only a substring ngram: the term corpus splits this into `v0` and `2`, so `0.2` is not in the ngrams table.
When executing term queries, search for term ngrams instead. This makes "v0.2" work properly again.
Test Plan: Searched for "v0.2", found a task with "v0.2" in the title.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18581
Summary:
Ref T12819. This worked right in a non-cluster environment, but `bin/storage upgrade` iterates over each master in a partitioned cluster environment.
Tweak the API so `bin/storage analyze` targets a single host but `bin/storage upgrade` can hit all the masters.
Test Plan: Will run `bin/storage upgrade` in production again. Ran `upgrade` and `analyze` locally, still work fine.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18576
Summary:
Ref T12819. Normallly "ANALYZE TABLE" is like sprinkling magic pixie dust on the database and hoping it will make "good vibes" that cause it to go faster, but in at least some concrete cases with the ngrams tables there really was a key cardinality issue which ANALYZE TABLE corrected, fixing bogus query plans.
Add `bin/storage analyze` to analyze all tables, and make `bin/storage upgrade` run it after adjustment if `--no-adjust` is not specified, and make `bin/storage adjust` run it always.
This runs in a couple seconds and should never hurt anything, so it should be fine to sprinkle lots of pixie dust into the `bin/storage` workflow.
Test Plan: Ran `bin/storage analyze`. Ran `bin/storage upgrade`, saw analyze run. Totally felt great vibes and really aligned chakras on the database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18573
Summary:
See brief discussion in D18554. All the index tables are the same for every application (and, at this point, seem unlikely to change) and we never actually pass these objects around (they're only used internally).
In some other cases (like Transactions) not every application has the same tables (for example, Differential has extra field for inline comments), and/or we pass the objects around (lots of stuff uses `$xactions` directly).
However, in this case, and in Edges, we don't interact with any representation of the database state directly in much of the code, and it doesn't change from application to application.
Just automatically define document, field, and ngram tables for anything which implements `FerretInterface`. This makes the query and index logic a tiny bit messier but lets us delete a ton of boilerplate classes.
Test Plan: Indexed objects, searched for objects. Same results as before with much less code. Ran `bin/storage upgrade`, got a clean bill of health.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18559
Summary:
Ref T12819. Currently, under the Ferret engine, we query each application's index separately and then aggregate the results.
At the moment, results are aggregated by type first, then by actual rank. For example, all the revisions appear first, then all the tasks.
Instead, surface the internal ranking data from the underlying query and sort by it.
Test Plan: Searched for "A B" with a task named "A B" and a revision named "A". Saw task first. Broadly, saw mixed task and revision order in result sets.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18551
Summary:
Ref T12819. I started trying to get individual engines to drive these constraints (e.g., `ManiphestTaskQuery` can do most of the work) but this is a big pain, especially since most engines don't support "any owner" or "no owner", and not everything has an owner, and so on and so on. Going down this path would have meant a huge pile of stub functions everywhere, I think.
Instead, drive these through the main engine using the fulltext document table, which already has everything we need to apply these constraints in a uniform way.
Also tweak some parts of query construction and result ordering.
Test Plan: Searched for documents by author, owner, unowned, any owner, tags, subscribers, fulltext in global search. Got sensible results without any application-specific code.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18550
Summary:
See PHI57. For example, a query for "ios, only()" finds tags tasked with iOS, exactly, and no other tags.
I called this "only()" instead of "exact()" because we use the term/function "Exact" elsewhere with a different meaning, e.g. in Differential.
Test Plan:
Basic query for a tag:
{F5168857}
Same query with "only", finds tasks tagged with only that tag:
{F5168858}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18543
Summary: Ref T12819. "Relevance" here just means "how many of your search terms are present in the title?" but that's about the best we can do anyway.
Test Plan: Indexed tasks "A B", "A Z", "Z B", and "Z Z" (all with "A B" in comments). Searched for "A B". Got results ranked in the listed order, with "A B" as the most relevant hit for query "A B".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18539
Summary: Ref T12819. Move these out of the core engine into the Ferret engine. In the future different applications can define different functions, like "summary:..." or whatever. This may get more formalization when I possibly do "author:" and such some time down the road.
Test Plan: Searched for "title:...". Searched for "dog:...", got a useful error.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18536
Summary:
Ref T12819. Push more of the magic `' '` stuff into the engine and simplify calls to ngram construction.
Also fixes a bug where a task with title "apple banana" and description "cherry doughnut" could match query "banana cherry" by separating separate term segments with newlines instead of spaces.
Test Plan:
- Indexed some objects.
- Searched (term, substring, quoted terms).
- Viewed index in database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18534
Summary: Ref T12819. Earlier I separated some ngram code into an "ngram engine" hoping to share it across the simple Ngrams stuff and the full Ferret stuff, but they actually use slightly different rules. Just pull more of this stuff into FerretEngine to reduce the number of moving pieces and the amount of code duplication.
Test Plan: Searched for terms, rebuilt indexes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18533
Summary: See PHI46. The `core:` function means "find results in either the title or body, but not other auxiliary fields like comments".
Test Plan: Searched for text present in the title (yes), body (yes), and comments (no) with the `core:...` prefix.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18514
Summary:
Ref T12819. Some of the extensions "enrich" the document (adding more fields or relationships), while others "index" it (insert it into some kind of index for later searching).
Currently, these are all muddled under a single "index" phase. However, the Ferret extension cares about fields and relationships which other extensions may add.
Split this into two phases: "enrich" adds fields and relationships so other extensions can read them later if they want. "Index" happens after the document is built and has all the fields and relationships.
The specific problem this solves is that comments may not have been added to the document when the Ferret extension runs. By moving them to the "enrich" phase, the Ferret engine will be able to see and index comments.
Test Plan: Ran `bin/search index ...`, grepped for `indexFulltextDocument`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18513
Summary:
Ref T12819. Adds (hacky, hard-coded) field support (for now, only for "title").
I've written this so `title:quick ferret` is the same as `title:quick title:ferret`. I think this is what users probably mean.
You can do the other thing as `ferret title:quick`, or `title:quick all:ferret`.
Test Plan: Searched for `title:x`, `title:"x"`, `title:~"x"`, etc. Searched for "garbage:y", got an exception since that's not a recognized function. Searched for `title:x y`, saw both do title search.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18503
Summary:
Ref T12819. Supports negating search terms, e.g. "apple -honeycrisp".
When negating a term, we're a little more strict about what can match (that is, what can //prevent// a document from being returned) since it's easy for a user to type "apple -honeycrisp -honey -crisp -crispies -olcrispers -honeyyums" to keep refining their search, but hard/impossible to split apart an overboard term.
Test Plan:
- Ran `apple -smith`, `apple -"granny smith"`, etc.
- Verified `phone -tact` does not exclude `phone contact`.
- (In theory, `phone -~tact` would, but the parser currently doesn't support this, and I'm not champing at the bit to add support.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18502
Summary:
Ref T12819. Distinguishes between "term" queries and "substring" queries, and tries to match them correctly most of the time. For example:
- `example` matches "example", obviously.
- `~amp` matches "example", but `amp` does not.
- `examples` matches "example" through stemming.
- `"examples"` does not match "example" (quoted text does not stem).
- `"an examp"` does not match "an example" (quoted text is still term text).
- `~"an examp"` matches "an example" (quoted, substring-operator text uses substring search).
Test Plan: Ran searches similar to the above, they seemed to do what they should.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18500
Summary:
Ref T12819. Depends on D18492. Instead of passing a raw query into the Query layer, parse it first.
This allows the query layer to figure out which parts should be substring vs term match, and would allow the SearchEngine layer to do `author:...` eventually by picking it out before sending it to the Ferret engine.
Test Plan: Ran some Ferret queries. They work like before, except that nonsense like `-+"quack"` raises an exception now.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18499
Summary:
Ref T12819. I gave this stuff a sweet code name because all the terms related to "fulltext" and "search" already mean 5 different things. It, uh, ferrets out documents for you?
I'm building this to work a lot like the existing ngram index, which seems to work pretty well. If this sticks, it will auto-resolve the join issue (in T12443) by letting us do the entire thing locally in a JOIN and thus dodge a lot of mess.
This index gets built alongside other indexes, but only shows up in the UI if you have prototypes enabled. If you do, it appears under the existing fulltext field in Maniphest. No existing functionality is affected or disrupted.
NOTE: The query engine half of this is still EXTREMELY primitive, and this probably performs worse than the existing field for now. If this doesn't show obvious signs of being awful on `secure` I'll improve that in followup changes.
Test Plan:
Indexed my tasks, ran some simple queries, got the results I wanted, even for queries "ko", "k", "v0.1".
{F5147746}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819, T12443
Differential Revision: https://secure.phabricator.com/D18484
Summary:
Ref T2543. Further consolidates status management into DifferentialRevisionStatus.
One change I'm making here is internally renaming "CLOSED" to "PUBLISHED". The UI will continue to say "Closed", at least for now, but this should make the code more clear because we care about "is closed, exactly" vs "is any closed status (closed, abandoned, sometimes accepted)". This distinction is more obvious as `isClosed()` vs `isPublished()` than, e.g., `isClosedWithExactlyTheClosedStatus()` or something. I think "Published" is generally more clear, too, and more consistent with modern language (e.g., "pre-publish review" replacing "pre-commit review" to make it more clear what we mean in Git/Mercurial).
I've removed the IN_PREPARATION status since this was just earlier groundwork for "Draft" and not actually used, and under the newer plan I'm trying to just abandon `ArcanistDifferentialRevisionStatus` entirely (or, at least, substantially).
Test Plan:
- Viewed revisions.
- Viewed revision list.
- Viewed revisions linked to a task in Maniphest.
- Viewed revision graph of dependencies in Differential.
- Grepped for `COLOR_STATUS_...` constants.
- Grepped for removed method `getRevisionStatusIcon()` (no callsites).
- Grepped for removed method `renderFullDescription()` (one callsite, replaced with just building a `TagView` inline).
- Grepped for removed method `isClosedStatus()` (no callsites after other changes).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18340
Summary: This word is not spelled properly.
Test Plan: Read the word.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18250
Summary: Ref T12845. Converts the cluster and project config options to the new stuff; this is mostly just shifting boilerplate around.
Test Plan: Edited, deleted, and mangled these options from the web UI and CLI.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12845
Differential Revision: https://secure.phabricator.com/D18166
Summary:
Fixes T12867. Also:
- Simplify the code a little.
- Stop mutating this on text/mobile -- there's no inherent value in the "youtu.be" link so I think this just changes the text the user wrote unnecessarily.
Test Plan: {F5013804}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12867
Differential Revision: https://secure.phabricator.com/D18149
Summary:
Ref T12855. PHP7 introduced "Throwables", which are sort of like super exceptions. Some errors that PHP raises at runtime have become Throwables instead of old-school errors now.
The major effect this has is blank pages during development under PHP7 for certain classes of errors: they skip all the nice "show a pretty error" handlers and
This isn't a compelete fix, but catches the most common classes of unexpected Throwable and sends them through the normal machinery. Principally, it shows a nice stack trace again instead of a blank page for a larger class of typos and minor mistakes.
Test Plan:
Before: blank page. After:
{F5007979}
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12855
Differential Revision: https://secure.phabricator.com/D18136
Summary:
Ref T12733. When a revision has unsubmitted checkmarks:
- Color the banner yellow.
- Show them in the "X unsubmitted" count.
- Make the "X unsubmitted" button cycle between all drafts (written but unpublished comments) and "draft done" (checked but unsubmitted "Done" checkbox comments).
Test Plan:
- Checked a "Done" box, saw "1 unsubmitted" and yellow banner.
- Clicked "5 unsubmitted" repeatedly, saw it cycle through all unsubmitted comments and checkboxes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18127
Summary:
Ref T12733. This paves the way for a separate "hide" operation which completely hides things.
(I didn't extend this to the server side because that would require schema changes and the new "hide" state is client-only.)
Test Plan: Collapsed and expanded inlines, viewed tooltips.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18126
Summary: Adds a very basic list of all inline comments, threaded, and their status. Kept this a little simpler than the mock, mostly because sorting here feels a little strange given threads would be all over the place. Not sure sorted is needed in practice anyways. I'd probably lean towards just adding a JS checkbox to hide certain rows if needed in the future.
Test Plan:
Test various commenting structures:
- Leave Comment
- Update Diff
- Leave new comment
- Reply to comment
- Reply to comment as revision author
- Mark items as done
- Update diff again
{F4996915}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D18112
Summary:
Fixes T12803. An install is having difficulty diagnosing mail failures, and one component is that permanent task failures aren't reaching the log.
It's reasonable to send these to the log even when "phd.verbose" is off. See T12803 for a rough review of when we generate these failrues today.
Test Plan:
- Faked some exceptions.
- Got a result in the log (P2058) with `phd.verbose` turned off.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12803
Differential Revision: https://secure.phabricator.com/D18106
Summary:
Fixes T12806. Ref T12733.
- Don't count synthetic (lint) comments as anything.
- When you begin writing an inline then cancel it, don't count it as anything.
- When we would show "0 / X", just show "X".
Test Plan:
- Viewed a diff with synthetic comments, no button.
- Wrote, then cancelled an inline. No "X comments".
- Clicked / unlicked "Done", saw "X" -> "1 / X".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12806, T12733
Differential Revision: https://secure.phabricator.com/D18103
Summary:
Fixes T12753. See that task for reproduction instructions.
We add a `GROUP BY` clause to queries with an "ANCESTOR" edge constraint only if the constaint has more than one PHID, but this is incorrect: the same row can be found twice by an ANCESTOR query if task T is tagged with both "B" and "C", children of "A", and the user queries for "tasks in A".
Instead, always add GROUP BY for ANCESTOR queries.
Test Plan:
- Followed test plan in T12753.
- Saw proper paging controls after change.
- Saw `GROUP BY` in DarkConsole.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12753
Differential Revision: https://secure.phabricator.com/D18012
Summary: The tag/shade stuff changed, so purge older markup (like Diviner documents).
Test Plan: {F4972666}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17998
Summary: Ref T12733. Shows a comment snippet when hovering inlines in the objective list.
Test Plan: {F4968490}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17980
Summary:
Add important objectives (like waygates and quest markers) to the minimap.
This also probably fixes @cspeckmim's bug with the {key @} keyboard shortcut.
Test Plan:
(This is probably easier to undestand if you `arc patch` + click around.)
{F4966037}
Reviewers: chad, amckinley
Reviewed By: chad
Subscribers: cspeckmim
Differential Revision: https://secure.phabricator.com/D17955
Summary: Our `local.json` configuration file contains various secrets, including database usernames and passwords. As such, we recently changed the permissions on this file from `0644` to `0640`. After doing so, however, I constantly forget to run commands with `sudo`. This is made worse by the fact that `PhabricatorConfigLocalSource` seems to simply ignore `local.json` is it isn't readable, whereas throwing an `Exception` would have saved me a lot of debugging.
Test Plan:
```name=Before
> /usr/local/src/phabricator/bin/config get mysql.pass
{
"config": [
{
"key": "mysql.pass",
"source": "local",
"value": null,
"status": "unset",
"errorInfo": null
},
{
"key": "mysql.pass",
"source": "database",
"value": null,
"status": "error",
"errorInfo": "Database source is not configured properly"
}
]
}
```
```name=After
> /usr/local/src/phabricator/bin/config get mysql.pass
[2017-05-16 21:49:26] EXCEPTION: (FilesystemException) Path '/usr/local/src/phabricator/conf/local/local.json' is not readable. at [<phutil>/src/filesystem/Filesystem.php:1124]
arcanist(head=stable, ref.master=3c4735795a29, ref.stable=20ad47f27331), phabricator(head=stable, ref.master=3dae9701298f, ref.stable=fcebaa5097f3), phutil(head=stable, ref.master=a900d7b63e95, ref.stable=d02cc05931b0)
#0 Filesystem::assertReadable(string) called at [<phutil>/src/filesystem/Filesystem.php:39]
#1 Filesystem::readFile(string) called at [<phabricator>/src/infrastructure/env/PhabricatorConfigLocalSource.php:25]
#2 PhabricatorConfigLocalSource::loadConfig() called at [<phabricator>/src/infrastructure/env/PhabricatorConfigLocalSource.php:6]
#3 PhabricatorConfigLocalSource::__construct() called at [<phabricator>/src/infrastructure/env/PhabricatorEnv.php:195]
#4 PhabricatorEnv::buildConfigurationSourceStack(boolean) called at [<phabricator>/src/infrastructure/env/PhabricatorEnv.php:95]
#5 PhabricatorEnv::initializeCommonEnvironment(boolean) called at [<phabricator>/src/infrastructure/env/PhabricatorEnv.php:75]
#6 PhabricatorEnv::initializeScriptEnvironment(boolean) called at [<phabricator>/scripts/init/lib.php:22]
#7 init_phabricator_script(array) called at [<phabricator>/scripts/init/init-setup.php:11]
#8 require_once(string) called at [<phabricator>/scripts/setup/manage_config.php:5]
```
Reviewers: #blessed_reviewers, joshuaspence
Reviewed By: joshuaspence
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17917
Summary:
Ref T12616. Fixes T11648. Currently, we snug up replies with a negative margin (from T10563) but this throws off the anchor highlighting.
Instead:
- Remove padding from these dolumns.
- Use margins on the stuff inside them instead.
- Less margins for replies.
- Less margins for collapsed comments.
- Show some text for collapsed comments.
Test Plan:
{F4960890}
{F4960891}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616, T11648
Differential Revision: https://secure.phabricator.com/D17913
Summary:
Fixes T8420. Now that hidden inlines no longer fold into a big clump, anchors can just jump to them in a normal way.
Move the anchors up a smidge so thing work.
Test Plan: Clicked an anchor pointed at a hidden inline, ended up in the right place.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8420
Differential Revision: https://secure.phabricator.com/D17910
Summary:
Ref T12616. Fixes T12715. I suspect these are very rarely used. (I think you tried to get rid of them before but I pushed back since we couldn't really offer great alternatives at the time?)
Now that the code is in a better place:
- Click an inline's header (just the colored part) to select it with the keyboard selection cursor.
- Click again to deselect it.
- You can use "n" and "p" to jump to comments, so "click + n" is the same as the old "V" action.
- This also makes it easier to swap between keyboard and mouse workflows, since you can jump into things with the keyboard at any inline.
Also, make "Reply" render more consistently.
Test Plan:
- Did all that stuff, things seemed to work OK.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12715, T12616
Differential Revision: https://secure.phabricator.com/D17908
Summary:
Ref T12616. This moves "reply" to the new stuff and deletes DifferentialInlineEditor, which no longer does anything.
(This breaks some keyboard shortcuts, but I'll rebase D17859 shortly.)
Test Plan: Replied to inlines; things seemed to work properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17894
Summary:
Ref T12616. This updates clicking the "Done" checkbox for the new stuff.
This one is pretty clean since the "Done" checkbox doesn't do too much weird magic.
Test Plan: Clicked the box a few times.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17888
Summary:
Ref T12616. Fixes T12153. Currently, when you hide inlines, they hide completely and turn into a little bubble on the previous line.
Instead, collapse them to a single line one-by-one. Narrowly, this fixes T12153.
In the future, I plan to make these changes so this feature makes more sense:
- Introduce global "hide everything" states (T8909) so you can completely hide stuff if you want, and this represents more of a halfway state between "nuke it" and "view it".
- Make the actual rendering better, so it says "epriestley: blah blah..." instead of just "..." -- and looks less dumb.
The real goal here is to introduce `DiffInline` and continue moving stuff from the tangled jungle of a million top-level behaviors to sensible smooth statefulness.
Test Plan:
- Hid and revealed inlines in unified and two-up modes.
- These look pretty junk for now:
{F4948659}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616, T12153
Differential Revision: https://secure.phabricator.com/D17861
Summary:
Ref T12616. Diffusion, only, has a "Show All Context" button which expands the full context on all changes.
I don't remember the exact history on this, but it hasn't existed in Differential for some time and no one has complained. I suspect that the "View Options > Show All Context" on each file may replace it. I can't really come up with good reasons to use it, offhand. If we want to restore it, I think global options after T1591 is promising.
{F4945561}
Test Plan:
- Loaded a commit in Diffusion, no longer saw a button.
- Grepped for relevant sigils.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17843
Summary: Fixes T12682.
Test Plan: Ran `bin/storage upgrade --dryrun` repeatedly with un-applied patches, saw it not apply them and not mark them applied.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12682
Differential Revision: https://secure.phabricator.com/D17837
Summary:
Ref T12646.
- Use "wb1" instead of "wb" to use level 1 gzip compression (faster, less compressy). Locally, this went about 2x faster and the output only grew 4% larger.
- LinesOfALargeExecFuture does a lot of unnecessary string operations, and can boil down to a busy wait. The process is pretty saturated by I/O so this isn't the end of the world, but just use raw ExecFuture with FutureIterator so that we wait in `select()`.
- Also, nice the process to +19 so we try to give other things CPU.
Test Plan:
- Ran `bin/storage dump --compress --output ...`.
- Saw CPU time for my local database drop from ~240s to ~90s, with a 4% larger output. Most of this was adding the `1`, but the ExecFuture thing helped a little, too.
- I'm not sure what a great way to test `nice` in a local environment is and it's system dependent anyway, but nothing got worse / blew up.
- Used `gzcat | head` and `gzcat | tail` on the result to sanity-check that everything was preserved.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12646
Differential Revision: https://secure.phabricator.com/D17795
Summary:
Ref T12611. Currently, the HTTP/SSH logs don't have an option to include the instance name.
Add such an option.
Leave it out of the default logs because most installs don't use this.
Test Plan: See next changes.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12611
Differential Revision: https://secure.phabricator.com/D17776
Summary:
Closes T7829 as wontfix. Closes T7965 as wontfix. Closes T7800 as wontfix. Closes T2731 as wontfix. Closes T1271 as wontfix.
We aren't maintaining this at all (see, e.g., T7829) and a user reported a technically accurate security issue via HackerOne: <https://hackerone.com/reports/222870>
Just throw it away until we get to the eventual Conphernece bot/API update and can do this stuff correctly.
Test Plan: Grepped for `phabricatorbot`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7965, T7829, T7800, T2731, T1271
Differential Revision: https://secure.phabricator.com/D17756
