HomePhabricator

Updated git submodules

Authored by mmodell on Sep 7 2016, 10:07 PM.

Description

Updated git submodules

Project: phabricator/phabricator wmf/stable dbeb7d006398e88d09c81b627dd7873f6a5252f1

Merge 'upstream/master' into wmf/dev

Always show a menu on durable column

Summary: If you don't have any rooms, we currently show no menu and users have no logical means of closing the column. This lets "Hide Column" at least still appear. Fixes T9195

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T9195

Differential Revision: https://secure.phabricator.com/D16516

Recover from a race when importing external objects (like JIRA issues) for the first time

Summary:
Fixes T11604. If we send two requests to render a brand new tag at about the same time (say, 50ms apart) but JIRA takes more than 50ms to return from its API call, the two processes will race one another and try to save the same external object.

If they do, have whichever one lost the race just load the object the other one created.

Apply this to other bridges, too.

Test Plan:

  • Created a new task in JIRA.
  • Referenced it for the first time in Differential, in a comment.
  • This causes two tag renders to fire. This might be a bug but I spend 30 seconds on it without figuring out what was up. Regardless, we should fix the race even if the reason it's triggering so easily legitimately is a bug.
  • Before patch: big error dialog (as in T11604).
  • After patch: smooth sailing.

{F1804008}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11604

Differential Revision: https://secure.phabricator.com/D16514

Add a few more high contrast CSS rules

Summary: In new PHUITwoColumnLayout, some new colors were used that aren't in the High Contrast PostProcessor. Adding them and proper fallbacks.

Test Plan: High Contrast on/off on a timeline page.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16515

Super Pro Dexule fix for project icon timeline

Summary: It's super

Test Plan: Set lots of project icons

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16513

Properly set project icon in timeline

Summary: This fixes a bug where the icon won't display since it doesn't include fa-.

Test Plan: Change icon on a project, revisit timeline and see icon.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16512

Fix properties box layout for profiles/projects on mobile

Summary: The spacing here is incorrect and no longer needed.

Test Plan: Test phone layout of project, profile.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16511

Add a .. Remarkup rule for discussing keystrokes

Summary: Ruleset for styles in D16506.

Test Plan: {F1803883}

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D16510

Make storage adjustment a little nicer, especially the first time

Summary:
Fixes T11583.

  • When users run bin/storage upgrade for the first time on a new install, we currently give them a prompt which feels rough and which they can only reasonably ever answer "yes" to.
  • We generally use cautionary language ("found issues with schema") in this workflow. Adjustments are now routine, so use more neutral and progress-oriented language ("found adjustments to apply").

Test Plan:

  • Ran bin/storage upgrade --namesapce kappa123, got an adjustment using neutral language without prompting.
  • Dropped a key, ran bin/storage upgrade, got normal workflow (but with more neutral language).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11583

Differential Revision: https://secure.phabricator.com/D16509

Fix ContainsConstraints for array-valued custom field constraints in "*.search" methods

Summary: Fixes T11593. We ask for a list of values when searching for custom "link" fields, but don't handle it correctly when actually construcitng a query.

Test Plan:
Added this custom field:

{
  "mycompany.target-version": {
    "name": "Target Version",
    "type": "link",
    "search": true
  }
}

Set a task to "beta". Let daemons index it. Queried for:

constraints: {
  "custom.mycompany.target-version": [
    "beta"
  ]
}

Got just one result back.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11593

Differential Revision: https://secure.phabricator.com/D16508

Throw when callers pass an invalid constraint to a "*.search" method

Summary:
Ref T11593. When you call a *.search method like maniphest.search, we don't currently validate that all the constraints you pass are recognized.

I think there were two very weak arguments for not doing this:

  • It makes compatibility in arc across versions slightly easier: if we add a new constraint, we could add it to arc but also do client-side filtering for a while.
  • Conduit parameter types could, in theory, accept multiple inputs or optional/alias inputs.

These reasons are pretty fluff and T11593 is a concrete issue caused by not validating. Just validate instead.

Test Plan:

  • Made a maniphest.search call with a bogus constraint, got an explicit error about the bad constraint.
  • Made a maniphest.search call with a valid constraint ("ids").

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11593

Differential Revision: https://secure.phabricator.com/D16507

Basic kbd CSS for remarkup

Summary: Keyboard key CSS for Remarkup.

Test Plan:
edit test in html, see new CSS

{F1803550}

Reviewers: stratosgear, epriestley

Reviewed By: stratosgear, epriestley

Subscribers: stratosgear, Korvin

Differential Revision: https://secure.phabricator.com/D16506

Add a friendlier "in flight" error page

Summary:
Ref T11589. When we hit a fatal setup issue (essentially always a connection failure) after we've already survived them on at least one request, we can be pretty sure a server went down and that the problem is not a setup/configuration issue.

In this case, show a friendlier error page instead of the fairly detailed technical one.

Test Plan:

  • Broke MySQL config.
  • Restarted Apache.
  • Got the "admin/setup" error page:

{F1803268}

  • Fixed the MySQL config.
  • Loaded any page, to put us "in flight".
  • Broke MySQL config.
  • Loaded any page.
  • Got the friendly "in flight" error page:

{F1803271}

If you want to design this better, easiest way to get to it is:

  • Set mysql.port to 9999 in conf/local/local.json.
  • Reload any page while already running (don't restart).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11589

Differential Revision: https://secure.phabricator.com/D16503

Continue on bad database configuration from select scripts

Summary: Ref T11589. Provide a way for scripts to say "just continue if database config fails", and use it in bin/config and bin/storage.

Test Plan:

  • Broke database config.
  • Ran bin/config, worked fine.
  • Ran bin/storage, got helpful "set up the database" message.
  • Ran bin/repository, got fatal.
  • Ran normal site with valid/invalid config, got proper feedback.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11589

Differential Revision: https://secure.phabricator.com/D16502

Raise setup warnings immediately when failing to load configuration from the database

Summary:
Ref T11589. Previously, when we failed to load database configuration we just continued anyway, in order to get to setup checks so we could raise a better error.

There was a small chance that this could lead to pages running in a broken state, where ONLY that connection failed and everything else worked. This was accidentally fixed by narrowing the exceptions we continue on in D16489.

However, this "fix" meant that users no longer got helpful setup instructions. Instead:

  • Keep throwing these exceptions: it's bad to continue if we've failed to connect to the database.
  • However, catch them and turn them into setup errors.
  • Share all the setup code so these errors and setup check errors work the same way.

Test Plan:

  • Intentionally broke mysql.host and mysql.pass.
  • Loaded pages.
  • Got good setup errors.
  • Hit normal setup errors too.
  • Put everything back.
  • Swapped into cluster mode.
  • Intentionally broke cluster mode, saw failover to readonly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11589

Differential Revision: https://secure.phabricator.com/D16501

Split setup check phases into "preflight" and "post-config"

Summary:
Ref T11589. This runs:

  • preflight checks (critical checks: PHP version stuff, extensions);
  • configuration;
  • normal checks.

The PHP checks are split into critical ("bad version") and noncritical ("sub-optimal config").

I tidied up the extension checks slightly, we realistically depend on cURL nowadays.

Test Plan:

  • Faked a preflight failure.
  • Hit preflight check.
  • Got expected error screen.
  • Loaded normal pages.
  • Hit a normal setup check.
  • Used DarkConsole "Startup" tab to verify that preflight checks take <1ms to run (we run them on every page without caching, at least for now, but they only do trivial checks like PHP versions).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11589

Differential Revision: https://secure.phabricator.com/D16500

Add a "--copy" flag to "bin/files migrate"

Summary:
Ref T11596. When exporting data from the Phacility cluster, we bin/files migrate data from S3 into a database dump on the aux tier.

With current semantics, this moves the data and destroys it in S3.

Add a --copy flag to copy the data instead. This leaves the old copy around, which is what we want for exports.

Test Plan:

  • Ran bin/files migrate to go from blob to disk with --copy. Verified a copy was left in the database.
  • Copied it back, verified a copy was left on disk (total: 2 database copies, 1 disk copy).
  • Moved it back without copy, verified database was destroyed and disk was created (total: 1 database copy, 2 disk copies).
  • Moved it back without copy, verified local disk was destroyed and blob was created (total: 2 datbabase copies, 1 disk copy).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11596

Differential Revision: https://secure.phabricator.com/D16497

Merge branch 'upstream/master' into wmf/dev

Merge 'upstream/stable' into wmf/dev

Split setup checks into "preflight" and "normal" checks

Summary:
Ref T11589. Currently, initialization order is a bit tangled: we load configuration from the database, then later test if we can connect to the database.

Instead, I'm going to do: preflight checks ("PHP Version OK?", "Extensions installed?"), then configuration, then normal setup checks.

To prepare for this, flag core checks as "preflight" and add a setup panel to visually confirm that I didn't miss anything.

Test Plan: {F1803210}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11589

Differential Revision: https://secure.phabricator.com/D16499

Fix two "proably" typos

Summary: Caught one of these while reviewing docs, grepped for the other one.

Test Plan: grep, reading

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D16498

Add Slack authentication adapter

Summary: For phabricator. Adds a Slack auth adapater and icon.

Test Plan:
Create a new Slack Application for login, generate id and secret. Activate login and registration for Slack. Create a new account with Slack credentials. Log out. Log in with Slack credentials. Set my avatar with Slack. Slack. Slack.

{F1802649}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16496

(stable) Allow bin/storage renamespace to work with underscores

If the namespace is something like "test_example" we currently fail to
renamespace the dump.

(Cowboy committing this since this is currently blocking a data export.)

Test Plan:

  • Renamespaced a local dump, examined the output, saw 60 create / 60 use, reimported it.
  • Will export in production.

Auditors: chad

Allow bin/storage renamespace to work with underscores

If the namespace is something like "test_example" we currently fail to
renamespace the dump.

(Cowboy committing this since this is currently blocking a data export.)

Test Plan:

  • Renamespaced a local dump, examined the output, saw 60 create / 60 use, reimported it.
  • Will export in production.

Auditors: chad

Clean up some log spam caused by races in VersionedDraft

Summary:
Two minor issues that I caught in the log while fixing Phame permissions:

  • We had a JS bug which would cause us to immediately generate two comment previews at the exact same time -- one for loading the page, and one for "switching to desktop". Instead, only generate the "switch to desktop" preview if we really switched to desktop from a different device layout.
  • These two requests could end up reading/writing the VersionedDraft table at exactly the same time fairly often (e.g., after a comment submission, the page would load, send two preview requests at exactly the same time, and they'd race fairly reliably for me locally). If we do race, recover from the race.

Test Plan:
Submitted some Phame comments.

  • No more error log errors about VersionedDraft keys.
  • Saw only one preview request when loading the page instead of two.

Here's the specific stack trace I caught:

[Mon Sep 05 12:15:33.639930 2016] [:error] [pid 50608] [client 127.0.0.1:55278] [2016-09-05 14:15:33] EXCEPTION: (AphrontDuplicateKeyQueryException) #1062: Duplicate entry 'PHID-POST-fknnpzjnsdgc3rqobhst-PHID-USER-pr5rjpuilpfserepsd2k-13' for key 'key_object' at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:314]
[Mon Sep 05 12:15:33.640801 2016] [:error] [pid 50608] [client 127.0.0.1:55278] arcanist(head=master, ref.master=9e82ef979e81), corgi(head=master, ref.master=5b9171222bc9), instances(head=stable, ref.master=485bc8128198, ref.stable=2983bc917601), ledger(head=master, ref.master=4da4a24b8779), libcore(), phabricator(head=phame2, ref.master=4b6da9735ba7, ref.phame2=4b6da9735ba7), phutil(head=stable, ref.master=97f05269fdb1, ref.stable=c14343ee620e), services(head=stable, ref.master=1fcb5cdb7582, ref.stable=2d8088a5b4b3)
[Mon Sep 05 12:15:33.640815 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #0 <#2> AphrontBaseMySQLDatabaseConnection::throwCommonException(integer, string) called at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:348]
[Mon Sep 05 12:15:33.640830 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #1 <#2> AphrontBaseMySQLDatabaseConnection::throwQueryCodeException(integer, string) called at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:289]
[Mon Sep 05 12:15:33.640833 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #2 <#2> AphrontBaseMySQLDatabaseConnection::throwQueryException(mysqli) called at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:185]
[Mon Sep 05 12:15:33.640836 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #3 <#2> AphrontBaseMySQLDatabaseConnection::executeRawQuery(string) called at [<phutil>/src/xsprintf/queryfx.php:8]
[Mon Sep 05 12:15:33.640839 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #4 <#2> queryfx(AphrontMySQLiDatabaseConnection, string, string, string, array, string)
[Mon Sep 05 12:15:33.640841 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #5 <#2> call_user_func_array(string, array) called at [<phutil>/src/aphront/storage/connection/AphrontDatabaseConnection.php:42]
[Mon Sep 05 12:15:33.640844 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #6 <#2> AphrontDatabaseConnection::query(string, string, string, array, string) called at [<phabricator>/src/infrastructure/storage/lisk/LiskDAO.php:1261]
[Mon Sep 05 12:15:33.640846 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #7 <#2> LiskDAO::insertRecordIntoDatabase(string) called at [<phabricator>/src/infrastructure/storage/lisk/LiskDAO.php:1106]
[Mon Sep 05 12:15:33.640849 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #8 <#2> LiskDAO::insert() called at [<phabricator>/src/infrastructure/storage/lisk/LiskDAO.php:1075]
[Mon Sep 05 12:15:33.640851 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #9 <#2> LiskDAO::save() called at [<phabricator>/src/applications/draft/storage/PhabricatorVersionedDraft.php:65]
[Mon Sep 05 12:15:33.640854 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #10 <#2> PhabricatorVersionedDraft::loadOrCreateDraft(string, string, integer) called at [<phabricator>/src/applications/transactions/editengine/PhabricatorEditEngine.php:1669]
[Mon Sep 05 12:15:33.640857 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #11 <#2> PhabricatorEditEngine::buildCommentResponse(PhamePost) called at [<phabricator>/src/applications/transactions/editengine/PhabricatorEditEngine.php:894]
[Mon Sep 05 12:15:33.640859 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #12 <#2> PhabricatorEditEngine::buildResponse() called at [<phabricator>/src/applications/phame/controller/post/PhamePostEditController.php:60]
[Mon Sep 05 12:15:33.640862 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #13 <#2> PhamePostEditController::handleRequest(AphrontRequest) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:237]
[Mon Sep 05 12:15:33.640865 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #14 phlog(AphrontDuplicateKeyQueryException) called at [<phabricator>/src/aphront/handler/PhabricatorAjaxRequestExceptionHandler.php:27]
[Mon Sep 05 12:15:33.640868 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #15 PhabricatorAjaxRequestExceptionHandler::handleRequestException(AphrontRequest, AphrontDuplicateKeyQueryException) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:644]
[Mon Sep 05 12:15:33.640870 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #16 AphrontApplicationConfiguration::handleException(AphrontDuplicateKeyQueryException) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:242]
[Mon Sep 05 12:15:33.640873 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #17 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:149]
[Mon Sep 05 12:15:33.640879 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #18 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phabricator>/webroot/index.php:17]

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D16494

Remove overbearing policy checks in Phame

Summary:
Fixes T11584. This controller does unnecessary CAN_EDIT policy checks.

These checks are enforced by EditEngine, and you can make certain types of edits (including comments) even without full-blown edit permission.

Test Plan:

  • Commented as a user without edit permission.
  • Tried to edit as a user without edit permission, was rebuffed with a policy dialog.
  • Edited as a user with edit permission.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11584

Differential Revision: https://secure.phabricator.com/D16493

Ignore repository versions on inactive devices in "Repository Servers" panel in Config

Summary:
Fixes T11590. Currently, we incorrectly consider cluster repository versions that are (or were) on devices which are no longer part of the active cluster service when building this status screen.

Instead, ignore them. This is just a display bug; the actual ClusterEngine already had similar logic.

Test Plan:

  • Added a bad leader record to repository_workingcopyversion.
  • Before patch, got a bad "Partial (1w)" sync:

{F1802292}

  • After patch, got a good "Sycnchronized":

{F1802293}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11590

Differential Revision: https://secure.phabricator.com/D16492

Rebuild CSS packages

Summary: Review multimeter logs for new CSS/JS resources that should be in packages.

Test Plan: Review some pages in sandbox, no CSS errors.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16490

Use new Guide layout in Config->Welcome

Summary: Ref T11132, swaps in new UI for welcome page using guide modules

Test Plan: Test instance and non instance guides. Test each setting. Unclear on how to test people / Phacility. Just change the URL link?

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11132

Differential Revision: https://secure.phabricator.com/D16482

Fix Argument 4 passed to ManiphestTaskDetailController::buildPropertyView() must be of the type array

Summary:
Argument 4 passed to ManiphestTaskDetailController::buildPropertyView()
must be of the type array, object given, called in
/srv/phab/phabricator/src/applications/maniphest/controller/ManiphestTaskDetailController.php
on line 68 and defined

Change-Id: Ie394225525d1e4805d46c517fd1b2cbeaf3de8ec

Reviewers: mmodell

Reviewed By: mmodell

Subscribers: jenkins

Differential Revision: https://phabricator.wikimedia.org/D334

Merge 'upstream/stable' into wmf/dev

(stable) Rebuild Celerity map on stable.

(stable) Promote 2016 Week 36

Show broken units in revision history

Summary:
This is hacky, and I'm not sure I'm happy with it; Until T9365 is done, this will show up
broken tests with an appropriate star in the Revision History.

Test Plan: Created 1M messages in a couple of old diffs in a revision. The query took ~80us (On SSD drive).

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D16483

Distinguish between unreachable cluster database hosts and missing MySQL databases

Summary:
Fixes T11577. When we connect to a host and try to select a database which does not exist, we currently treat it as though the host wasn't reachable.

This isn't correct, and prevents storage from being initialized while already in cluster mode, since the "config" database won't exist yet the first time we connect.

Instead, distinguish between AphrontSchemaQueryException (thrown on connection if the requested database is not present) and other errors.

Test Plan:

  • Put Phabricator into cluster database mode (cluster.databases = ...).
  • Swapped storage.default-namespace to force initialization of a new install.
  • Ran bin/storage upgrade.
    • Before patch: Immediate fatal about unreachablility.
    • After patch: Database initialized.
  • Also ran initialization steps in tranditional single-host mode (cluster.databases empty, mysql.host configured).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11577

Differential Revision: https://secure.phabricator.com/D16489

Fix a Repository doc spelling mistake

Summary: Those letters don't go there!

Test Plan: O__O

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D16486

Provide a workflow to restart Harbormaster builds

Summary:
Ref T10867 for original use case. This workflow provides a plausible way for administrators to stop the daemons when performing upgrades or maintenance, then bring those daemons back up without resulting in the failure of builds that were running at the time.

On our organization's phab install, builds are running 24/7. The majority of these builds last for at least several minutes, and contain build steps which fail if interrupted and then resumed, as happens when turning daemons on and off.

Instead of allowing these build steps to resume execution as normal, this workflow will instruct active builds to restart their entire build process instead of just resuming whichever step they were on.

Test Plan:
contrived a build plan which would fail if resumed partway through:

  • lease a working copy
  • command touch restart_{build.id}
  • command test -e restart_{build.id} && rm restart_{build.id} && sleep 60

followed old procedure:

  • run a few of these builds manually
  • ./bin/phd stop
  • ./bin/phd start
  • saw the builds fail

followed new procedure:

  • run a few of these builds manually
  • ./bin/phd stop
  • ./bin/harbormaster restart --active
  • ./bin/phd start
  • saw the builds pass

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Maniphest Tasks: T10867

Differential Revision: https://secure.phabricator.com/D16485

Fix typo at calendar transaction

Summary:

  • Fixed conveted => converted

Ref T11576

Test Plan: * Looked at a page, where somebody converted an AllDay Event to a normal one

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Tags: ShoutWiki Calendar

Maniphest Tasks: T11576

Differential Revision: https://secure.phabricator.com/D16488

Update Diviner for array-valued @doc-stuff return values from DocblockParser

Summary: Ref T11575. After D16431, the parser may return arrays.

Test Plan: Ran bin/diviner generate --clean in phabricator/ without errors. Previously, this raised some parsing errors related to getting arrays where strings were expected.

Reviewers: chad, yelirekim, joshuaspence

Reviewed By: joshuaspence

Maniphest Tasks: T11575

Differential Revision: https://secure.phabricator.com/D16487

Support object mentions in Calendar Event descriptions

Summary:
Ref T7924. This:

  • Adds support for remarkup block changes to Modular Transactions.
  • Exposes remarkup changes from the Calendar event "Description" transaction.

This makes stuff like mentions and file embeds work properly.

Test Plan:
Mentioned a task in an event description, saw a mention appear on the task.

Uploaded a file to an event description, saw the file become "Attached" to the event.

(Neither of these worked properly before.)

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7924

Differential Revision: https://secure.phabricator.com/D16481

Correctly calculate "any_failed"

Summary: See T10746.

Test Plan: Fail one of several builds, run ./bin/harbormaster update, see that Build Status is Failed.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, O14 ATC Monitoring, yelirekim

Maniphest Tasks: T10746

Differential Revision: https://secure.phabricator.com/D16480

Add a basic first feed story on /home/

Summary: Ref T11132. Adds a text panel to feed if no stories are present and the user is an admin. Seems ok-ish for 15 minutes. Happy to take content suggestions.

Test Plan: Make a new install, see panel. Log in as new user, don't see panel.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11132

Differential Revision: https://secure.phabricator.com/D16479

Convert Guides to Modules

Summary: Splitting these up to re-use in Config as a stop gap.

Test Plan: Visit welcome, install, and quick start on guides app

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16478

Fix some more setIcon() issues

Summary: Fixes T11569. This fixes a known bad setIcon(). I also looked for more calls to setIcon() without success, and stubbed setIcon() so we're in good shape even if more exist.

Test Plan:

  • Grepped for setIcon( and manually inspect all 1,004 callsites to look for calls on PHUIObjectItemView objects.
  • Grepped for "high risk" callsites (setIcon in file after PHUIObjectItemView) and re-examined them. I identified these files with this command:
git ls-tree -r --name-only HEAD | xargs pcregrep -i -M -H -c --files-with-matches -o 'PHUIObjectItemView(.|\n)*setIcon'

There might be some more clever way to do that.

  • Since this only identified the callsites I already knew about and I don't have a ton of confidence that I didn't miss any, I put a stub in place that logs a deprecation warning. I'll file a followup to go clean these up in a month or so if the logs are clean.
  • Loaded Nuance, saw it work but warn.
  • Changed Nuance to use setStatusIcon(), loaded Nuance, no more fatal.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11569

Differential Revision: https://secure.phabricator.com/D16477

Correct spelling of "therefore"

Summary: Fixes T11565.

Test Plan: git grep -i therefor | grep -vi therefore

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11565

Differential Revision: https://secure.phabricator.com/D16476

Add some icons/color to Config Settings UI

Summary: This adds status icons, locked, hidden, editable, customized, to the list of options in config. Makes it easier to read and assertain state.

Test Plan:
View a hidden, customized, editable, and locked.

{F1796320}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16475

(stable) Update the resource map on "stable".

(stable) Fix search results with tables, fatals in Phortune

Summary: Previously we collapsed all table search results, but the new UI doesn't need it. Remove unused methods and fix CSS.

Test Plan: Legalpad Signatures, Phortune Accounts.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16469

Start actually showing the phurl urls to users

Summary: Fixes T10679. Added a 'short url' field to the phurl link page and changed the "view url" button to link to the shortened version

Test Plan: Create a phurl link (or navigate to an existing one) and note that there is now a field for "Short URL". Also verified that the "Visit URL" button in the top right still works as intended

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley, yelirekim

Maniphest Tasks: T10679

Differential Revision: https://secure.phabricator.com/D16473

Fix the feed line items for autodetect paste languages

Summary:
Fixes T11555. Previously changing the extension of a paste wouldn't change the syntax highlight language. Now it does.
Also, feed items involving autodetect weren't rendering in a readable way.

Test Plan: Created a paste named paste.php and let it autodetect language. Then edited the paste to be named paste.rainbow. It should now be highlighted in raiinnboow

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley, yelirekim

Maniphest Tasks: T11555

Differential Revision: https://secure.phabricator.com/D16474

Add a summary view of all repository errors to the repository cluster screen

Summary: Ref T11559. This makes managing large numbers of repositories slightly easier.

Test Plan: {F1796119}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11559

Differential Revision: https://secure.phabricator.com/D16472

Implement PhabricatorApplicationTransactionInterface on PhortunePaymentProviderConfig

Summary: Fixes T11556. This was just missing an implements ..., which became necessary at some point even for classes that don't use much of the beahvior (ModularTransactions?).

Test Plan: Created a new test payment provider on a Phortune merchant.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11556

Differential Revision: https://secure.phabricator.com/D16471

Revert "beautify repository search result view"

This reverts commit d29badbfc756fd262fbc0244afbe8c7f205dc11e.

Merge 'upstream/stable' into wmf/stable

Fixed Conflicts:
.arcconfig
src/applications/files/storage/PhabricatorFile.php
src/applications/maniphest/controller/ManiphestTaskDetailController.php
src/infrastructure/graph/PhabricatorObjectGraph.php
webroot/rsrc/image/sprite-menu-X2.png
webroot/rsrc/image/sprite-menu.png

Removed all instances of getIconURI

Summary: Fixes T11541. PhabricatorApplication::getIconURI() has been returning only null for a while (I assume in preparation to remove it). I removed the method and all the remaining call sites.

Test Plan: Removed the method and then clicked around. Things didn't explode!

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, yelirekim

Maniphest Tasks: T11541

Differential Revision: https://secure.phabricator.com/D16470

Fix search results with tables, fatals in Phortune

Summary: Previously we collapsed all table search results, but the new UI doesn't need it. Remove unused methods and fix CSS.

Test Plan: Legalpad Signatures, Phortune Accounts.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16469

Converted Paste language selection to a typeahead

Summary: Fixes T11532. The language selection for pastes is now a typeahead that is backed by pygments.dropdown-choices. There is still a bit of weirdness around making "auto-detection" the default state. To actually select a different language, you first need to remove the "auto detect" option that is pre-populated in a new paste. Other than that, it works as intended.

Test Plan:
Create a new paste with a file extension that can be auto-detected.
Created a new paste and manually selected the language
Edited a paste and changed the language.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley, yelirekim

Maniphest Tasks: T11532

Differential Revision: https://secure.phabricator.com/D16463

Redesign Config Application

Summary: Ref T11132, significantly cleans up the Config app, new layout, icons, spacing, etc. Some minor todos around re-designing "issues", mobile support, and maybe another pass at actual Group pages.

Test Plan: Visit and test every page in the config app, set new items, resolve setup issues, etc.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam, Korvin

Maniphest Tasks: T11132

Differential Revision: https://secure.phabricator.com/D16468

Revert "Fix inconsistent behavior in calendar recurrence sequence, add sequence nav buttons"

This reverts commit ff38787beb5c542c8f5c6dd89d390bd15f983c13 in preparation for merging
upstream changes which conflict. I may re-apply the patch after merging.

Move Setup Issues into it's own notification style menu

Summary: Ref T11132. This gets rid of the red bar for admins and instead shows a new menu item next to notifications/chat if there are unresolved configuration issues. Menu goes away if there are no issues. May move this later into the bell icon, but think think might be the right place to start especially for NUX and updates. Maybe limit the number of items?

Test Plan:
Tested with some, lots, and no config issues.

{F1790156}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11132

Differential Revision: https://secure.phabricator.com/D16461

Move Guides ObjectList styles to PHUIObjectItemListView

Summary: I plan to reuse these styles with Config, maybe also Almanac, etc.

Test Plan: Review /guides/, see same styles.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16467

Return Diffusion diffs through Files, not directly over Conduit

Summary:
Fixes T10423. Ref T11524. This changes diffusion.rawdiffquery to return a file PHID instead of a blob of data.

This is better in general, but particularly better for huge diffs (as in T10423) and diffs with non-utf8 data (as in T10423).

Test Plan:

  • Used bin/differential extract to extract a latin1 diff, got a clean diff.
  • Used bin/repository reparse --herald to rerun herald on a latin1 diff, got a clean result.
  • Pushed latin1 diffs to test commit hooks.
  • Triggered the the too large / too slow logic.
  • Viewed latin1 diffs in Diffusion.
  • Used "blame past this change" in Diffusion to hit the before logic.

Reviewers: chad

Reviewed By: chad

Subscribers: eadler

Maniphest Tasks: T10423, T11524

Differential Revision: https://secure.phabricator.com/D16460

Make logic for streaming VCS stuff directly to Files more reusable

Summary:
Ref T11524. Ref T10423. Earlier, I converted diffusion.filecontentquery to put the actual file content in Files, then return a PHID for the file, instead of trying to send the content over Conduit.

In T11524, we have a similar set of problems with diffs that contain non-UTF8 data (and, in T10423, diffs that are simply enormous).

I want to provide an API method to do the same sort of thing with diff output (like from git diff), so we call the method, it shoves the data in Files, and then we go pull it out of Files.

To support this, take the "shove the output of a Future into Files" logic and put it in a new base FileFuture query. This will let me make RawDiffQuery share the logic more easily.

Test Plan: Browsed Diffusion, ran diffusion.filecontentquery to fetch file content.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10423, T11524

Differential Revision: https://secure.phabricator.com/D16458

(stable) Promote 2016 Week 35

Converted the pinned applications selector to a typeahead.

Summary: Fixes T11513. Previously the selector was just a giant dropdown which was just... just too much. Now there's a handy typeahead.

Test Plan:
Happy Path:
Go to Settings -> Home Page -> Pin Application, start typing in the form then select one of the options. Click on "Pin Application". The application should now be in the list.

Other paths:

  • Type nothing into the box and submit, nothing should happen.
  • Choose an application that is already pinned. The list should stay the same.
  • Type nonsense into the box and submit, nothing should happen.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: chad, Korvin, epriestley, yelirekim

Maniphest Tasks: T11513

Differential Revision: https://secure.phabricator.com/D16459

Use phutil_json_encode() in AphrontResponse to raise better errors

Summary:
Ref T11524. This problem was more difficult to diagnose than necessary because we swallow errors silently in AphontResponse when emitting JSON responses.

Instead of using json_encode(), use phutil_json_encode() which throws on failure.

Test Plan:
Old behavior was HTTP 200 with no body.

New behavior is HTTP 500 with this message:

[2016-08-26 07:33:59] EXCEPTION: (HTTPFutureHTTPResponseStatus) [HTTP/500] Internal Server Error
Exception: Failed to JSON encode value (#5: Malformed UTF-8 characters, possibly incorrectly encoded): Dictionary value at key &quot;result&quot; is not valid UTF8, and cannot be JSON encoded: diff --git a/latin1.txt b/latin1.txt
new file mode 100644
index 0000000..ce6c927
--- /dev/null
+++ b/latin1.txt
@@ -0,0 +1 @@
+&lt;�&gt;
. at [<phutil>/src/future/http/BaseHTTPFuture.php:339]

Reviewers: chad, avivey

Reviewed By: avivey

Maniphest Tasks: T11524

Differential Revision: https://secure.phabricator.com/D16457

When importing Git repositories, treat out-of-range timestamps as the current time

Summary:
Fixes T11537. See that task for discussion.

Although we could accommodate these faithfully, it requires a huge migration and affects one repository on one install which was written with buggy tools.

At least for now, just replace out-of-32-bit-range epoch values with the current time, which is often somewhat close to the real value.

Test Plan:

  • Following the instructions in T11537, created commits in 40,000 AD.
  • Tried to import them, reproducing the "epoch" database issue.
  • Applied the patch.
  • Successfully imported future-commits, with some liberties around commit dates. Note that author date (not stored in an epoch column) is still shown faithfully:

{F1789302}

Reviewers: chad, avivey

Reviewed By: avivey

Maniphest Tasks: T11537

Differential Revision: https://secure.phabricator.com/D16456

Add a setup warning for "always_populate_raw_post_data"

Summary:
Fixes T9235. When the stars align, PHP 5.6 or newer emits a deprecation warning on startup about "always_populate_raw_post_data" which occurs too early for us to intercept and can break responses by adding garbage to the output.

These settings appear to be sufficient:

always_populate_raw_post_data = 1
display_errors = 1
display_startup_errors = 1
error_reporting = -1

Then make a request with an unusual content type:

$ curl -X POST -H "Content-Type: application/json" -d "{foo: bar}" http://phabricator.example.com/

This triggers the warning:

<br />
<b>Deprecated</b>:  Automatically populating $HTTP_RAW_POST_DATA is deprecated and will be removed in a future version. To avoid this warning set 'always_populate_raw_post_data' to '-1' in php.ini and use the php://input stream instead. in <b>Unknown</b> on line <b>0</b><br />
<br />
...

To avoid this, just instruct administrators to set this value to "-1", which completely disables the feature and silences the warning.

Test Plan:

  • Reproduced this issue by following the instructions above.
  • Triggered the setup issue locally and read all the captivating prose:

{F1786911}

  • Made the configuration change it directed me to, saw the setup issue resolve.

Reviewers: jcox

Reviewed By: jcox

Maniphest Tasks: T9235

Differential Revision: https://secure.phabricator.com/D16454

Wrap long text in setup issues

Summary: Really long text strings break this ui when first setting up Phabricator.

Test Plan: MySQL sql_mode setup issue.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16455

Added autopatch to remove ponder vote data

Summary: Fixes T9117. Adds a migration to remove ponder vote data.

Test Plan: I added a bunch of lines to phabricator_user.edge with type 18 and they were successfully removed by this patch

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, yelirekim

Maniphest Tasks: T9117

Differential Revision: https://secure.phabricator.com/D16452

Add setup checks for unused homepage options

Summary: Ref T11533, Fixes T5315. Remove and add extra setup checks for removed homepage options.

Test Plan: Review text.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T5315, T11533

Differential Revision: https://secure.phabricator.com/D16453

New 'default' homepage

Summary: Ref T11132. This is a new default default (no dashboard) homepage. It offers (Diffs) (Tasks) (Repositories) in the main column and (Feed) in the side column. No NUX stuff, No logged out public view (upcoming diff). This should be complete, but unclear how to bucketize Differential.

Test Plan: Test new account's default homepage.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11132

Differential Revision: https://secure.phabricator.com/D16449

Update People for handleRequest

Summary: Ref T8628. Updates people controllers for handleRequest

Test Plan: Viewed the people list, viewed the activity logs, then went through the approval process for a new user account.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, yelirekim

Maniphest Tasks: T8628

Differential Revision: https://secure.phabricator.com/D16451

Added a setup check for empty REMOTE_ADDR

Summary: Fixes T8850. Previously, if a user's preamble script mangled $_SERVER['REMOTE_ADDR'] or somehow set it to null, the user would get errors when performing certain actions. Now those errors shouldn't occur, and instead the user will be warned that there is a setup issue related to their preamble script.

Test Plan: Create a preamble script that contains $_SERVER['REMOTE_ADDR'] = null; then navigate to /config/issue/. There should be a warning there about REMOTE_ADDR not being available.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, yelirekim, epriestley

Maniphest Tasks: T8850

Differential Revision: https://secure.phabricator.com/D16450

Added application name to the typeahead results for doc type search

Summary: Ref T10951. This adds the application name as an attribute below the document type in the UI for doc type search.

Test Plan: Verify that the application name appears as an attribute on the document type results.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Maniphest Tasks: T10951

Differential Revision: https://secure.phabricator.com/D16446

Remove unused apps from the DocumentType typeahead

Summary: Ref T10951. This diff removes uninstalled applications from the result set for DocumentType restults

Test Plan: Uninstall an application (diviner for example), then go to the document type search menu and ensure that the uninstalled application doesn't show up.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Maniphest Tasks: T10951

Differential Revision: https://secure.phabricator.com/D16445

Added high security requirement to add/delete email addresses

Summary: Fixes T10999. Now MFA will be required for all email address related operations.

Test Plan: Ensure that adding and removing email addresses now requires you to enter high security mode.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Maniphest Tasks: T10999

Differential Revision: https://secure.phabricator.com/D16444

Updated the docs so chatbots can use the Conduit API

Summary: Previously, the chatbot docs instructed users to get certificates for the conduit API and put the cert in a conduit.cert config key. In order to get the chatbot to work, I needed to instead get an API key and put it in the conduit.token config entry.

Test Plan: Doc fix. Tried the new documented way and it worked.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D16443

Fix app icons in homepage settings

Summary: These were blank, from last week's shenanigans.

Test Plan: View homepage settings, see icons.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16447

Make the chatbot obey the object name blacklist

Summary: Fixes T11508. The config entry remarkup.ignored-object-names already contains a blacklist of object names that should be ignored in the web UI. This change makes that blacklist also apply to the chatbot. This makes it possible to have a chatbot ignore things like V1, V2, Q1 and any other phrases the user may not want to generate links to objects.

Test Plan: Create objects (tasks, slowvotes, etc.) then mention the object names in chat (with the bot running). The bot should respond with helpful links to the given objects. Then add the object names to the blacklist through the config web UI. This apparently triggers the bot to restart itself. Then mention the object names in chat again. The bot should no longer respond with links because those object names have been added to the blacklist regex.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley

Maniphest Tasks: T11508

Differential Revision: https://secure.phabricator.com/D16442

Document the use of repository commit hints

Summary: Ref T11522. This explains how to actually use bin/repository hint.

Test Plan: Read the document. Used bin/repository hint as directed.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11522

Differential Revision: https://secure.phabricator.com/D16441

When commits have a "rewritten" hint, try to show that in handles in other applications

Summary:
Ref T11522. This tries to reduce the cost of rewriting a repository by making handles smarter about rewritten commits.

When a handle references an unreachable commit, try to load a rewrite hint for the commit. If we find one, change the handle name to "OldHash > NewHash" to provide a strong hint that the commit was rewritten and that copy/pasting the old hash (say, to the CLI) won't work.

I think this notation isn't totally self-evident, but users can click it to see the big error message on the page, and it's at least obvious that something weird is going on, which I think is the important part.

Some possible future work:

  • Not sure this ("Recycling Symbol") is the best symbol? Seems sort of reasonable but mabye there's a better one.
  • Putting this information directly on the hovercard could help explain what this means.

Test Plan: {F1780719}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11522

Differential Revision: https://secure.phabricator.com/D16437

When a commit has a "rewritten" hint, show it in the UI instead of the generic "deleted" message

Summary:
Ref T11522. When a commit is no longer reachable from any branch/tag, we currently show a "this has been deleted" message.

Instead, go further: check if there is a "rewritten" hint pointing at a commit the current commit was rewritten into. If we find one, show a message about that instead.

(This isn't super pretty, just getting it working for now. I expect to revisit this UI in T9713 if we don't get to it before that.)

Test Plan: {F1780703}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11522

Differential Revision: https://secure.phabricator.com/D16436

Migrate the "badcommit" table to use the less-hacky "hint" mechanism

Summary: Ref T11522. This migrates any "badcommit" data (which probably only exists at Facebook and on 1-2 other installs in the wild) to the new "hint" table.

Test Plan:

  • Wrote some bad commit annotations to the badcommit table.
  • Viewed them in the web UI and used bin/repository reparse --change ... to reparse them. Saw "this is bad" messages.
  • Ran migration, verified that valid "badcommit" rows were successfully migrated to become "hint" rows.
  • Viewed the new web UI and re-parsed the change, saw "unreadable commit" messages.
  • Viewed a good commit; reparsed a good commit.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11522

Differential Revision: https://secure.phabricator.com/D16435

Provide a new "hint" table for weird commits (rewritten, unreadable)

Summary:
Ref T11522. This provides storage for tracking rewritten commits (new feature) and unreadable commits (existing feature, but really hacky).

This doesn't do anything yet, just adds a table and a CLI tool for updating it. I'll document the tool once it works. You just pipe in some JSON, but I need to document the format.

Test Plan:

  • Piped JSON for "none", "rewritten" and "unreadable" hints into bin/repository hint.
  • Examined the database to see that the table was written properly.
  • Tried to pipe bad JSON in, invalid hint types, etc. Got reasonable human-readable error messages.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11522

Differential Revision: https://secure.phabricator.com/D16434

Removed unused buildApplicationPage method from PhabricatorController

Summary: Getting rid of some code! This method has no callsites so it should be safe to remove completely. Ref T9690

Test Plan: Removed method and clicked around to make sure nothing broke.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: yelirekim, epriestley

Maniphest Tasks: T9690

Differential Revision: https://secure.phabricator.com/D16439

Add patch to remove conduit_connectionlog table (Fixes T9982)

Summary: Adds a schema patch that removes conduit_connectionlog. This table hasn't been used in 8ish months so it's probably safe to get rid of.

Test Plan: Apply the patch locally and confirm that the table does indeed get dropped.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D16438

Removing deprecated method calls

Summary: Removed call to the deprecated buildStandardPageResponse method from XHProfProfileController

Test Plan: Install, configure, and use XHProf. I'll need some guidance with this

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D16432

Update Celerity map parser for new docblock code

Summary:
After D16431, listing the same @annotation multiple times makes the docblock parser return a list.

We have some resources which list @requires or @provides several times, but don't handle the new parser properly. Make the code more flexible, since this is a reasonable way to specify the annotations.

See also D16432. This produces a failure in this form:

[2016-08-23 21:10:15] ERROR 2: trim() expects parameter 1 to be string, array given at [/core/data/drydock/workingcopy-74/repo/phabricator/src/applications/celerity/CelerityResourceMapGenerator.php:236]
2	arcanist(head=master, ref.master=89e8b4852384), phabricator(head=6c940fb71b0a8850c6a1b7f5fc642a8f8135a76a, ref.master=b521f2349e46), phutil(head=master, ref.master=237549280f08)
3	  #0 trim(array) called at [<phabricator>/src/applications/celerity/CelerityResourceMapGenerator.php:236]
4	  #1 CelerityResourceMapGenerator::getProvidesAndRequires(string, string) called at [<phabricator>/src/applications/celerity/CelerityResourceMapGenerator.php:193]
5	  #2 CelerityResourceMapGenerator::rebuildTextResources(CelerityPhabricatorResources, CelerityResourceTransformer) called at [<phabricator>/src/applications/celerity/CelerityResourceMapGenerator.php:54]
6	  #3 CelerityResourceMapGenerator::generate() called at [<phabricator>/src/__tests__/PhabricatorCelerityTestCase.php:16]
7	  #4 PhabricatorCelerityTestCase::testCelerityMaps()
8	  #5 call_user_func_array(array, array) called at [<arcanist>/src/unit/engine/phutil/PhutilTestCase.php:492]
9	  #6 PhutilTestCase::run() called at [<arcanist>/src/unit/engine/PhutilUnitTestEngine.php:69]
10	  #7 PhutilUnitTestEngine::run() called at [<arcanist>/src/unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php:147]
11	  #8 ArcanistConfigurationDrivenUnitTestEngine::run() called at [<arcanist>/src/workflow/ArcanistUnitWorkflow.php:167]
12	  #9 ArcanistUnitWorkflow::run() called at [<arcanist>/scripts/arcanist.php:394]

Test Plan: Ran bin/celerity map, no more warnings and no change to the actual map.

Reviewers: joshuaspence, chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D16433

Explain how cats use their time

Summary: Added a brand new shiny cat fact

Test Plan: Pulled up a project with motivator installed and nothing broke

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D16430

Update Config Application UI

Summary: Switches over to new property UI boxes, splits core and apps into separate pages. Move Versions into "All Settings". I think there is some docs I likely need to update here as well.

Test Plan: Click on each item in the sidebar, see new headers.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16429

(stable) Promote 2016 Week 34

Add a "--force" flag to "bin/repository move-paths"

Summary:
Ref T7148. The automated export process runs this via daemon, which can't answer "Y" to this prompt. Let it "--force" instead.

(Some of my test instances didn't have any repositories, which is why I didn't catch this sooner.)

Test Plan: Ran bin/repository move-paths --force ..., saw change applied without a prompt.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7148

Differential Revision: https://secure.phabricator.com/D16426

Rough in of NUX Guide steps

Summary: Ref T11132. Will work on CSS tomorrow, but wanted to rough in the UI Steps to get guidance. Not sure what you have in mind for the "app" part, if you want to explain it and I build or you build.

Test Plan: Visit each page and click on links. Very rough and unfinished.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11132

Differential Revision: https://secure.phabricator.com/D16419

Add setBackground to PHUIIconView

Summary: Ref T11132. Adds a background color option to PHUIIconView, for use whereever, and NUX. Also normalize icon placement for mixed image/icon result list.

Test Plan: Test in UIExamples, and Global Settings.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11132

Differential Revision: https://secure.phabricator.com/D16424

Add a missing table key to improve performance of "Recently Completed Tasks" query

Summary:
Fixes T11490. Currently, this query can not use a key and the table size may be quite large.

Adjust the query so it can use a key for both selection and ordering, and add that key.

Test Plan: Ran EXPLAIN on the old query in production, then added the key and ran EXPLAIN on the new query. Saw key in use, and "rows" examined drop from 29,273 to 15.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11490

Differential Revision: https://secure.phabricator.com/D16423

Simplify how tag lists manage their handles

Summary: Fixes T11493. This code is a little bit weird/clever, simplify it so that we always cast the handles to an array early on.

Test Plan: {F1767668}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11493

Differential Revision: https://secure.phabricator.com/D16422

Fix getIcon calls in PHUIObjectListItem

Summary: Fixes T11501. Let's you pass in a full PHUIIconView or just the icon name to give ObjectListItem a large icon.

Test Plan: Alamanac, Applications, Drydock, Settings, Search Typeahead, Config page...

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T11501

Differential Revision: https://secure.phabricator.com/D16421

Keep setIcon() working for now on PHUIObjectItemView

Summary:
Ref T11501. This method was removed in D16418, but still has some callsites. I know of four:

  • Config
  • Settings
  • Drydock main page
  • Almanac main page

Since I might be missing some and it's close to the release cut, just put the method back for now until we can clean it up more properly.

Test Plan: Viewed Settings, Config, Drydock, Almanac. No more fatal on this method being missing.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11501

Differential Revision: https://secure.phabricator.com/D16420

Simplify PHUIObjectItemList a bit

Summary: I don't think we use footicons, removing that CSS. States were added but only used in Auth, convert them to statusIcon instead.

Test Plan: Visit Auth, UIExamples, grep for setState

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16418

Restrict aprhont-form-width to certain input types

Summary: Fixes T11491, I believe these are the only two that need to be specified.

Test Plan: Checked Create pages for diffs, tasks, projects, events, blogs, login, etc. Mobile/Desktop.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11491

Differential Revision: https://secure.phabricator.com/D16416

Rough in Guides Application

Summary: Ref T11132, Ref T11478. Builds out a basic PHUICMSView and Guides Application, no content / modules.

Test Plan: Go to /guides/, see blank states for new guides.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11132, T11478

Differential Revision: https://secure.phabricator.com/D16414

Provide "--output" flags for "bin/storage renamespace"

Summary:
Ref T6996. Depends on D16407. This does the same stuff as D16407, but for bin/storage renamespace. In particular:

  • Support writing directly to a file (so we can get good errors on failure).
  • Support in-process compression.

Also add support for reading out of a storage dump subprocess, so we don't have to do a dump-to-disk + renamespace + compress dance and can just stream out of MySQL directly to a compressed file on disk.

This is used in the second stage of instance exports (see T7148).

It would be nice to share more code with bin/storage dump, and possibly to just make this a flag for it, although we still do need to do the file-based version when importing (vs exporting). I figured that was better left for another time.

Test Plan:
Ran bin/storage renamespace --live --output x --from A --to B --compress --overwrite and similar commands.

Verified that a compressed, renamespaced dump came out of the other end.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T6996

Differential Revision: https://secure.phabricator.com/D16410

Provide an "--output" mode for "bin/storage dump" with better error handling

Summary:
Ref T6996. If you do this kind of thing in the shell, you don't get a good error by default if the dump command fails:

$ bin/storage dump | gzip > output.sql.gz

This can be worked around with some elaborate bash tricks, but they're really clunky and uninintuitive.

We also need to do this in several places (while writing backups; while performing exports), and I don't want to copy clunky bash tricks all over the codebase.

Instead, provide --output and --compress flags which just do this processing inside bin/storage dump. It will fail appropriately if any of the underlying operations fail. This also makes the write a little safer (refuses to overwrite) and the code more reusable.

Test Plan:

  • Did three dumps, with no flags, --output, and --output --compress.
  • Verified all three took similar amounts of time and were identical except for "Date Exported" timestamps in comments (except that the compressed one was compressed).
  • Used gunzip to examine the compressed one, verified it was really compressed.
  • Faked a write error, saw properly command behavior (clean up file + exit with error).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T6996

Differential Revision: https://secure.phabricator.com/D16407

Fix Herald test adapter for commits

Summary:
Fixes T11488. I broke this in D16360, I think by doing a little extra refactoring after testing it.

This code is very old, before commits always needed to have repositories attached in order to do policy checks.

Modernize it by mostly just using the repository which is present on the Commit object, and using the existing edge cache.

Test Plan: Ran a commit through the Herald test adapter.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11488

Differential Revision: https://secure.phabricator.com/D16413

Fix a typo in "Internationalization" documentation

Summary: Caught this while linking to it from D16405.

Test Plan: Consulted a dictionary.

Reviewers: chad, alexmv

Reviewed By: alexmv

Differential Revision: https://secure.phabricator.com/D16406

Update Preamble documentation for clusters with mixed request sources and loadbalancer chains

Summary:
Fixes T11487. Improve documentation for three situations:

  • When you configure a cluster behind a load balancer, all requests are trusted but not all have an "X-Forwarded-For" header. Change the suggested snippet to read this header only if it exists.
  • When a request goes through a series of load balancers (as with a CDN) they can end up writing a list of IPs to the header. Parse these.
  • Remove the "rate limiting" stuff -- this got disabled/removed a long time ago and is misleading/incorrect.

Test Plan: Read documentation.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11487

Differential Revision: https://secure.phabricator.com/D16403

Document how to manually close revisions

Summary: Fixes T11484. These mechanisms aren't necessarily obvious and make sense to document here.

Test Plan: Read documentation.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11484

Differential Revision: https://secure.phabricator.com/D16404

Convert some whiny exceptions into quiet MalformedRequest exceptions

Summary:
Fixes T11480. This cleans up the error logs a little by quieting three common errors which are really malformed requests:

  • The CSRF error happens when bots hit anything which does write checks.
  • The "wrong cookie domain" errors happen when bots try to use the security.alternate-file-domain to browse stuff like /auth/start/.
  • The "no phcid" errors happen when bots try to go through the login flow.

All of these are clearly communicated to human users, commonly encountered by bots, and not useful to log.

I collapsed the CSRFException type into a standard malformed request exception, since nothing catches it and I can't really come up with a reason why anything would ever care.

Test Plan:
Hit each error through some level of curl -H ... and/or fakery. Verified that they showed to users before/after, but no longer log.

Hit some other real errors, verified that they log.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11480

Differential Revision: https://secure.phabricator.com/D16402

Correct various spelling errors

Summary: Fixes T11477.

Test Plan: Grep for Mulitple

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11477

Differential Revision: https://secure.phabricator.com/D16399

Fix bad array index test in Differential package code

Summary: This needs an isset() for cases when authority and packages don't completely overlap.

Test Plan:

  • With a package set to trigger autoreview, created a revision.
  • Observed error log, saw no more error.
  • Saw package trigger autoreview properly.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D16398

Survive receipt of too much workboard column position information

Summary:
Fixes T11468. I was able to find a reproduction case for the issue, at least, described in my comments there.

When the server sends back position information for columns we aren't displaying, for whatever reason, just ignore that information.

Future work in T4900 to do more synchronization may revisit this.

Test Plan: See T11468#190177 for steps. Did that stuff with this patch, got a clean edit.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11468

Differential Revision: https://secure.phabricator.com/D16397

Don't allow empty list constraints in Conduit calls

Summary:
Ref T11473. If you write a method like get_stuff(ids) and then call it with an empty list of IDs, you can end up passing an empty constraint to Conduit.

If you run a *.search method with such a constraint, like this one:

{
  "ids": []
}

...we have three possible beahviors:

  1. Treat it like the user passed no constraint (basically, ignore the constraint).
  2. Respect the constraint (return no results).
  3. Error.

Currently, we do (1). However, this is pretty confusing and I think clearly the worst option, since it means get_stuff(array()) in client code will often tend to return a ton of results.

We could do (2) instead, but this is also sort of confusing (it may not be obvious why nothing matched, even though it's an application bug) and I think most reasonable client code should be doing an if ($ids) test: this test makes clients a little more complicated, but they can save a network call, and I think they often need to do this test anyway (for example, to show the user a different message).

This implements (3), and just considers these to be errors: this is the least tricky behavior, it's consistent with what we do in PHP, makes fairly good sense, and the only cost for this is that client code may need to be slightly more complex, but this slightly more complex code is usually better code.

Test Plan: Ran Conduit *.search queries with "ids":[] and "phids":[], got sensible errors instead of runaway result sets.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11473

Differential Revision: https://secure.phabricator.com/D16396

Execute Harbormaster buildable filtering properly from HarbormasterBuildSearchEngine

Summary:
Ref T11473. When running harbormaster.build.search with a buildables constraint, the constraint doesn't get passed to the Query and so currently has no effect.

This piece of logic was just accidentally omitted from D16356. It is probably not used anywhere today and doesn't show up in the UI, so it's easy to overlook (I missed it in review, too).

Test Plan: Ran harbormaster.build.search with a buildables constraint, got expected filtering.

Reviewers: yelirekim, chad

Reviewed By: chad

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T11473

Differential Revision: https://secure.phabricator.com/D16395

(stable) Promote 2016 Week 33

Make "Core Applications" more reasonable

Summary: Ref T11132, cleaning up what "Core Applications" means.

Test Plan: Visit /applications/, see less poseurs.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11132

Differential Revision: https://secure.phabricator.com/D16394

Fix scrollbar in filetree sidenav

Summary: Fixes T11457, I guess. It feels wierd when you drag it a bit, and Im not positive if it's the calc css or javascript.

Test Plan: Enable filetree, shorten browser, see scrolling appear if files too long.

Reviewers: avivey, epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11457

Differential Revision: https://secure.phabricator.com/D16393

List both hosted and observed repositories in "Cluster Repository Status" configuration console

Summary:
When I wrote this the first time, only hosted repositories could be clustered.

This check wasn't removed when I allowed observed repositories to be clustered in D15986.

Test Plan:
Reloaded ConfigRepository Servers page, saw more stuff locally.

Reviewed the cardinal digits between 1 and 17, inclusive.

Reviewers: chad, avivey

Reviewed By: avivey

Differential Revision: https://secure.phabricator.com/D16392

When already running as the daemon user, don't "sudo" daemon commands

Summary:
The cluster synchronization code runs either actively (before returning a response to git clone, for example) or passively (routinely, as the daemons update reposiories).

The active sync runs as the web user (if running git clone http://...) or the VCS user (if running git clone ssh://...). But the passive sync runs as the daemon user.

All of these sync processes need to run actual commands as the daemon user (git fetch ...).

For the active ones, we must sudo.

For the passive ones, we're already the right user. We run the same code, and end up trying to sudo to ourselves, which sudo isn't happy about by default.

Depending on how sudo is configured and which users things are running as this might work anyway, but it's silly and if it doesn't work it requires you to go make non-obvious, weird config changes that are unintuitive and somewhat nonsensical. This is probably worse on the balance than adding a bit of complexity to the code.

Instead, test which user we're running as. If it's already the right user, don't sudo.

Test Plan:

  • Ran bin/repository update --trace as daemon user, saw no more sudo.
  • Ran a git clone to make sure that didn't break.

Reviewers: chad, avivey

Reviewed By: avivey

Differential Revision: https://secure.phabricator.com/D16391

Fix a bad DiffusionCommandEngine parameter from HTTPEngine conversion

Summary:
I converted this call incorrectly in D16092. We should pass the PhutilURI object, not the string version of it.

Specifically, this resulted in hitting an error like this if a replica needed synchronization:

[2016-08-11 21:22:37] EXCEPTION: (InvalidArgumentException) Argument 1 passed to DiffusionCommandEngine::setURI() must be an instance of PhutilURI, string given, called in...
#0 PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<phabricator>/src/applications/diffusion/protocol/DiffusionCommandEngine.php:52]
#1 DiffusionCommandEngine::setURI(string) called at [<phabricator>/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php:601]
...

Test Plan: Clusterized an observed repository, demoted a node, ran bin/repository update Rxxx to update, saw no typehint fatal.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D16390

In taskmaster daemons, only close connections which were not used recently

Summary:
Ref T11458. Depends on D16388. Currently, we're very aggressive about closing connections in the taskmaster daemons.

This can end up taking up a lot of resources. In particular, because the outgoing port for outbound connections normally can not be reused for 60 seconds after a connection closes, we may exhaust outbound ports on the host if there's a big queue full of stuff that's being processed very quickly.

At a minimum, we always are holding open a worker connection, which we always need again right away. So even in the best case we end up opening/closing this about once per second and each daemon takes up about ~60 outbound ports when it should take up ~1.

So, make two adjustments:

  • First, only close connections which we haven't issued a query on in the last 60 seconds. This should prevent us from closing connections that we'll need again immediately in most cases. In the worst case, we shouldn't be eating up any extra ports under default TCP behavior.
  • Second, explicitly close connections. We were relying on implicit/GC behavior (maybe as a holdover from very long ago, before we got connection wrappers in place?), which probably did about the same thing but isn't as predictable and can't be profiled or instrumented.

Test Plan:
This is somewhat difficult to test completely convincingly in isolation since the problem behavior depends on production scales and the workload, and to some degree on configuration.

I tested that this stuff baiscally works by adding logging to connect/close and running the daemons, verifying that they churned connections a lot before this change (e.g., ~1/s even at no load) and churn rarely afterward (e.g., almost never at no load).

I ran some workload through them to make sure I didn't completely break anything.

The best real test is just seeing how production responds. Current inbound/outbound connections on secure001 are 1,200:

secure001 $ netstat -t | grep :mysql | wc -l
1164

Current outbound from repo001 are 18,600:

repo001 $ netstat -t | grep :mysql | wc -l
18663

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11458

Differential Revision: https://secure.phabricator.com/D16389

Fix misleading error message when only cluster database masters are configured

Summary:
Fixes T11446. We can raise the misleading error:

No valid databases are configured!

...when a valid master is configured but unreachable.

Instead, more carefully raise either "nothing is configured" or "nothing is reachable".

Test Plan: Configured only a master, artificially severed it, got "nothing is reachable" instead of "nothing is configured".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11446

Differential Revision: https://secure.phabricator.com/D16386

Increase the storage size for commit summaries

Summary:
Fixes T11453. Currently, commit message summaries are limited to 80 bytes. This may only be 20-40 characters for CJK languages or langauges with Cyrillic script.

Increase storage size to 255, then truncate to the shorter of 255 bytes or 80 glyphs. This preserves the same behavior for latin languages, but is less tight for Russian, etc.

Some minor additional changes:

  • Provide a way to ask "how much data fits in this column?" so we don't have to duplicate column lengths across summary checks or UI errors like "title too long".
  • Remove the text80 datatype, since no other columns use it and we have no use cases (or likely use cases) for it.

Test Plan:

  • Made a commit with a Cyrillic title, saw reasonable summarization in UI:

{F1757522}

  • Added and ran unit tests.
  • Grepped for removed SUMMARY_MAX_LENGTH constant.
  • Grepped for removed text80 data type.

Reviewers: avivey, chad

Reviewed By: avivey

Subscribers: avivey

Maniphest Tasks: T11453

Differential Revision: https://secure.phabricator.com/D16385

Update Herald documentation for modern policies and beahvior

Summary: Ref T11428. This documentation was a bit misleading and out of date. Update it to reflect modern reality.

Test Plan: Read documentation.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11428

Differential Revision: https://secure.phabricator.com/D16384

Link Herald rules to rule detail pages in Herald transcripts

Summary: Fixes T9410. Depends on D16382. Since all users can now view all Herald rules, we can link them in the transcripts.

Test Plan: Viewed a transcript, clicked rule names, reviewed rules.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9410

Differential Revision: https://secure.phabricator.com/D16383

Let everyone view Herald rules

Summary:
Ref T9410. This changes the view policy for all Herald rules to the most public policy ("All Users" for private installs, "Public" for public installs).

See T11428 for discussion of this change in greater detail. In practice, this is approximately how things work today anyway, since you can almost always see almost all of this information in transcripts.

I believe this narrower view policy is helpful in zero cases and slightly confusing or harmful in a number of reasonable cases.

Test Plan: Viewed personal, object and global rules as users who could and could not edit the rules.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9410

Differential Revision: https://secure.phabricator.com/D16382

Convert bin/storage workflow to presume utf8mb4 as the default encoding

Summary: see comments on rP68904d941c54

Test Plan: run ,/bin/storage upgrade

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16370

Remove slight wash on transparent background on logo

Summary: Very slight gradient behind the eye I didn't catch (extra layer).

Test Plan: Remove all layers except logo, re-export.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16381

Force logotext to nowrap

Summary: When using spaces in the logo header, nowrap the text so it properly displays when too long on mobile, tablet.

Test Plan: Snowmen

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16380

Remove PHUIDocumentView

Summary: Converts final call site to PHUIDocumentViewPro.

Test Plan: grep for PHUIDocumentView, view new Welcome Page

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16379

Make new logo and wordmark more reasonably configurable by human users

Summary: Fixes T11437. Provides a normal form for configuring this, instead of weird "look up the PHID and adjust things in the database" stuff.

Test Plan:
{F1753651}

{F1753652}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11437

Differential Revision: https://secure.phabricator.com/D16377

Update Phabricator logo

Summary:
This updates the eye logo and removes the formal wordmark "Phabricator" as an image. Instead we'll use the new updated eye logo and plain text for "Phabricator", both of which are more friendly and less industrial.

Installs that already use the header-logo customization setting will need to rebuild their logo to 80px x 80px. They will then also get to use plain text to whitebox their install as they see fit.

Test Plan:
Tested new logo at desktop, tablet, and mobile sizes. Set a random instance name, saw new wordmark. Created a really long wordmark of MMMMMMMMMMMM, saw text cut off so UI doesn't break. May need some additional tweaking, but I think we covered the most edge cases here.

{F1751791, size=full}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: edibiase, bjshively, yelirekim, Korvin

Maniphest Tasks: T4214, T11096

Differential Revision: https://secure.phabricator.com/D16373

(stable) Promote 2016 Week 32

Add basic Herald support to Calendar

Summary: Fixes T7939. This doesn't get too fancy, but allows you to write Herald rules against Calendar events.

Test Plan:

  • Wrote an "add red flag to events with party in the name" rule.
  • Created a "mundane meeting", didn't get flagged.
  • Created a "cool party", got flagged.
  • Ran rules from the Herald test console.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7939

Differential Revision: https://secure.phabricator.com/D16368

Explain reasoning behind kitty gifts

Summary: I find this fact very useful for understanding my feline companion

Test Plan: Added "Motivator: Cat Facts" to the project, nothing broke

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D16374

Clean up some tablet issues with new nav layouts

Summary: Makes sidenav disappear again on projects/profiles, but shows it on home again (tablet views).

Test Plan: Visit Profile/Projects/Home on mobile, desktop, and tablet. See nav disappear correctly.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16369

Use consistent tag order in Maniphest list views and workboard cards

Summary: Fixes T11420. These are selected in newest-to-oldest order from the database, but we should show them in oldest-to-newest order in the UI.

Test Plan:
Tagged a couple tasks with "A, B, C" projects, saw correct order in UI:

{F1749351}

{F1749352}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11420

Differential Revision: https://secure.phabricator.com/D16367

Make Herald test workflow modular and more clear

Summary:
Fixes T9719. Currently, the Herald "Test Console" has a big instanceof thing, so new adapters (like a Calendar adapter, or third-party adapters) aren't available automatically. Instead, do a standard modular thing: load the available adapters, ask which ones can test the object the user selected, then let the user pick which one they want to move forward with.

Additionally, it isn't very clear that you can't test "commit hook" rules because they rely on push state which we don't really have a good way to simulate. When the user picks a commit, we now show them the "Hook" events, but the options are disabled and explain why they can not be selected.

Test Plan:

  • Ran test rules for revisions, commits, mocks, tasks, wiki documents, questions, and outbound mail.
  • Plugged in a commit, got a more-helpful choice screen explaining why you do a test run of hook rules.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9719

Differential Revision: https://secure.phabricator.com/D16360

Set "history.immutable" to "false" explicitly in .arcconfig

Summary:
If contributors use "history.immutable" by default (for example, in user/global config) and then try to contribute to Phabricator, they might land in a way that creates merge commits, which we'd prefer to avoid.

To make this a bit harder to do unintentionally, set "history.immutable" to "false" explicitly in .arcconfig. This will override any user or system setting and keep land in squash mode in this project.

This might also be a problem in general, and maybe in the long term we need something like one of these warnings:

(when arc landing) You haven't landed changes to this project before, are you sure you want to create a merge commit / are you sure you want to squash?

...or:

(when doing anything) This project doesn't have a history.immutable setting; you should probably set one so user settings don't win out.

...or just not let this be set at the user/system level, although that's annoying in some cases. Also maybe this:

(when arc landing) You're about to create a merge commit, but the last Differential change in history squashed instead. Merge anyway?

However, we haven't seen this as a widespread issue outside of this project yet (and even in this project I think it has only happened 2-3 times), so just put up a guard rail in our own configuration for now.

Test Plan:

  • Ran arc set-config history.immutable true to set the flag in my user settings.
  • Ran arc get-config to verify that the setting overrode system/user settings:
epriestley@orbital ~/dev/phabricator $ arc get-config history.immutable --verbose
history.immutable

    If true, arc will never change repository history (e.g., through amending
    or rebasing). Defaults to true in Mercurial and false in Git. This
    setting has no effect in Subversion.

       Example Value: false

       Current Value: false
      Current Source: project

         local Value: -
       project Value: false
          user Value: true
        system Value: -
       default Value: -

Reviewers: chad, fooishbar

Reviewed By: fooishbar

Differential Revision: https://secure.phabricator.com/D16364

Fix broken link to PHPExcel site

Summary:
phpexcel.net currently serves a 500 page, and the top Google hit for
PHPExcel (on codeplex) gives you a site warning you that it is 3 years
out of date, and to see GitHub instead.

Update the link from Maniphest's 'please install PHPExcel to enable
export' prompt.

Test Plan:

  • Click 'Export to Excel' from Maniphest query result page without PHPExcel installed
  • Follow suggested link to install it
  • See code rather than a HTTP 500 error

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D16363

Fix broken link to PHPExcel site

phpexcel.net currently serves a 500 page, and the top Google hit for
PHPExcel (on codeplex) gives you a site warning you that it is 3 years
out of date, and to see GitHub instead.

Update the link from Maniphest's 'please install PHPExcel to enable
export' prompt.

Fix a documentation typo ("repositorie")

Summary: This isn't a word!

Test Plan: Read carefully.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D16362

Always pre-wrap code blocks

Summary: Fixes T11416. Unclear what the side-effects of this would be, so bark if you find something. Previously, we'd have to overflow and scroll, which is kind of a pain since you're hiding content on long code blocks. This just wraps long lines, and preserves line breaks globally.

Test Plan: Test feed, profile, comments, inline comments, triple backticks.

Reviewers: avivey, epriestley

Reviewed By: avivey, epriestley

Subscribers: Korvin

Maniphest Tasks: T11416

Differential Revision: https://secure.phabricator.com/D16361

Fix Differental Filetree browser hotkey

Summary: Adds a class for explicitly hiding the sidenav.

Test Plan: Set Config to Enable Filetree. View a diff, see tree. Press f, see it go away. Reload page, see persistence.

Reviewers: avivey, epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16359

Redesign Application Search

Summary: This moves aphront-side-nav to use same table css display as profile nav. Slightly less code to support. Cleans up AppSearch UI, think I've gotten all the edge cases here, but bang on it, can hold until after release cut.

Test Plan: Config, Maniphest, Differential, Diffusion, Home.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16346

Use long array syntax for compatibility instead of short array syntax

Summary: Fixes T11409. This syntax isn't compatible with older PHP.

Test Plan: Ran arc lint on the file.

Reviewers: yelirekim, chad

Reviewed By: chad

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T11409

Differential Revision: https://secure.phabricator.com/D16358

Subsume 'harbormaster.querybuilds' with a modern search API method

Summary: We deprecate the existing API method used to access build information from the API, but preserve its response structure after calling through to the new method. I've cordoned off the fields I needed to define in order to meet the output structure by putting those fields in a search attachment.

Test Plan:
Used the API console and looked at the list view controller for builds.

Old output structure:

{
  "data": [
    {
      "id": "16823",
      "phid": "PHID-HMBD-xghrwfz6luoye5rgc2hq",
      "uri": "https://secure.phabricator.com/harbormaster/build/16823/",
      "name": "Run Core Tests",
      "buildablePHID": "PHID-HMBB-s6ykzm2jzxz4ymduztq3",
      "buildPlanPHID": "PHID-HMCP-pcfxcgyoif67l3buc4zt",
      "buildStatus": "passed",
      "buildStatusName": "Passed"
    }
  ],
  "cursor": {
    "limit": 100,
    "after": "16823",
    "before": null
  }
}

New output structure:

{
  "data": [
    {
      "id": 1,
      "type": "HMBD",
      "phid": "PHID-HMBD-qpgcmv67tzaauzayzit5",
      "uri": "http://ec2-54-165-244-168.compute-1.amazonaws.com/harbormaster/build/1/",
      "name": "arc lint + arc unit",
      "buildStatusName": "Passed",
      "buildablePHID": "PHID-HMBB-qdefith5uakkepqpjr2g",
      "buildPlanPHID": "PHID-HMCP-zswbhazb7ipmaf4plygg",
      "buildStatus": "passed",
      "initiatorPHID": "PHID-USER-rihx4366f3aczsvc2wtb",
      "dateCreated": 1450295643,
      "dateModified": 1450295644,
      "policy": {
        "view": "users",
        "edit": "users"
      }
    }
  ],
  "maps": {},
  "query": {
    "queryKey": null
  },
  "cursor": {
    "limit": 100,
    "after": null,
    "before": null,
    "order": null
  }
}

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D16356

Search builds based on who kicked them off

Summary:
It's only natural for users to be interested their own builds. We are also building in support for other sources of builds, the only formally supported way to run a build right now is via Herald.

In our third party codebase, we designate an application as the "thing" that started builds which are scheduled and managed automatically by phabricator. I believe this is a common practice elsewhere in the codebase when you're at a loss for a real human identity and you need to apply some transactions.

Test Plan: Ran some builds manually and saw them show up under the list of things I've run. Looking up builds based on those that had been started by a herald rule.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D16353

Resolve an issue with differential.query if no results are matched

Fixes T11406.

Auditors: chad

Improve performance when constructing custom fields for objects

Summary:
Ref T11404. This improves things by about 10%:

  • Use PhutilClassMapQuery, which has slightly better caching.
  • Do a little less work to generate pretty error messages.
  • Make the "disabled" code a little faster (and sort of clearer, too?) by doing less fancy stuff.

These are pretty minor adjustments and not the sort of optimizations I'd make normally, but this code gets called ~100x (once per revision) and generates ~10 fields normally, so even small savings can amount to something.

(I also want to try to make arc faster in the next update, and improving Conduit performance helps with that.)

Test Plan: Ran differential.revision.search, saw cost drop from ~195ms to ~170ms locally.

Reviewers: yelirekim, chad

Reviewed By: chad

Maniphest Tasks: T11404

Differential Revision: https://secure.phabricator.com/D16355

Remove expensive, pointless typeachecking in custom fields

Summary:
Ref T11404. On my system, this improves performance by 10-15% for differential.revision.search.

PhutilTypeSpec provides high quality typechecking and is great for user-facing things that need good error messages.

However, it's also a bit slow, and pointless here (the API is internal and it only has one possible option).

I think I added this after writing checkMap just because I wanted to use it more often. My desire is sated after finding many reasonable ways to use it to give users high-quality error messages about things like configuration files.

Test Plan: Profiled differential.revision.search before and after change, saw wall time drop from ~220ms to ~195ms.

Reviewers: yelirekim, chad

Reviewed By: chad

Maniphest Tasks: T11404

Differential Revision: https://secure.phabricator.com/D16354

Improve Conduit performance of special edge-based custom Revision fields

Summary:
Ref T11404. Depends on D16351. Currently, both differential.query and differential.revision.search issue 2N queries to fetch:

  • dependencies for each revision; and
  • projects for each revision.

Fix this:

  • Take these custom fields out of Conduit so they don't load this data by default.
  • For differential.query, put this data back in by hard coding it.
  • For differential.revision.search, just leave it out. You can already optionally get projects efficiently, and this endpoint is a work in progress. I would tentatively be inclined to expose graph data as a "graph" extension once we need it.

This makes both methods execute in O(1) time (which is still 20-30 queries, but at least it's not 320 queries anymore).

Test Plan:

  • Ran differential.query, observed no change in results but 199 fewer internal queries.
  • Ran differential.revision.search, observed data gone from results and 200 fewer internal queries.

Reviewers: yelirekim, chad

Reviewed By: chad

Maniphest Tasks: T11404

Differential Revision: https://secure.phabricator.com/D16352

Improve Conduit performance for custom fields

Summary:
Ref T11404. Depends on D16350.

Currently, custom fields can issue "N+1" queries in some cases, so querying 100 revisions issues 100 extra queries.

This affects all *.search endpoints for objects with custom fields, and some older endpoints (notably differential.query).

This change bulk loads "normal" custom fields, which gets rid of some of these queries. Instead of loading fields for each object, we build a big list of all fields and load them all at once.

The next change will tackle the remaining inefficient edge queries.

Test Plan:

  • Configured a custom field with normal database storage in Differential.
  • Ran differential.query, looking at custom fields in results for correctness.
  • Ran differential.revision.search, looking at custom fields in results for correctness.
  • In both cases, observed queries drop from 3N to 2N (all the "normal" custom field stuff got bulk loaded).

Reviewers: yelirekim, chad

Reviewed By: chad

Maniphest Tasks: T11404

Differential Revision: https://secure.phabricator.com/D16351

Allow *.search Conduit API methods to have data bulk-loaded by extensions

Summary:
Ref T11404. Currently, SearchEngineAttachments can bulk-load data but SearchEngineExtensions can not.

This leads to poor performance of custom fields. See T11404 for discussion.

This changes the API to support a bulk load + format pattern like the one Attachments use. The next change will use it to bulk-load custom field data.

Test Plan:

  • Ran differential.query, differential.revision.search as a sanity check.
  • No behavioral changes are expected
  • See next revision.

Reviewers: yelirekim, chad

Reviewed By: chad

Maniphest Tasks: T11404

Differential Revision: https://secure.phabricator.com/D16350

Pick some preset build statuses

Summary:
We're picking three useful groups of build statuses to provide as default queries:

  • Stuff not yet building
  • Stuff building
  • Stuff which has finished building

These are reasonable buckets for builds since (unlike most objects in phabricatorland) users are generally waiting impatiently for the machine to do something for them, rather than being responsible for doing something with the machine.

Test Plan: clicked around the search engine and enjoyed my defaults

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D16349

Move build statuses to a constants class

Summary: No functional changes here, just lifting this out to make room for activities, heeding lint warnings along the way.

Test Plan:
before:

$ grep -Rn "HarbormasterBuild::" *
src/applications/harbormaster/storage/HarbormasterBuildable.php:169:    $build = HarbormasterBuild::initializeNewBuild($viewer)
src/applications/harbormaster/storage/HarbormasterBuildable.php:173:      ->setBuildStatus(HarbormasterBuild::STATUS_PENDING);
src/applications/harbormaster/controller/HarbormasterStepEditController.php:242:    $variables = HarbormasterBuild::getAvailableBuildVariables();
src/applications/harbormaster/controller/HarbormasterBuildableViewController.php:200:        'fa-dot-circle-o '.HarbormasterBuild::getBuildStatusColor($status),
src/applications/harbormaster/controller/HarbormasterBuildableViewController.php:201:        HarbormasterBuild::getBuildStatusName($status));
src/applications/harbormaster/controller/HarbormasterBuildableViewController.php:203:      $item->addAttribute(HarbormasterBuild::getBuildStatusName($status));
src/applications/harbormaster/controller/HarbormasterBuildViewController.php:584:        HarbormasterBuild::getBuildStatusName($status);
src/applications/harbormaster/controller/HarbormasterBuildViewController.php:585:      $icon = HarbormasterBuild::getBuildStatusIcon($status);
src/applications/harbormaster/controller/HarbormasterBuildViewController.php:586:      $color = HarbormasterBuild::getBuildStatusColor($status);
src/applications/harbormaster/event/HarbormasterUIEventListener.php:135:      $status_name = HarbormasterBuild::getBuildStatusName($status);
src/applications/harbormaster/event/HarbormasterUIEventListener.php:136:      $icon = HarbormasterBuild::getBuildStatusIcon($status);
src/applications/harbormaster/event/HarbormasterUIEventListener.php:137:      $color = HarbormasterBuild::getBuildStatusColor($status);
src/applications/harbormaster/conduit/HarbormasterQueryBuildsConduitAPIMethod.php:78:        'buildStatusName' => HarbormasterBuild::getBuildStatusName($status),
src/applications/harbormaster/engine/HarbormasterBuildEngine.php:66:      $build->setBuildStatus(HarbormasterBuild::STATUS_ERROR);
src/applications/harbormaster/engine/HarbormasterBuildEngine.php:109:      $build->setBuildStatus(HarbormasterBuild::STATUS_ABORTED);
src/applications/harbormaster/engine/HarbormasterBuildEngine.php:113:    if (($build->getBuildStatus() == HarbormasterBuild::STATUS_PENDING) ||
src/applications/harbormaster/engine/HarbormasterBuildEngine.php:116:      $build->setBuildStatus(HarbormasterBuild::STATUS_BUILDING);
src/applications/harbormaster/engine/HarbormasterBuildEngine.php:121:      $build->setBuildStatus(HarbormasterBuild::STATUS_BUILDING);
src/applications/harbormaster/engine/HarbormasterBuildEngine.php:126:      $build->setBuildStatus(HarbormasterBuild::STATUS_PAUSED);
src/applications/harbormaster/engine/HarbormasterBuildEngine.php:132:    if ($build->getBuildStatus() == HarbormasterBuild::STATUS_BUILDING) {
src/applications/harbormaster/engine/HarbormasterBuildEngine.php:246:      $build->setBuildStatus(HarbormasterBuild::STATUS_FAILED);
src/applications/harbormaster/engine/HarbormasterBuildEngine.php:254:      $build->setBuildStatus(HarbormasterBuild::STATUS_PASSED);
src/applications/harbormaster/engine/HarbormasterBuildEngine.php:290:      $build->setBuildStatus(HarbormasterBuild::STATUS_DEADLOCKED);
src/applications/harbormaster/engine/HarbormasterBuildEngine.php:446:      if ($build->getBuildStatus() != HarbormasterBuild::STATUS_PASSED) {
src/applications/harbormaster/engine/HarbormasterBuildEngine.php:449:      if ($build->getBuildStatus() == HarbormasterBuild::STATUS_FAILED ||
src/applications/harbormaster/engine/HarbormasterBuildEngine.php:450:          $build->getBuildStatus() == HarbormasterBuild::STATUS_ERROR ||
src/applications/harbormaster/engine/HarbormasterBuildEngine.php:451:          $build->getBuildStatus() == HarbormasterBuild::STATUS_DEADLOCKED) {

after:

$ grep -Rn "HarbormasterBuild::" *
src/applications/harbormaster/storage/HarbormasterBuildable.php:169:    $build = HarbormasterBuild::initializeNewBuild($viewer)
src/applications/harbormaster/controller/HarbormasterStepEditController.php:242:    $variables = HarbormasterBuild::getAvailableBuildVariables();

ran a manual build as a sanity check

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D16348

Provide a basic search engine for builds

Summary:
This supports a few basic use cases that aren't served by the buildable search engine:

  • I'm trying to discover when the last time that this particular build plan failed was.
  • I want to know if any builds have deadlocked.
  • At a glance, I'm more interested in what build plans are running, not which buildables are being built. This is more often than not the case.

Test Plan: {F1744003}

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D16347

(stable) Promote 2016 Week 31

Fix task graph fatal for graphs containing restricted tasks

Summary: Fixes T11392. If some tasks are restricted, we only have PHIDs for them, not objects. Just use the PHIDs instead.

Test Plan: {F1741335}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11392

Differential Revision: https://secure.phabricator.com/D16345

When a task graph has too much stuff, only show adjacent nodes (direct parents/children)

Summary:
Ref T4788. This gives us a new level of graceful degradation, so now we show:

  • Zero through 100 connected tasks: whole graph.
  • More than 100 connnected tasks, but fewer than 100 direct parents/subtasks: just parents and subtasks, with "..." to hint that the graph is cut off.
  • More than 100 parents and children: just the "sorry, too much stuff" error message.

Test Plan: {F1740882}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4788

Differential Revision: https://secure.phabricator.com/D16344

Provide a link to parent/child tasks as a search result from task graphs

Summary:
Ref T4788. Add links to jump to search results with a task's parents or subtasks. This allows relationships to remain useful if there are a zillion of them, and you can sort/filter stuff more easily.

Language might need some tweaking at some point, feeling a little un-brainy today with wordstuff.

Test Plan:
{F1740855}

{F1740856}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4788

Differential Revision: https://secure.phabricator.com/D16343

In Task Graphs, provide a parent/child hint and fix weird strikethrough

Summary:
Fixes T11386. Ref T4788.

  • Apparently fix weird strikethrough effect? Spooky!
  • Provide a little icon hint in the left column about which tasks are direct parents/children, vs just reachable somehow. I don't think this is super useful/important, but seems maybe nice?

Test Plan: {F1740779}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4788, T11386

Differential Revision: https://secure.phabricator.com/D16342

Provide URI/API support for querying subtasks/parents of a particular task

Summary:
Ref T8126. Ref T4788. This adds a way to query by parent or subtask.

I plan to link to this from the task graph (e.g., ViewSearch Subtasks or similar, in a dropdown on the "Task Graph" element) as a way to let us bail out if tasks have 300 subtasks and send the user to a big query result list. That'll give us more flexibility to tailor the UI for reasonable numbers of tasks.

There's no UI for this unless you specify a query yourself, so the only ways to get to it are:

  • Manually put ?parentIDs=... into the URI.
  • Use the API.
  • Future link from task graphs.

It doesn't seem too useful to me on its own, outside of the context of links from tasks.

Test Plan:

  • Manually put ?parentIDs=... and ?subtaskIDs=... into Maniphest query UI, got expected results.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4788, T8126

Differential Revision: https://secure.phabricator.com/D16341

Fix "Blocked" task queries with multiple subtasks, and update language

Summary:
Ref T8126. See that task for discussion. This change:

  • Updates language to be more consistent ("Parents", "Subtasks") since I moved us away from the often-confusing "Block" language in T4788.
  • Fixes bugs with finding the wrong set of tasks if tasks have a mixture of open and closed subtasks or parents.

Test Plan:

  • Created four tasks: no subtasks, one closed subtask, one open subtask, mixture of open and closed subtasks.
  • Created four more tasks: no parents, one closed parent, one open parent, mixture of open and closed parents.
  • Searched for all this stuff, got the proper results:

{F1740683}

{F1740684}

{F1740685}

{F1740686}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8126

Differential Revision: https://secure.phabricator.com/D16340

Clean up recurring event information on Calendar events

Summary: Ref T11326. This adds prev/next links for recurring events (ala D16179) and moves the "accept/decline" buttons closer to the invite list. This might need some fiddling, but should be a little more human-friendly.

Test Plan: {F1740541}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11326

Differential Revision: https://secure.phabricator.com/D16339

In Packages, give publishers a list of packages and packages a list of versions

Summary: Ref T8116. Puts a list of packages on the publisher page, and a list of versions on the package page.

Test Plan: Viewed a publisher, saw packages. Viewed a package, saw versions. Looked at list views.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8116

Differential Revision: https://secure.phabricator.com/D16321

Add basic search capabilities to Packages

Summary: Ref T8116. Add search-by-name and per-package / per-publisher search to Packages.

Test Plan: Searched publishers, packages, versions by name. Searched packages by publisher. Searched versions by package.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8116

Differential Revision: https://secure.phabricator.com/D16320

Add default create, view, edit capabilities to Packages

Summary:
Ref T8116. This adds a control for creating publishers (default: administrators) and default publisher/package edit controls.

I've left the edit defaults at "no one" for now to force you to select a policy. This might be something to look at later.

Test Plan: Created publishers, packages. Tried to create publishers with "can create" policy set restrictively.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8116

Differential Revision: https://secure.phabricator.com/D16319

Add PackagesVersion

Summary:
Ref T8116. A version has:

  • a package (like "Arcanist") which it belongs to;
  • a name (like "v3.1.5").

The name is immutable and unique, like the package key and publisher key.

Policy stuff:

  • Versions have the exact same policies as their packages.
  • You must be able to edit a package to create new versions of it.

This is still entirely uninteresting.

Test Plan: {F1731703}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8116

Differential Revision: https://secure.phabricator.com/D16316

Add PackagesPackage

Summary:
Ref T8116. A package has:

  • a publisher (like "Phacility"), from the previous revision;
  • a name (like "Arcanist");
  • a package key (like "arcanist").

The package key is immutable, like the publisher key.

This gives a package a full key like "phacility/arcanist".

Policy stuff:

  • You must be able to view a publisher to view a package (currently, everyone can always see all publishers).
  • You must be able to edit a publisher to create a new package inside it.
  • Packages have separate view/edit permissions.

This still does nothing interesting.

Test Plan: {F1731663}

Reviewers: chad

Reviewed By: chad

Subscribers: eadler

Maniphest Tasks: T8116

Differential Revision: https://secure.phabricator.com/D16315

Add a Packages application and PackagePublisher

Summary:
Ref T8116. Partially scavenged from D14152. This roughs in a new Packages application for Arcanist extensions and third-party applications, and adds a "Publisher" object.

A "Publisher" represents an individual or entity who is publishing a package, like "Phacility". It's explicitly not necessarily the original author -- just the primary entity vouching for the safety of the code.

A publisher just has a name and a unique key for now. For example, Phacility might have "Phacility" and "phacility", respectively.

Unique keys are immutable, e.g., the package "phacility/arcanist" will always be exactly the same package by exactly the same publisher.

Test Plan: {F1731621}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8116

Differential Revision: https://secure.phabricator.com/D16314

Provide better headers and crumbs for Calendar result views

Summary:
Ref T11326. This isn't perfect, but should be a little easier to use and less weird/confusing.

Generally, provide a "Query > Month > Day" crumb on day views, and a "Wed, July 3" header.

Generally, provide a "Query > Month" crumb on month views, and a "July 2019" header.

Also try to fix a bit of padding/spacing on the day view.

Test Plan: {F1739128}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11326

Differential Revision: https://secure.phabricator.com/D16338

When a Calendar day has too many events, show a "More..." link

Summary: Fixes T8361.

Test Plan: {F1739073}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8361

Differential Revision: https://secure.phabricator.com/D16337

Give Calendar days from adjacent months a background color hint

Summary:
Ref T11326. When viewing "February", add a class to dates in January and March to let them be styled a little differently as a UI hint.

For now, I've given them a grey background. (Calendar.app changes the date number color instead.)

Test Plan: {F1738990}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11326

Differential Revision: https://secure.phabricator.com/D16336

Remove extra margins on Calendar month view

Summary:
Ref T11326. This doesn't go quite as far as the mock in T11326#185932, but gets rid of the easy margins.

Also cleans up some of the border rules so they're simpler and more consistent (no weird ragged edges on the far right).

Test Plan: {F1738951}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11326

Differential Revision: https://secure.phabricator.com/D16335

Give the Calendar month view a nice hover effect

Summary:
Ref T11326. Currently, we link Calendar days using hidden DOM nodes.

This is nice because it's simple, and right-clicking a day works properly. However, it's a bit ugly/unintuitive, messy, and unclear. It's especially messy because days are really two different rows, one for events and one for day/week numbers.

Instead, use JS to highlight day cells. You can still right-click by clicking the actual day number, which seems like a reasonable compromise.

Test Plan: {F1738941}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11326

Differential Revision: https://secure.phabricator.com/D16334

Cruncy legumes.

Auditors: chad

Hide time controls when editing all-day Calendar events

Summary:
Ref T11326. When an event is all-day, hide the time controls for the start/end dates. These aren't used and aren't helpful/useful.

This got a little more complicated than it used to be because EditEngine forms may have only some of these controls present.

Test Plan: Edited an all-day event; edited a normal event; swapped an event between normal and all-day.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11326

Differential Revision: https://secure.phabricator.com/D16327

Clean up mobile spacing on info views in a column

Summary: When we have an info view in a column, the css isn't specific enough to override the core info-view css.

Test Plan: Review an importing repository, see info view properly spaced.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16331

Clean up spacing on mobile form instructions

Summary: This padding is a little off / custom. Normalizes it to the form on mobile.

Test Plan: Review some settings forms, save form changes.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16330

Clean up mobile crumb padding

Summary: These feel mis-aligned now. Equalizes paddding left and right.

Test Plan: Review a task create, workboards.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16328

Roomier forms on mobile

Summary: Mobile forms are super tight, this opens them up a little bit.

Test Plan: Review editing a document, task, mobile, tablet.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16329

Correct an issue with epoch timestamps in Conduit

Summary:
Fixes T11375. Some validation code was mishandling raw epoch timestamps.

For numeric values larger than 29999999 (e.g., 2999-12-25, christmas 2999), assume the value is a timestamp.

Test Plan: Used maniphest.search to query for modifiedStart, got a better result set and saw the dateModified constraint in the query.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11375

Differential Revision: https://secure.phabricator.com/D16326

Use numeric input control for TOTP factor entry

Summary:
Finishes fixing T11365. rP28199bcb48 added the new numeric entry
control and used it for TOTP setup, but missed the case of entering
a factor when TOTP was already set up.

Test Plan:
Observe behaviour of TOTP setup and subsequent factor entry
in iOS browser, make sure they're consistent.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Maniphest Tasks: T11365

Differential Revision: https://secure.phabricator.com/D16325

Add a numeric input control for TOTP codes

Summary:
Fixes T11365. I tested these variants:

  • <input type="number" />
  • <input type="text" pattern="\d*" />

Of these, this one (using pattern) appears to have the best behavior: it shows the correct keyboard on iOS mobile and does nothing on desktops.

Using type="number" causes unwanted sub-controls to appear in desktop Safari, and a numbers + symbols keyboard to appear on iOS (presumably so users can type "." and "-" and maybe ",").

Test Plan: Tested variants in desktop browsers and iOS simulator, see here and T11365 for discussion.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11365

Differential Revision: https://secure.phabricator.com/D16323

Re-implement calendar.invite transactions

Summary:
Fix T11339.
Now, old and new are both simple lists of phids, and the rendering should make sense.

Test Plan: Viewed existing transaction with all 3 states.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T11339

Differential Revision: https://secure.phabricator.com/D16311

Blanket reject request which may have been poisoned by a "Proxy" header to mitigate the httpoxy vulnerability

Summary:
See accompanying discussion in T11359.

As far as I can tell we aren't vulnerable, but subprocesses could be (now, or in the future). Reject any request which may have a Proxy: header.

This will also do a false-positive reject if HTTP_PROXY is defined in the environment, but this is likely a misconfiguration (cURL does not read it). I'll provide guidance on this.

Test Plan:

  • Made requests using curl -H Proxy:..., got rejected.
  • Made normal requests, got normal pages.

Reviewers: chad, avivey

Reviewed By: avivey

Differential Revision: https://secure.phabricator.com/D16318

bin/storage shell: force TCP

Summary:
mysql has the magic feature of ignoring port arguments and using the socket when connecting to localhost.

This flag makes it not do that.

Test Plan: ./bin/storage shell, execute status, see Connection: localhost via TCP/IP.

Reviewers: joshuaspence, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16317

Raise human-readable error messages for overlong Phame blog titles and subtitles

Summary:
Fixes T11358. Entering a too-long title/subtitle currently raises an unfriendly (database-level) error.

Raise a friendlier error.

Test Plan: {F1731533}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11358

Differential Revision: https://secure.phabricator.com/D16313

Allow file comments to be edited

Summary:
Fixes T10750. Files have some outdated cache/key code which prevents recording an edit history on file comments.

Remove this ancient cruft.

(Users must bin/storage adjust after upgrading to this patch to reap the benefits.)

Test Plan:

  • Ran bin/storage adjust.
  • Edited a comment in Files.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10750

Differential Revision: https://secure.phabricator.com/D16312

Add crumbs to Calendar events to return to the month/day view

Summary: Ref T11326. This makes it a little easier to jump back up to check out your day.

Test Plan: {F1725575}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11326

Differential Revision: https://secure.phabricator.com/D16309

Make Calendar day view a little more consistent

Summary: Ref T11326. This just cleans things up a little and removes some of the obvious layout/CSS issues.

Test Plan:

  • Viewed day view before/after. Also viewed profile panel.

Before:

{F1725547}

After:

{F1725548}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11326

Differential Revision: https://secure.phabricator.com/D16308

discouarges → discourages

Test Plan: Read again the sentence.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D16307

Make Calendar month view events render a little more consistently

Summary:
Ref T11326. This just inches things forward a little bit:

  • Make it easier to see current day.
  • Line-through cancelled events.
  • Don't colorize the whole event title, just use an Attending/Invited/Custom icon.
  • Slightly subtler treatment for all-day events.

Test Plan: See screenshot in T11326.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11326

Differential Revision: https://secure.phabricator.com/D16306

Store "All Day" events in a way that is compatible with EditEngine

Summary:
Ref T11326. Normally, events occur at a specific epoch, independent of the viewer. For example, if we're having a meeting in 35 hours, every user who looks at the event will see that it starts 35 hours from now.

But when an event is "All Day", the start time and end time depend on the viewer. A day like "Christmas" does not start at the same time for everyone: it starts sooner if you're in a more-eastern timezone. Baiscally, an event on "July 15th" starts whenever "July 15th" starts for whoever is looking at it.

Previously, we stored these events by using the western-most and eastern-most timezones as the start and end times (the earliest possible start and latest possible end).

This worked OK, but we get into a bunch of trouble with EditEngine, mostly because each field can be updated individually now. We can't easily tell if an event is all-day or not when reading or updating the start time and end time, and making that easier would introduce a huge amount of complexity.

Instead, when we update the start or end time, we write two times:

  • The epoch timestamp of the time the user entered, which is the start time we will use if the event is a normal event.
  • The epoch timestamp of 12:00 AM in UTC on the same date as the local date the user entered. This is pretty much like just storing the date the user actually typed. This is what w'ell use if the event is an all-day event.

Then, no matter whether the event is later made all-day or not, we have all the information we need to display it correctly.

Test Plan:

  • Created and edited all-day events.
  • Migrated existing all-day events, which appeared to survive without problems. (Note that events all-day which were created or edited in the last couple of days master won't survive this mutation correctly and will need to be fixed.)
  • Created and edited normal, recurring, and recurring all-day events.
  • Swapped back to stable, created an event, specifically migrated it forward, made sure it survived with times intact.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11326

Differential Revision: https://secure.phabricator.com/D16305

Fix some Calendar Event userPHID/hostPHID/"Creator" confusion in searching

Summary: Ref T11326. Align this stuff with "Host" and "hostPHID".

Test Plan: Searched for events by host.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11326

Differential Revision: https://secure.phabricator.com/D16303

Remove old paged forms

Summary:
Ref T11326. This gets rid of the old multi-paged form stuff used in the last version of Diffusion.

This incidentally removes a callsite for a date control to make it a little easier to simplify them.

Test Plan: Grepped for all removed classes, no more callsites.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11326

Differential Revision: https://secure.phabricator.com/D16302

Remove two unused SearchEngine methods

Summary:
Ref T11326. These are last-generation and neither of these have callsites anymore.

(I nuked these since I'm trying to simplify date handling.)

Test Plan: Grepped for callsites.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11326

Differential Revision: https://secure.phabricator.com/D16301

Make Calendar Event handles slightly more modern

Summary: Ref T11326. Use modern methods instead of building this stuff separately.

Test Plan: Used E123, {E123}, saw references render normally.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11326

Differential Revision: https://secure.phabricator.com/D16300

Make upcoming events view more viewer-oriented

Summary:
Ref T11326. Try to make this a little more useful:

  • Don't show entire attendee list (not useful?)
  • Show host (useful?)
  • Show your own status prominently (attending vs declined vs invited).
  • Show cancelled events prominently.

Test Plan: {F1723550}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11326

Differential Revision: https://secure.phabricator.com/D16299

Move event host and times into event subheader

Summary:
Ref T11326. Show this information with a subheader instead of in properties.

Also, slightly simplify the list view.

Test Plan: {F1723539}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11326

Differential Revision: https://secure.phabricator.com/D16298

Show event icons in header instead of property list

Summary:
Ref T11326. Currently, events show the icon as a property, like this:

Icon: Default

This is boring and terrible. Show the icon in the header instead:

{F1723530}

Also minor cleanup on active/cancel states.

Test Plan: Viewed an event, saw icon.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11326

Differential Revision: https://secure.phabricator.com/D16297

Make ghost event instances render better on the event list

Summary:
Ref T11326. Currently, we render "E (99)" for ghost instances, which is meaningless and inconsistent.

Render these more sensibly and consistently.

Test Plan: Viewed event list, saw reasonable monograms / object names.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11326

Differential Revision: https://secure.phabricator.com/D16296

For now, hard limit task graph at 100 nodes

Summary:
Ref T4788. One install has some particularly impressive task graphs which are thousands of nodes large.

The current graph is pretty broken in these cases. For now, just render a "too big to show" message. In the future, I'd plan to finesse this (e.g., show parents/children, show links to parents/children, etc).

Test Plan:

  • Viewed a normal task.
  • Set limit to 3, viewed a task with graph size 6, saw an error message.
  • Viewed a revision stack graph (unaffected).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4788

Differential Revision: https://secure.phabricator.com/D16295

Provide default view and edit policies in Calendar, plus "Event Host" and "Event Invitees"

Summary:
Fixes T9224. This adds:

  • A "Default Edit Policy" and "Default View Policy" to Calendar, similar to other applications.
  • "Event Host" and "Event Invitees" objects policies.

These policies often end up being redundant (the host can always view/edit, the invitees can always view), but they can be more clear than setting "No One", and "Editable By: Event Invitees" is a legitimately useful policy.

Test Plan:

  • Created and edited events.
  • Fiddled with defaults.
  • Tried to remove myself as the event host for an "Editable By: Host" event, got an error ("you wouldn't be able to edit").
  • Tried to remove myself as host/invitee for an "Editable By: Invitees" event, got an error.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9224

Differential Revision: https://secure.phabricator.com/D16294

Slightly simplify some Paste modular transactions

Summary: Modular transactions have slightly more modern ways to express values now.

Test Plan: Looked at transaction record of a paste.

Reviewers: chad, avivey

Reviewed By: avivey

Differential Revision: https://secure.phabricator.com/D16293

Make event hosts editable

Summary:
Fixes T10909. I think this is a generally reasonable sort of capability to expose, although I've made it edit-only for now (when creating an event, you're always the host).

Also clean up some minor leftovers in the code, and a couple of little bugs with recurrence frequencies.

Test Plan: Created an event, edited the host of an event.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10909

Differential Revision: https://secure.phabricator.com/D16292

Rename Event "userPHID" to "hostPHID"

Summary: Ref T10909. Ref T9224. We label this field "Host" in the UI; make the storage format consistent.

Test Plan:

  • Viewed month view, day view, detail view of an event.
  • Created a new event, saw myself as the host.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9224, T10909

Differential Revision: https://secure.phabricator.com/D16291

Respect 12 hour vs 24 hour time formats in Calendar day views

Summary: Fixes T9202.

Test Plan:

  • Viewed day in 12-hour, saw "8:00 PM".
  • Viewed day in 24-hour, saw "16:00".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9202, T10932

Differential Revision: https://secure.phabricator.com/D16290

Make limits and ranges work better with Calendar event queries

Summary:
Fixes T8911. This corrects several issues which could crop up if a calendar event query matched more results than the query limit:

  • The desired order was not applied by the SearchEngine -- it applies the first builtin order instead. Provide a proper builtin order.
  • When we generate ghosts, we can't do limiting in the database because we may select and then immediately discard a large number of parent events which are outside of the query range.
    • For now, just don't limit results to get the behavior correct.
    • This may need to be refined eventually to improve performance.
  • When trimming events, we could trim parents and fail to generate ghosts from them. Separate parent events out first.
  • Try to simplify some logic.

Test Plan: An "Upcoming" dashboard panel with limit 10 and the main Calendar "Upcoming Events" UI now show the same results.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8911

Differential Revision: https://secure.phabricator.com/D16289

Add calendar.event.search and calendar.event.edit

Summary: Ref T7944. The search method is a bit bare-bones for now, but these substantially work.

Test Plan: Edited events via API; queried events via API.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7944

Differential Revision: https://secure.phabricator.com/D16288

In email, render dates with an explicit timezone offset

Summary:
Fixes T10633. When generating email about a transaction which adjusts a date, render the offset explicitly (like "UTC-7").

This makes it more clear in cases like this:

  • mail is being sent to multiple users, and not necessarily using the viewer's settings;
  • you get some mail while travelling and aren't sure which timezone setting it generated under.

Test Plan: Rendered in text mode, saw UTC offset.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10633

Differential Revision: https://secure.phabricator.com/D16287

Convert Calendar to Modular Transactions

Summary:
Ref T9275. Swaps Calendar over to modular transactions. Theoretically, this has almost no effect on anything.

Ref T10633. I didn't actually do anything here yet, but this gets us ready to put timestamps in email.

Test Plan: Created and edited a bunch of events, nothing seemed catastrophically broken.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9275, T10633

Differential Revision: https://secure.phabricator.com/D16286

Restore date validation errors to Calendar

Summary: Ref T9275. I waffled back and forth on these transactions a bit, but put these back here in better working order.

Test Plan: Tried to schedule an event on "taco".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9275

Differential Revision: https://secure.phabricator.com/D16285

Fix some EditEngine issues with rendering "invite" transactions

Summary:
Ref T9275. We were rendering too many transactions and/or over-rendering invitees.

Clean this logic up a bit:

  • List all before/after invitees.
  • Simplify the lists before rendering.

Test Plan: Viewed an event, edited invitees, got sensible human-readable transactions.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9275

Differential Revision: https://secure.phabricator.com/D16284

Switch Calendar to EditEngine

Summary:
Ref T9275. This throws away the old EditController and switches fully to EditEngine.

There's still some sketchy behavior (particularly, no JS stuff yet) but I think all the basics work properly.

Test Plan: Created and edited events via EditEngine, everything seemed to work alright.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9275

Differential Revision: https://secure.phabricator.com/D16283

Smooth over a few more transaction compatibility/structure issues with Calendar events

Summary: Ref T9275. This gets things roughly into shape for a cutover to EditEngine, mostly by fixing some problems with "recurrence end date" not being nullable while editing events.

Test Plan: Edited events with EditPro controller, nothing was obviously broken.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9275

Differential Revision: https://secure.phabricator.com/D16282

Make EditEngine form for Calendar Events almost fully-functional

Summary:
Ref T9275. This still has a number of rough edges and other minor problems (no JS on the controls, some date handling control bugs) but I'll smooth those over in future changes.

It does make all the editable transaction types available from EditEngine, technically speaking.

Test Plan: Created and edited events with the "pro" controller, which mostly worked.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9275

Differential Revision: https://secure.phabricator.com/D16281

Simplify the TYPE_INVITE Calendar Event transaction for EditEngine

Summary:
Ref T9275. Now that TYPE_ACCEPT and TYPE_DECLINE have been separated out, we can simplify TYPE_INVITE.

This now just takes a list of invited PHIDs, uninvites ones that were removed and invites ones that were added. This is simpler, lets more logic live in the Editor, and makes EditEngine/API access easier.

Test Plan: Created events, added and removed invitees. Used comment stacked action and "pro" editor to adjust invitees.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9275

Differential Revision: https://secure.phabricator.com/D16280

Prepare event dates for EditEngine/API

Summary:
Ref T9275. Currently, the "Start Date", "End Date", and "Recurrence End Date" transcations take a complex value (AphrontFormDateControlValue) and reduce it to an epoch.

Do this a little earlier, since the API will be much more usable if it just passes in epoch timestamps.

Events also have some logic where they rewrite the from date and to date on the actual object for all day events, then undo the changes later. Specifically, if you have an all-day event on "July 24th", the exact start and end times vary based on who is looking at it. Instead of overwriting the persistent dateFrom and dateTo properties, add separate viewer properties to make it easier to keep this stuff straight.

Since this means all-day events get stored in UTC, we need to query/fetch (and then discard) slightly more events. This is perfectly and much simpler to do.

The one weird "UTC" hack in here will get nuked when this moves to EditEngine properly.

Test Plan: Edited times for normal events and all-day events.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9275

Differential Revision: https://secure.phabricator.com/D16274

Create separate "Accept" and "Decline" transactions for Calendar

Summary:
Ref T9275. Currently, there's a single "invite" transaction type for managing Calendar invites, and it takes a map of invitees to status.

This isn't great for EditEngine or API access, since it lets you set anyone else to any status and we can't reuse as much code as we can with a simpler API.

Make "Accept" and "Decline" separate actions which affect the actor's invite, so "invite" can be a simpler transaction which just invites or uninvites people.

Test Plan:

  • Joined/accepted/declined an event invitation.
  • Edited event invitees.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9275

Differential Revision: https://secure.phabricator.com/D16272

Move more event fields to EditEngine

Summary:
Ref T9275. This moves description, icon, and cancel/uncancel to EditEngine.

It removes TYPE_SEQUENCE_INDEX and TYPE_INSTANCE_OF_EVENT. These are currently never generated and I do not expect to genereate them (instead, these changes happen automatically when you edit a stub).

Test Plan: Edited an event with normal and pro edit forms.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9275

Differential Revision: https://secure.phabricator.com/D16264

Generate "stub" events earlier, so more infrastructure works with Calendar

Summary:
Ref T9275. When you create a recurring event which recurs forever, we want to avoid writing an infinite number of rows to the database.

Currently, we write a row to the database right before you edit the event. Until then, we refer to it as E123/999 or whatever ("instance 999 of event 123").

This creates a big mess with trying to make recurring events work with EditEngine, Subscriptions, Projects, Flags, Tokens, etc -- all of this stuff assumes that whatever you're working with has a PHID.

I poked at letting this stuff work without a PHID a little bit, but that looked like a gigantic mess.

Instead, generate an event "stub" a little sooner (when you look at the event detail page). This is basically just an ID/PHID to refer to the instance.

Then, when you edit the stub, "materialize" it into a real event.

This still has some issues, but I think it's more promising than the other approach was.

Also:

  • Removes dead user profile calendar controller.
  • Replaces comments with EditEngine comments.

Test Plan:

  • Commented on a recurring event.
  • Awarded tokens to a recurring event.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9275

Differential Revision: https://secure.phabricator.com/D16248

Initial cut of CalendarEditEngine

Summary:
Ref T9275. This builds a Calendar EditEngine which only edits "name".

I'll add more fields, Conduit, etc., and move to modular transactions in future changes.

Test Plan: Used editpro/ URI manually to edit the name of an event.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9275

Differential Revision: https://secure.phabricator.com/D16235

Simplify "builtin file" management and recover from races

Summary:
Fixes T11307. Fixes T8124. Currently, builtin files are tracked by using a special transform with an invalid source ID.

Just use a dedicated column instead. The transform thing is too clever/weird/hacky and exposes us to issues with the "file" and "transform" tables getting out of sync (possibly the issue in T11307?) and with race conditions.

Test Plan:

  • Loaded profile "edit picture" page, saw builtins.
  • Deleted all builtin files, put 3 second sleep in the storage engine write, loaded profile page in two windows.
    • Before patch: one of them failed with a race.
    • After patch: both of them loaded.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8124, T11307

Differential Revision: https://secure.phabricator.com/D16271

When users choose a default project icon, make a permanent file

Summary:
Fixes T10907. As written, this workflow will incorrectly reuse a temporary file if one exists.

Instead, make a new permanent file.

(Storage is still shared, so this usually will not actually create a copy of the file's data.)

Test Plan:

  • Set a project's icon by clicking first button in "Use Picture" row.
  • Before patch: temporary image was reused.
  • After patch: new permanent file is generated.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10907

Differential Revision: https://secure.phabricator.com/D16270

Ignore unreachable commits when testing if a repository has imported

Summary:
Fixes T11309. When checking if a repository was fully imported, we incorrectly allow unreachable, un-imported commits to prevent the repository from moving to "Imported".

This can happen if you delete branches from a repository while it is importing.

Instead, ignore unreachable commits when checking for remaining imports, and when reporting status via bin/repository importing.

Test Plan:

  • Stopped daemons.
  • Created a new repository and activated it.
  • Ran bin/repository update Rxx.
  • Deleted a branch in the repository.
  • Ran bin/repository update Rxx.
  • Ran daemons to flush queue.

Now:

  • Ran bin/repository importing. Old behavior: showed unreachable commits as importing. New behavior: does not show unreachable commits.
  • Ran bin/repository update. Old behavior: failed to move repository to "imported" status. New behavior: correctly moves repository to "imported" status.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11309

Differential Revision: https://secure.phabricator.com/D16269

Make permanent worker failures more user-friendly

Summary:
Ref T11309. In that task, a user misunderstood two parts of this error:

  • They took "exception" to mean "unexpected failure", when it was intended to mean "rare circumstance".
  • They intereted the internal ID number of a commit to mean that Phabricator was malfunctioning.

Make the language of this condition more direct, explaining what the situation means in greater detail.

Additionally, we would previously re-throw this exception, which would make the daemon exit, wait a moment, and restart. This was normal and expected.

When unexpected failures occur, it's important do to this: it prevents a daemon failing in a loop from causing too many side effects (e.g., limit of 1 email per 5 seconds instead of thousands per second).

When expected, permanent failures occur, we do not need to do this: the task will not be retried. I just did it because it was slightly more consistent ("failures restart daemons") and we had few permanent failure types at the time.

We have more now, and restarting the daemons generates some additional logs which have the potential to confuse. Cycling the daemon also (intentionally) reduces the rate at which we process tasks, which can be bad for permanent failures like "deleted commit" because users can delete a huge number of commits and possibly clog up the queue with cycle-after-failure actions.

Test Plan:
Tried to process a deleted commit, saw a new message:

2016-07-11 9:30:22 AM [STDE] <VERB> PhabricatorTaskmasterDaemon Task 1428658 was cancelled: Commit "R55:6c46b7d0fb82a859ca3f87a95dc8dcceef8088c9" (with internal ID "282161") is no longer reachable from any branch, tag, or ref in this repository, so it will not be imported. This usually means that the branch the commit was on was deleted or overwritten.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11309

Differential Revision: https://secure.phabricator.com/D16268

Allow worker tasks to be cancelled by classname

Summary:
Ref T3554. Makes bin/worker cancel --class <classname> work (cancel all tasks with that type).

This is useful in development if your queue is full of a bunch of gunk, and a need has occasionally arisen in production environments (usually "one option is cancel everything and move on").

Test Plan: Ran bin/worker cancel to cancel blocks of tasks by class name.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T3554

Differential Revision: https://secure.phabricator.com/D16267

By default, do not save queries when executing Conduit "*.search" calls

Summary:
Fixes T11304. Prior to this change, we did an unnecessary write on every "*.search" call (this write didn't always actually write a row, since we only save unique saved queries, but still doesn't do anything useful ever, currently).

Instead, change this to not-write by default. We could add an "oh, and also I want you to do a write" option later, which would let us implement something like arc query-stuff which says "To see more results, view this URI in your browser: ...".

(It's possible to run one of these methods with an existing SavedQuery by using the key, so we still sometimes have a queryKey to return.)

Test Plan: Ran almanac.service.search, used DarkConsole to verify that no serachengine writes occurred.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11304

Differential Revision: https://secure.phabricator.com/D16263

In Git, only use "--find-copies-harder" on small diffs

Summary:
Ref T10423. This flag can cause git diff to take an enormously long time (the problem case was a 5M line, 20K file commit).

Instead:

  • Run without the flag first.
  • If that shows that the diff is definitely small, try again with the flag.
  • If that works, return the slower, better output.
  • If the fast diff affects too many paths or generating the slow diff takes too long, return the faster, slightly worse output.

The quality of the output differs in how well Git is able to detect "M" and "C" (moves and copies of files).

For example, if you copy src/ to srcpro/, the fast output may not show that you copied files. The slow output will.

I think this is rarely useful for large copies anyway: it's interesting if a 1-2 file diff is a copy, but usually obvious/uninteresting if a 500-file diff is a copy.

Test Plan:

  • Ran bin/repository reparse --change rXnnn on Git changes.
  • Saw fast and slow commands execute normally.
  • Tried on a large diff, saw only the fast command execute.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10423

Differential Revision: https://secure.phabricator.com/D16266

Fix mobile menu for files in Differential Changesets

Summary: Fixes T11305, Ref T7754. Makes this menu dropdown act like actions and collapse to a fa-bars menu.

Test Plan:
View on mobile, desktop, browser. Click an action, spawn new page.

{F1717953}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T7754, T11305

Differential Revision: https://secure.phabricator.com/D16265

Make links to hidden comments work (Fixes T11117)

Summary: This Fixes T11117.

Test Plan:

  • Ran bin/celerity map
  • Followed STR in T11117.
  • Now it works.

Reviewers: epriestley, #blessed_reviewers, chad

Reviewed By: epriestley, #blessed_reviewers, chad

Subscribers: chad, Korvin

Maniphest Tasks: T11117

Differential Revision: https://secure.phabricator.com/D16256

Fix an issue with creating new Repository URIs via the Web UI

Summary I broke this in D16237: that made the CLI workflow work, but we attach the repository earlier in the web workflow and won't have one when we arrive here.

Test Plan: Created a new repository URI from the web UI.

Auditors: chad

Make links to hidden comments work (Fixes T11117)

Details

Committed
Gerrit Code Review <gerrit@wikimedia.org>Sep 7 2016, 10:16 PM
Reviewer
epriestley
Parents
rPHDEP8f513f52e9e3: Updated git submodules
Branches
Unknown
Tags
Unknown
ChangeId
Ie394225525d1e4805d46c517fd1b2cbeaf3de8ec

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Gerrit Code Review <gerrit@wikimedia.org> committed rPHDEP81dc55d04fec: Updated git submodules (authored by mmodell).Sep 7 2016, 10:16 PM
mmodell added a task: Restricted Task.
mmodell added a task: T7939: mysql.so.
mmodell removed tasks: T4788: SpecialPreferences: If $wgAllowRealName is false, then inputbox and help text still show Email superscript-"2" (cosmetic), T3554: Interlink containing letter "ş" is broken, T5315: Rollback button on history page, T7148: Image pushes edit links down, T6996: Update request portals wikipedia.be and wikipedia.nl, T7754: Creation of a new page should never be a minor edit, T7924: Wrong redirection from "Wikipedia:" namespace on "w:fa:" to "w:en:", T7944: Set logo for the Chinese Wikiquote, T7939: mysql.so, T8116: history feed for not existing pages is not properly handled, T8124: Add interwiki links for nl.wikimedia.org, T8126: No license selector box appears at Special:Upload on Commons if MediaWiki:Licenses/XX is not defined, T8361: Enable subpages in Hungarian Wikisource, T8628: Special page to edit interwiki table, T9117: Set up interwiki links for Beta Wikiversity, T8911: {{CURRENTMONTHNAMEGEN}} gives wrong output for August in English, T8850: "Save Page" or "Show Preview" buttons in Firefox, T9202: MediaWiki:Monobook.js gets loaded by other skins, T9224: In 1.7.1, "unknown datatype" exception thrown by Hooks.php for ParserClearState, T9275: History for currently selected Expression not shown on WiktionaryZ, T9235: Update for Cantonese language (zh-yue) #2, T9690: Request for new namespaces on ru.Wiktionary, T9410: Namespace prefixes show up in Category search, T9719: Wildcardsearch is not functional, T10423: realpath, commandLine.inc, T9982: Logo for Old Slavonic Wikipedia, T10633: Use geo microformat when displaying gps coordinates, Restricted Task, T10750: Problem with Special:BrokenRedirects on fawiki, T10907: Warn when uploading files with no extension, T10867: Hook to alter upload form, T10909: List of links "linked to page", T10951: Allow users to specify file categories when uploading, T10999: User.php gives "undefined user editcount" PHP notice, T11117: add a link to [[Special:Undelete]] in [[Special:Log/delete]], T11304: New namespace for lists and tables., T11305: Rollback buttons in Special:Recentchanges and Special:Watchlist and Special:RecentChangesLinked, T11132: Uploading new images sets mime type to application/x-archive and dimensions to 0x0, T11309: Whitelist external links to toolserver in captcha configuration, T11307: Set logo for Chechen Wikipedia, T11326: Filter Special:Protectedpages by namespace, T11358: Missing escape of single quote generates error, T11365: segfault on /index.php after installation, T11375: Generate automatic edit summaries for addition of short bits of text to pages, T11386: Run maintenance/updateRestrictions.php on all wikis, T11406: Captcha form should be set off visually on edit page, T11392: Russian translations, T11404: LinkSearch: Remove restriction that full hostname be entered into search, T11416: Spam blacklist match should show error above edit box during preview, T11409: Image frame width in units other than pixels, T11428: <pre> doesn't nest, T11420: textPath is not supported by rsvg, T11446: Closing php tag ?> missing in generated LocalSettings.php, T11437: wikibits.js: scrollEditBox overwrites onsubmit, should use hookEvent method, T11458: Permissions: createpage restriction, T11457: eml interwiki broken on srv60, T11453: Enable subpages in the Template namespace, T11468: Right-to-left rendering on a left-to-right wiki screws up watchlist formatting, T11473: turning off more logos, T11477: Oversight and contribs pages, T11478: Misplaced edit links on 15th Amendment Wiki article, T11480: Overwriting description when overwriting old file, T11484: Mediawiki/PHP locks up, T11487: Backwards scrolling at Special:Whatlinkshere is broken, T11491: Update Japanese i18n patches, T11490: Allow revisions to be shown as they were at a time, T11488: optimize js/css output, T11493: add "Translate" Component to Product:MediaWiki extension at MediaZilla, T11501: Make file uploads patrollable, T11513: Add a sysadmin annotation system for backup dump runner, T11508: Special page to list pages with the "fewest" revisions, T11522: Equal signs swallow parameters, T11532: Allow creation of the EditToken through JavaScript, T11524: Allow centralized user talk page for shared user accounts, T11533: Erratic corrupted diff (repeated lines, etc.), T11537: PHP 5.2.0: is_a(): Deprecated. Please use the instanceof operator, T11556: Update for Bulgarian language (bg), T11555: page creation logs, T11541: Set sitename and logo for Catalan Wiktionary, T11565: Translate template namespace on Hindi, T11559: aggregated Labelled Section Transclusion, T11576: User::isAllowed() doesn't work as expected in the first call, T11569: Add a Portal (Gátt) namespace to Icelandic Wikipedia, T11575: Accept upload description text from GET parameters, T11577: Feature request: Search through history, T11589: [[Special:Templates]] => List all templates (like [[Special:Categories]]), T11590: Allow different default preferences for different Accept-Languages, T11596: SVG previews: 'no higher resolution available', T11604: Get OpenID extension to a state where it could be used on Wikimedia projects as a provider.Sep 7 2016, 11:19 PM