Summary: All classes should extend from some other class. See D13275 for some explanation.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13283
Summary:
Ref T8387. This describes changes I haven't made yet, but plan to make.
Also removes the long-deprecated actAsUser capability so I can remove the caveat about it from the documentation.
Test Plan: `grep`, reading
Reviewers: btrahan, eadler
Reviewed By: btrahan, eadler
Subscribers: eadler, epriestley
Maniphest Tasks: T8387
Differential Revision: https://secure.phabricator.com/D13120
Summary: Fixes T8274. That report is very light on details, but I think the issue is that their Elasticsearch is misconfigured and the new setup warning dies a little too hard if the server is just completely dead.
Test Plan: Set `search.elastic.host` to an invalid server, got a similar-looking exception (?), applied patch, got a setup warning instead.
Reviewers: btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T8274
Differential Revision: https://secure.phabricator.com/D12948
Summary: Fixes T5703. These have been unused in production for a while and the new stuff seems good.
Test Plan: Mostly `grep`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5703
Differential Revision: https://secure.phabricator.com/D12949
Summary: Remove the `PhabricatorDefaultSearchEngineSelector` class. This is quite similar to D12053.
Test Plan: Went to `/view/PhabricatorSearchApplication/` and saw the storage engine configuration. Set `search.elastic.host` and saw the highlighted storage engine change.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12670
Summary: Use `__CLASS__` instead of hard-coding class names. Depends on D12605.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12806
Summary:
Ref T5501. Currently, we emit some bad warnings about, e.g., "apc.stat" on PHP 5.5+ systems with OPcache, where the warnings are not relevant.
Generate and raise warnings out of the CacheSpec pipeline so we only run relevant code.
Test Plan: Faked various warnings and saw them render correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5501
Differential Revision: https://secure.phabricator.com/D12318
Summary: Ref T5501. This expands cache information a little more.
Test Plan: {F362975}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5501
Differential Revision: https://secure.phabricator.com/D12316
Summary: Fixes T7764. These settings have low utility, are no longer used by default, have become less useful on modern Windows which has a better selection of available fonts, and will eventually be subsumed (at least, for the most part) by T4103.
Test Plan:
- Grepped for strings.
- Viewed settings.
- Changed font to "24px impact".
- Viewed diffs with default and custom font.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: chad, epriestley
Maniphest Tasks: T7764
Differential Revision: https://secure.phabricator.com/D12301
Summary: These arrays looks a little odd, most likely due to the autofix applied by `ArcanistXHPASTLinter::LINT_ARRAY_SEPARATOR`. See D12296 in which I attempt to improve the autocorrection from this linter rule.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12281
Summary:
Ref T7199. In the vein of D12231, these options were a bad idea.
- They once served a very narrow, Facebook-specific need (see T1992), except even Facebook only used the Differential setting AFAIK.
- Outside of that special case, they are unused and essentially unusable (generally speaking, they do not meaningfully implement anything modular or replaceable).
- I have no knowledge of any install ever changing these settings, and can imagine no reason why they would.
Moving forward:
- If they really need to, they can fork locally and chagne one line.
- I expect "!actions" to make mail at least somewhat more modular soon, anyway.
- Any derived handlers would break after T7199 and need to be rewritten anyway, so this is just taking advantage of a BC break to do cleanup.
Test Plan:
- Grepped for removed configuration.
- Sent some mail from applications, verified the reply handlers set proper reply addresses.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12233
Summary:
Ref T7199. These were a bad idea which got copy-pasted a bunch.
- There is zero reason to ever set these to different things.
- Unsurprisingly, I don't know of any install which has them set to different things.
Unless I've completely forgotten about it, this option was not motivated by some obscure business need, it was just a bad decision which didn't catch anyone's attention at the time.
We partially remedied the mistake at some point by introducing `metamta.reply-handler-domain`, which works as a default for all applications, but never cleaned this mess up.
Test Plan: Sent some mail from applications, verified it picked up appropraite reply handler domains.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12231
Summary:
Ref T7199. Although this is useful for discovery, it's un-useful enough that we already have an option to disable it, and most applications do not provide any meaningful instructions.
Throwing it away makes it easier to move forward and lets us get rid of a config option.
This is becoming a more advanced/power-user feature anyway, and the new syntax will be significantly more complex and hard to explain with a one-liner. I'm currently thinking that I'll maybe make the "help" menu a dropdown and give it some options like:
+---+
| O |
+---+---------------------+
| Maniphest Documentation |
| Maniphest Email Actions |
+-------------------------+
Then you click the "Email Actions" thing and get a runtime-derived list of available options. Not sure if I'll actually build that, but I think we can fairly throw the in-mail instructions away even if we don't go in that specific direction.
Test Plan: Grepped for `replyHandlerInstructions`, got no hits.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12229
Summary:
Ref T6755. This is a partial fix, but:
- Allow netblocks to be blacklisted instead of making the feature all-or-nothing.
- Default to disallow requests to all reserved private/local/special IP blocks. This should generally be a "safe" setting.
- Explain the risks better.
- Improve the errors rasied by Macro when failing.
- Removed `security.allow-outbound-http`, as it is superseded by this setting and is somewhat misleading.
- We still make outbound HTTP requests to OAuth.
- We still make outbound HTTP requests for repositories.
From a technical perspective:
- Separate URIs that are safe to link to or redirect to (basically, not "javascript://") from URIs that are safe to fetch (nothing in a private block).
- Add the default blacklist.
- Be more careful with response data in Macro fetching, and don't let the user see it if it isn't ultimately valid.
Additionally:
- I want to do this check before pulling repositories, but that's enough of a mess that it should go in a separate diff.
- The future implementation of T4190 needs to perform the fetch check.
Test Plan:
- Fetched a valid macro.
- Fetched a non-image, verified it didn't result in a viewable file.
- Fetched a private-ip-space image, got an error.
- Fetched a 404, got a useful-enough error without additional revealing response content (which is usually HTML anyway and not useful).
- Fetched a bad protocol, got an error.
- Linked to a local resource, a phriction page, a valid remote site, all worked.
- Linked to private IP space, which worked fine (we want to let you link and redierect to other private services, just not fetch them).
- Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6755
Differential Revision: https://secure.phabricator.com/D12136
Summary:
Fixes T7621. The engine selection code started out making sense, but didn't make as much sense by the time I was done with it.
Specifically, from the vanilla file upload, we may incorrectly try to write directly to the chunk storage engine. This is incorrect, and produces a confusing/bad error.
Make chunk storage engines explicit and don't try to do single-file one-shot writes to them.
Test Plan:
- Tried to upload a large file with vanilla uploader, got better error message.
- Uploaded small and large files with drag and drop.
- Viewed {nav Files > Help/Options}.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7621
Differential Revision: https://secure.phabricator.com/D12110
Summary:
Ref T7149. We can simplify configuration somewhat by removing the upload limit setting, now that we support arbitrarily large files.
- Merge configuration documentation.
- Tell users to set things to at least 32MB. This is 8MB maximum one-shot file + 4x headroom. Chunk sizes are 4MB.
Test Plan:
- Faked all the setup warnings.
- Read documentation.
- Uploaded some files.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12083
Summary:
Right now, if a daemon dies it can leave the setup warning around for like 10 minutes or something until we reap it.
Tighten the warning so we only care about actively running daemons.
Test Plan: Checked setup issues.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12088
Summary:
Fixes T5843. File storage engines use a very old "selector" mechanism which makes them difficult to extend.
This mechanism predates widespread use of `PhutilSymbolLoader` to discover available implementations at runtime. Runtime discovery has generally proven more flexible and easier to use than explicit selection (although it sometimes needs more UI to support it in cases where order or enabled/disabled flags can not be directly determined).
Use a modern runtime discovery mechanism instead of an explicit selector. This might break any installs which subclassed the `Selector`, but I believe almost no such installs exist, and they'll receive a meaningful exception upon upgrading (any custom engines will no longer implement all of the required methods).
Looking forward, this modernizes infrastructure to prepare for new "virtual" chunked-storage engines, with the eventual goal of supporting very large file uploads and data import into the Phacility cluster.
This uses D12051 to add UI to make it easier to understand the state of storage engines.
Test Plan:
Used new UI panel to assess storage engines:
{F336270}
- Uploaded a small file, saw it go to MySQL engine.
- Uploaded a larger file, saw it go to S3 engine.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5843
Differential Revision: https://secure.phabricator.com/D12053
Summary: See IRC. This check is somewhat misleading right now because it could arise from a mangled/broken Host header rather than a bad `phabricator.base-uri` configuration.
Test Plan: Faked this to trip, read all the text.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11894
Summary:
Fixes T7287. This trades off 4-byte character support for case insensitivity in these columns, which is a much better trade on the balance.
Also adds more warnings about old MySQL. Note that we already issue a warning when you run "storage adjust" (which I've made stronger) and already "strongly recommend" MySQL 5.5 or newer in the install documentation.
Test Plan:
- Ran `storage adjust --disable-utf8mb4` to go to old definitions, then ran `storage adjust` to get back to the new ones. Everything seemed OK in both cases.
- Verified that utf8mb4 data can be migrated out of these colums with `--unsafe` (which will truncate).
- Verified that manual explains this.
- Faked my way into the setup warning.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7287
Differential Revision: https://secure.phabricator.com/D11893
Summary: Ref T7352. This is pretty straightforward. I renamed `phd.start-taskmasters` to `phd.taskmasters` for clarity.
Test Plan:
- Ran `phd start`, `phd start --autoscale-reserve 0.25`, `phd restart --autoscale-reserve 0.25`, etc.
- Examined PID file to see options were passed.
- I'm defaulting this off (0 reserve) and making it a flag rather than an option because it's a very advanced feature which is probably not useful outside of instancing.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7352
Differential Revision: https://secure.phabricator.com/D11871
Summary:
Ref T7185. We currently have "locked", "masked", and "hidden" config.
However, "masked" does not really do anything. It was intended to mask values in DarkConsole, but Config got built out instead and "hidden" is strictly better in modern usage and protects against compromised administrator accounts. "hidden" implies "locked", so it's now strictly more powerful than just locked.
Remove "masked" and upgrade all "masked" config to "hidden". In particular, this hides some API keys and secret keys much more aggressively in Config, which is desirable.
Test Plan: Browsed things like S3 API keys in config and could no longer see them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7185
Differential Revision: https://secure.phabricator.com/D11763
Summary:
Ref T7152. Ref T1139. This updates Phabricator so third-party libraries can translate their own stuff. Also:
- Hide "All Caps" when not in development mode, since some users have found this a little confusing.
- With other changes, adds a "Raw Strings" mode (development mode only).
- Add an example silly translation to make sure the serious business flag works.
- Add a basic British English translation.
- Simplify handling of translation overrides.
Test Plan:
- Flipped serious business / development on and off and saw silly/development translations drop off.
- Switched to "All Caps" and saw all caps.
- Switched to Very English, Wow!
- Switched to British english and saw "colour".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7152, T1139
Differential Revision: https://secure.phabricator.com/D11747
Summary:
Ref T7184. I managed to write a phantom setup issue which fails normally and succeeds when looked at carefully, so clicking "you have open issues..." always cleared them. This made it very difficult to figure out what the problem was.
Show issue keys in the "title" attribute to make this sort of thing easier to deal with.
Test Plan: Moused over "You have issues..." text, saw issue key, quickly fixed issue with new information.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7184
Differential Revision: https://secure.phabricator.com/D11743
Summary: Groups setup issues into Important, PHP, MySQL, and Base for easier parsing on initial installations.
Test Plan:
Test my internal server and various issues.
{F289699}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7207
Differential Revision: https://secure.phabricator.com/D11726
Summary:
I'm hitting this in the cluster and couldn't figure it out after staring at it for a couple minutes. Produce a better error.
This dumps a hash of each configuration key value which is set to a non-default value into the daemon log. This is much more compact than the full config, and doesn't spread secrets around, so it seems like a good balance between providing information and going crazy with it.
Test Plan: {F284139}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11699
Summary:
Fixes T2380. Fixes T2382. Users should really configure this, but when we had a warning before a lot of users had trouble with it.
- Tout performance benefits.
- Document easy setup via CDN.
- We have an "Ignore" button now for users who really don't care.
Test Plan:
- Set up `admin.phacility.com` through AWS CloudFront (need a few changes to handle instances to put it on the cluster in general).
- Set up `secure.phabricator.com` through CloudFlare (almost; waiting for DNS).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T2382, T2380
Differential Revision: https://secure.phabricator.com/D11649
Summary:
Hit this locally, with an error like:
> Version <empty string> is older than 1.9, the minimum supported version.
(Where `<empty string>` was just the empty string, not literally the text `<empty string>`.)
Be more careful about parsing versions, and parse the newer string.
Test Plan: Got "unknown version" with intentionally-broken test data, then clean readout.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11558
Summary:
Fixes T6559. No more flash, use Websockets. This is less aggressive than the earlier version, and retains more server logic.
- Support "wss".
- Make the client work.
- Remove "notification.user" entirely.
- Seems ok?
Test Plan:
In Safari, Firefox and Chrome, saw the browsers connect. Made a bunch of comments/updates and saw notifications.
Notable holes in the test plan:
- Haven't tested "wss" yet. I'll do this on secure.
- Notifications are //too fast// now, locally. I get them after I hit submit but before the page reloads.
- There are probably some other rough edges, this is a fairly big patch.
Reviewers: joshuaspence, btrahan
Reviewed By: joshuaspence, btrahan
Subscribers: fabe, btrahan, epriestley
Maniphest Tasks: T6713, T6559
Differential Revision: https://secure.phabricator.com/D11143
Summary:
Ref T2783. When repository services are defined, skip the check for local repository storage.
In particular, in the Phacility cluster, this path won't exist on web nodes, but nothing will ever try to access it.
Test Plan: Verified new branch gets hit with cluster services defined.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11100
Summary:
Fixes T5196
If no phd.user is configured the behaviour is unchanged besides printing a warning when run as root (Usually i would add an exit(1) here but that would break existing installs who do that).
If phd.user is set and the current user is root it will run the daemon as: su USER -c "command" (I'm not sure if this works for every platform needed)
Otherwise it will refuse to start if configured and current user mismatch.
Test Plan: Stopped & Started phd daemon with various users and different phd.user settings including root
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: vinzent, epriestley
Maniphest Tasks: T5196
Differential Revision: https://secure.phabricator.com/D11036
Summary:
When the index does not exist and auto_create_index isn't
enabled, running ./bin/index results in a failure. That's
T5990
Instead create an index properly. This also allows us to do
nice things like do a proper mapping and analysis like for
substring matching like outlined by @fabe in T6552.
Test Plan:
Deleted and created index multiple times to verify
proper index creation and usage.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, manybubbles, chasemp, fabe, epriestley
Differential Revision: https://secure.phabricator.com/D10955
Summary: Ref: T6747 add a setup warning if mysql is configured with the default boolean syntax
Test Plan: Run setup checks with default mysql config and then add the desired config option, restart and check again.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T6747
Differential Revision: https://secure.phabricator.com/D11030
Summary: Fixes T6243.
Test Plan: simulated having this on by adding 'ONLY_FULL_GROUP_BY' to the $modes array. verified the error message looked nice {F233954}.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6243
Differential Revision: https://secure.phabricator.com/D10856
Summary: Fixes T3046. Git has no minimum or bad versions; svn has no minimum and the bad version of 1.7.1, and mercurial has a minimum of 1.9 and bad versions 2.2 and 2.1. These error messages are specific about the problem but do not include the details as to what specifically is bad about a given version. I don't think that's really necessary - the details don't help solve the problem and its probably booooooring at best to the average user. The details about a bad version are included in the code however.
Test Plan: hardcoded a VCS array to let me test all the VCS stuff. added some phlog() calls to make sure the VCS version parsing stuff was working correctly relative to the version(s) on my system. played around with setting minimum versions and bad versions and saw good results depending on what I set.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3046
Differential Revision: https://secure.phabricator.com/D10852
Summary: Fixes T6119. This is a little fuzzy, but generally bumping up `innodb_buffer_pool_size` to something bigger than the default (which is often anemic, at `8M`) is desriable, and it seems like it will fix the specific issue a user encountered in T6119.
Test Plan: {F211855}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6119
Differential Revision: https://secure.phabricator.com/D10630
Summary: Fixes T6210. The current messaging may be confusing if `pygmentize` is available but broken.
Test Plan: Faked the binary names and hit the errors, which seemed helpful.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6210
Differential Revision: https://secure.phabricator.com/D10626
Summary: Ref T6185. Although it seems that we can't easily defuse or mitigate this, we can at least warn administrators.
Test Plan: Ran on my (unpatched, local) system, got a setup warning.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6185
Differential Revision: https://secure.phabricator.com/D10561
Summary:
Fixes T6084. Changes:
- Rename `phabricator.show-beta-applications` to `phabricator.show-prototypes`, to reinforce that these include early-development applications.
- Migrate the config setting.
- Add an explicit "no support" banner to the config page.
- Rename "Beta" to "Prototype" in the UI.
- Use "bomb" icon instead of "half star" icon.
- Document prototype applications in more detail.
- Explicitly document that we do not support these applications.
Test Plan:
- Ran migration.
- Resolved "obsolete config" issue.
- Viewed config setting.
- Browsed prototypes in Applications app.
- Viewed documentation.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T6084
Differential Revision: https://secure.phabricator.com/D10493
