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: 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: 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 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
Summary: Part of the groundwork for T11476.
Test Plan: ran `./bin/storage upgrade` and observed expected DB tables
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11476
Differential Revision: https://secure.phabricator.com/D17736
Summary:
Depends on D17670. Fixes T12137. Fixes T12003. Ref T2632.
This shows users a readout of which terms were actually searched for.
This also drops those terms from the query we submit to the backend, dodging the weird behaviors / search engine bugs in T12137.
This might need some design tweaking.
Test Plan: {F4899825}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12137, T12003, T2632
Differential Revision: https://secure.phabricator.com/D17672
Summary:
Depends on D17669. Ref T12137. Ref T12003. Ref T2632. Ref T7860.
Converts Phabricator to the new parse + compile workflow with intermediate tokens.
Also fixes a bug where searches for `cat"` or similar (unmatched quotes) wouldn't produce a nice exception.
Test Plan:
- Fulltext searched.
- Fulltext searched in Conpherence.
- Fulltext searched with bad syntax.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12137, T12003, T7860, T2632
Differential Revision: https://secure.phabricator.com/D17670
Summary: This updates the backend of PhortuneAccount to use EditEngine and Modular Transactions and updates language to "account manager" for clarity of role.
Test Plan:
- Wiped `phortune_account` table
- Visit Phortune, see new account automatically created.
- Edit name and managers
- Try to set no name or remove myself as a manager, get error messages
- Visit `/phortune/` and create another new account
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17585
Summary: Modernize PhortuneMerchant for Modular Transactions. Also changed the language of "Members" to "Managers", which I think fits better given the power/capability.
Test Plan:
- Create a new Merchant
- Test not filling in a name, see error
- Test removing myself, see error
- Edit an existing Merchant
- Add new managers
- Test removing myself, see error
- Replace Picture
- Update various fields, contact info, email, footer
- Verify transactions are now nice and pretty
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17584
Summary:
Even with `innodb_file_per_table` enabled, individual table files on disk don't normally shrink.
For most tables, like `maniphest_task`, this is fine, since the data in the table normally never shrinks, or only shinks a tiny amount.
However, some tables (like the "worker" and "daemon" tables) grow very large during a huge import but most of the data is later deleted by garbage collection. In these cases, this lost space can be reclaimed by running `OPTIMIZE TABLE` on the tables.
Add a script to `OPTIMIZE TABLE` every table.
My primary goal here is just to reduce storage pressure on `db001` since there are a couple of "import the linux kernel" installs on that host wasting a bunch of space. We're not in any trouble, but this should buy us a good chunk of headroom.
Test Plan: Ran `bin/storage optimize` locally and manually ran `OPTIMIZE TABLE` in production, saw tables get optimized.
Reviewers: chad
Reviewed By: chad
Subscribers: cspeckmim
Differential Revision: https://secure.phabricator.com/D17640
Summary: Ref T12509. This encourages code to move away from HMAC+SHA1 by making the method name more obviously undesirable.
Test Plan: `grep`, browsed around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D17632
Summary:
Ref T12509. Many of the calls to HMAC+SHA1 are just to compute cachekeys for remarkup objects.
Make these use HMAC+SHA256 instead. There is no downside to swapping these since they just cause a cache miss in the worst case.
I also plan to get rid of `PhabricatorMarkupInterface` eventually, but this doesn't go that far.
Test Plan: Browsed some different types of documents (tasks, legalpad documents, phame blogs / posts, pholio mocks, etc).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D17631
Summary:
Ref T12509. This adds support for HMAC+SHA256 (instead of HMAC+SHA1). Although HMAC+SHA1 is not currently broken in any sense, SHA1 has a well-known collision and it's good to look at moving away from HMAC+SHA1.
The new mechanism also automatically generates and stores HMAC keys.
Currently, HMAC keys largely use a per-install constant defined in `security.hmac-key`. In theory this can be changed, but in practice essentially no install changes it.
We generally (in fact, always, I think?) don't use HMAC digests in a way where it matters that this key is well-known, but it's slightly better if this key is unique per class of use cases. Principally, if use cases have unique HMAC keys they are generally less vulnerable to precomputation attacks where an attacker might generate a large number of HMAC hashes of well-known values and use them in a nefarious way. The actual threat here is probably close to nonexistent, but we can harden against it without much extra effort.
Beyond that, this isn't something users should really have to think about or bother configuring.
Test Plan:
- Added unit tests.
- Used `bin/files integrity` to verify, strip, and recompute hashes.
- Tampered with a generated HMAC key, verified it invalidated hashes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D17630
Summary:
Ref T12470. Provides an "integrity" utility which runs in these modes:
- Verify: check that hashes match.
- Compute: backfill missing hashes.
- Strip: remove hashes. Useful for upgrading across a hash change.
- Corrupt: intentionally corrupt hashes. Useful for debugging.
- Overwrite: force hash recomputation.
Users normally shouldn't need to run any of this stuff, but this provides a reasonable toolkit for managing integrity hashes.
I'll recommend existing installs use `bin/files integrity --compute all` in the upgrade guidance to backfill hashes for existing files.
Test Plan:
- Ran the script in many modes against various files, saw expected operation, including:
- Verified a file, corrupted it, saw it fail.
- Verified a file, stripped it, saw it have no hash.
- Stripped a file, computed it, got a clean verify.
- Stripped a file, overwrote it, got a clean verify.
- Corrupted a file, overwrote it, got a clean verify.
- Overwrote a file, overwrote again, got a no-op.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12470
Differential Revision: https://secure.phabricator.com/D17629
Summary: Ref T12450. These are like 95% my fault, but Elastic appears to spell the name "Elasticsearch" consistently in their branding.
Test Plan: `grep ElasticSearch`
Reviewers: chad, 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17601
Summary:
Ref T12450. Currently, if a write fails, we stop and don't try to write to other index services. There's no technical reason not to keep trying writes, it makes some testing easier, and it would improve behavior in a scenario where engines are configured as "primary" and "backup" and the primary service is having some issues.
Also, make "no writable services are configured" acceptable, rather than an error. This state is probably goofy but if we want to detect it I think it should probably be a config-validation issue, not a write-time check. I also think it's not totally unreasonable to want to just turn off all writes for a while (maybe to reduce load while you're doing a background update).
Test Plan:
- Configured a bad ElasticSearch engine and a good MySQL engine.
- Ran `bin/search index ... --force`.
- Saw MySQL get updated even though ElasticSearch failed.
Reviewers: chad, 20after4
Reviewed By: 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17599
Summary:
If you have `maniphest.custom-field-definitions` set to include "required" fields, a bunch of tests which create tasks can fail.
To avoid this, reset this config while running tests.
This mechanism should probably be more general (e.g., reset all config by default, only whitelist some config) but just fix this for now since it's a one-liner and doesn't make eventual cleanup any harder.
Test Plan: Ran `arc unit`, hitting tests that create tasks.
Reviewers: chad, 20after4
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17595
Summary: When building a tokenizer-based edit control for a custom field (e.g. a datasource type), preserve a field validation error whilst building edit controls.
Test Plan:
- Create custom datasource field, set it to required
- Observe that 'Required' does not appear next to control
- Apply patch
- Observe 'Required' appears next to control
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17592
Summary:
Two little issues
1. there was an extra call to getHostForWrite,
2. The engine instance was shared between multiple service definitions so it
was overwriting the list of writable hosts from one service with hosts from another.
Test Plan:
tested in wikimedia production with multiple services defined like this:
```language=json
[
{
"hosts": [
{
"host": "search.svc.codfw.wmnet",
"protocol": "https",
"roles": {
"read": true,
"write": true
},
"version": 5
}
],
"path": "/phabricator",
"port": 9243,
"type": "elasticsearch"
},
{
"hosts": [
{
"host": "search.svc.eqiad.wmnet",
"protocol": "https",
"roles": {
"read": true,
"write": true
},
"version": 5
}
],
"path": "/phabricator",
"port": 9243,
"type": "elasticsearch"
}
]
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17581
Summary:
These exception messages & comments didn't quite match reality.
Fixed and added pht() around a couple of them.
Test Plan: I didn't test this :P
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17578
Summary:
Ref T12450. The way that config repair and setup issues interact is kind of complicated, and if `cluster.search` is invalid we may end up using `cluster.search` before we repair it.
I poked at things for a bit but wasn't confident I could get it to consistently repair before we use it without doing a big messy change.
The only thing that really matters is whether "type" is valid or not, so just put a slightly softer/more-tailored check in for that.
Test Plan:
- With `"type": "elastic"`, loaded setup issues.
- Before patch: hard fatal.
- After patch: softer fatal with more useful messaging.
{F4321048}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17576
Summary:
Ref T12450. Normally, we validate config when:
- You restart the webserver.
- You edit it with `bin/config set ...`.
- You edit it with the web UI.
However, you can also change config by editing `local.json`, `some_env.conf.php`, a `SiteConfig` class, etc. In these cases, you may miss config warnings.
Explicitly re-run search config checks from `bin/search`, similar to the additional database checks we run from `bin/storage`, to try to produce a better error message if the user has made a configuration error.
Test Plan:
```
$ ./bin/search init
Usage Exception: Setting "cluster.search" is misconfigured: Invalid search engine type: elastic. Valid types are: elasticsearch, mysql.
```
Reviewers: chad, 20after4
Reviewed By: 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17574
