From b352cae97de47e699f6c8810f940dec7a0609a68 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 30 May 2016 08:30:57 -0700 Subject: [PATCH 01/24] Don't stop on read-only mode for read-only storage workflows Summary: Fixes T11042. Currently, all `bin/storage` workflows stop if `cluster.read-only` is set: ``` $ ./bin/storage adjust Usage Exception: Phabricator is currently in read-only mode. Use --force to override this mode. ``` However, some of them (`status`, `dump`, `databases`, etc) are read-only anyway and safe to run. Don't prompt in these cases. Test Plan: - Set `cluster.read-only` to `true`. - Ran `bin/storage dump`, `bin/storage status`, etc. No longer received messages. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11042 Differential Revision: https://secure.phabricator.com/D15987 --- ...atorStorageManagementDatabasesWorkflow.php | 4 ++++ ...abricatorStorageManagementDumpWorkflow.php | 4 ++++ ...bricatorStorageManagementProbeWorkflow.php | 4 ++++ ...orStorageManagementRenamespaceWorkflow.php | 4 ++++ ...bricatorStorageManagementShellWorkflow.php | 6 ++++-- ...ricatorStorageManagementStatusWorkflow.php | 4 ++++ .../PhabricatorStorageManagementWorkflow.php | 21 ++++++++++++------- 7 files changed, 37 insertions(+), 10 deletions(-) diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDatabasesWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDatabasesWorkflow.php index b10e9b9e41..e29dd60e33 100644 --- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDatabasesWorkflow.php +++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDatabasesWorkflow.php @@ -10,6 +10,10 @@ final class PhabricatorStorageManagementDatabasesWorkflow ->setSynopsis(pht('List Phabricator databases.')); } + protected function isReadOnlyWorkflow() { + return true; + } + public function didExecute(PhutilArgumentParser $args) { $api = $this->getAPI(); $patches = $this->getPatches(); diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php index 4933bd70bb..1aec53fd63 100644 --- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php +++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php @@ -19,6 +19,10 @@ final class PhabricatorStorageManagementDumpWorkflow )); } + protected function isReadOnlyWorkflow() { + return true; + } + public function didExecute(PhutilArgumentParser $args) { $api = $this->getAPI(); $patches = $this->getPatches(); diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementProbeWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementProbeWorkflow.php index d65b9bd85a..c45146e6d5 100644 --- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementProbeWorkflow.php +++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementProbeWorkflow.php @@ -10,6 +10,10 @@ final class PhabricatorStorageManagementProbeWorkflow ->setSynopsis(pht('Show approximate table sizes.')); } + protected function isReadOnlyWorkflow() { + return true; + } + public function didExecute(PhutilArgumentParser $args) { $console = PhutilConsole::getConsole(); $console->writeErr( diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementRenamespaceWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementRenamespaceWorkflow.php index ad4960ceba..e64242344b 100644 --- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementRenamespaceWorkflow.php +++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementRenamespaceWorkflow.php @@ -30,6 +30,10 @@ final class PhabricatorStorageManagementRenamespaceWorkflow )); } + protected function isReadOnlyWorkflow() { + return true; + } + public function didExecute(PhutilArgumentParser $args) { $console = PhutilConsole::getConsole(); diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementShellWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementShellWorkflow.php index c66fc73e98..3cf85e5c35 100644 --- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementShellWorkflow.php +++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementShellWorkflow.php @@ -10,9 +10,11 @@ final class PhabricatorStorageManagementShellWorkflow ->setSynopsis(pht('Launch an interactive shell.')); } + protected function isReadOnlyWorkflow() { + return true; + } + public function execute(PhutilArgumentParser $args) { - - $api = $this->getAPI(); list($host, $port) = $this->getBareHostAndPort($api->getHost()); diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementStatusWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementStatusWorkflow.php index be4f8e5434..f9e91427d4 100644 --- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementStatusWorkflow.php +++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementStatusWorkflow.php @@ -10,6 +10,10 @@ final class PhabricatorStorageManagementStatusWorkflow ->setSynopsis(pht('Show patch application status.')); } + protected function isReadOnlyWorkflow() { + return true; + } + public function didExecute(PhutilArgumentParser $args) { $api = $this->getAPI(); $patches = $this->getPatches(); diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php index 7f969793ec..8f22caa803 100644 --- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php +++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php @@ -45,19 +45,24 @@ abstract class PhabricatorStorageManagementWorkflow return $this; } + protected function isReadOnlyWorkflow() { + return false; + } public function execute(PhutilArgumentParser $args) { $this->setDryRun($args->getArg('dryrun')); $this->setForce($args->getArg('force')); - if (PhabricatorEnv::isReadOnly()) { - if ($this->isForce()) { - PhabricatorEnv::setReadOnly(false, null); - } else { - throw new PhutilArgumentUsageException( - pht( - 'Phabricator is currently in read-only mode. Use --force to '. - 'override this mode.')); + if (!$this->isReadOnlyWorkflow()) { + if (PhabricatorEnv::isReadOnly()) { + if ($this->isForce()) { + PhabricatorEnv::setReadOnly(false, null); + } else { + throw new PhutilArgumentUsageException( + pht( + 'Phabricator is currently in read-only mode. Use --force to '. + 'override this mode.')); + } } } From e81637a6c686c9b678197a3f0602da2b8171a2fc Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 30 May 2016 09:09:49 -0700 Subject: [PATCH 02/24] Fix some issues with the "Explain Why" dialog Summary: Ref T11051. This is still not as clear as it should be, but is at least working as intended now. I believe this part of the code just never worked. The test plan on D10489 didn't specifically cover it. Test Plan: Did this sort of thing in a repository: ``` $ git checkout -b featurex $ echo x >> y $ git commit -am wip $ arc diff ``` Then I simulated just pushing it (this flow is a little more involved than necessary): ``` $ arc land --hold $ git commit --amend $ # remove all metadata -- particularly, "Differential Revision"! $ git push HEAD:master ``` I got a not-great but more-useful dialog: {F1667318} Prior to this change, the hash match was incorrectly not reported at all. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11051 Differential Revision: https://secure.phabricator.com/D15989 --- .../DifferentialRevisionCloseDetailsController.php | 2 +- .../query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/applications/differential/controller/DifferentialRevisionCloseDetailsController.php b/src/applications/differential/controller/DifferentialRevisionCloseDetailsController.php index d5a7d897a8..d405125c8e 100644 --- a/src/applications/differential/controller/DifferentialRevisionCloseDetailsController.php +++ b/src/applications/differential/controller/DifferentialRevisionCloseDetailsController.php @@ -93,7 +93,7 @@ final class DifferentialRevisionCloseDetailsController 'href' => $obj_handle->getURI(), ), $obj_handle->getName()); - $body_why = pht( + $body_why[] = pht( 'This commit and the active diff of %s had the same %s hash '. '(%s) so we linked this commit to %s.', $diff_link, diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php index 7c7072dbf5..ab8f5e3e2e 100644 --- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php +++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php @@ -67,19 +67,23 @@ final class DiffusionLowLevelCommitFieldsQuery ->withCommitHashes($hash_list) ->execute(); - if (!empty($revisions)) { + if ($revisions) { $revision = $this->pickBestRevision($revisions); + $fields['revisionID'] = $revision->getID(); $revision_hashes = $revision->getHashes(); + $revision_hashes = DiffusionCommitHash::convertArrayToObjects( $revision_hashes); - $revision_hashes = mpull($revision_hashes, 'getHashType'); + $revision_hashes = mpull($revision_hashes, null, 'getHashType'); + // sort the hashes in the order the mighty // @{class:ArcanstDifferentialRevisionHash} does; probably unnecessary // but should future proof things nicely. $revision_hashes = array_select_keys( $revision_hashes, ArcanistDifferentialRevisionHash::getTypes()); + foreach ($hashes as $hash) { $revision_hash = idx($revision_hashes, $hash->getHashType()); if (!$revision_hash) { From f5f784f4c1d03473567a8b7ce7b05bb5bb0d782a Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 27 May 2016 06:21:19 -0700 Subject: [PATCH 03/24] Version clustered, observed repositories in a reasonable way (by largest discovered HEAD) Summary: Ref T4292. For hosted, clustered repositories we have a good way to increment the internal version of the repository: every time a user pushes something, we increment the version by 1. We don't have a great way to do this for observed/remote repositories because when we `git fetch` we might get nothing, or we might get some changes, and we can't easily tell //what// changes we got. For example, if we see that another node is at "version 97", and we do a fetch and see some changes, we don't know if we're in sync with them (i.e., also at "version 97") or ahead of them (at "version 98"). This implements a simple way to version an observed repository: - Take the head of every branch/tag. - Look them up. - Pick the biggest internal ID number. This will work //except// when branches are deleted, which could cause the version to go backward if the "biggest commit" is the one that was deleted. This should be OK, since it's rare and the effects are minor and the repository will "self-heal" on the next actual push. Test Plan: - Created an observed repository. - Ran `bin/repository update` and observed a sensible version number appear in the version table. - Pushed to the remote, did another update, saw a sensible update. - Did an update with no push, saw no effect on version number. - Toggled repository to hosted, saw the version reset. - Simulated read traffic to out-of-sync node, saw it do a remote fetch. Reviewers: chad Reviewed By: chad Maniphest Tasks: T4292 Differential Revision: https://secure.phabricator.com/D15986 --- .../DiffusionBranchQueryConduitAPIMethod.php | 5 +- .../DiffusionTagsQueryConduitAPIMethod.php | 5 +- .../diffusion/editor/DiffusionURIEditor.php | 13 ++ ...fusionRepositoryStorageManagementPanel.php | 28 ++- .../DiffusionRepositoryClusterEngine.php | 187 +++++++++++++++--- .../lowlevel/DiffusionLowLevelGitRefQuery.php | 35 ++-- .../DiffusionLowLevelResolveRefsQuery.php | 7 +- .../PhabricatorRepositoryDiscoveryEngine.php | 62 +++++- .../PhabricatorRepositoryPullEngine.php | 34 ++-- .../engine/PhabricatorRepositoryRefEngine.php | 10 +- 10 files changed, 316 insertions(+), 70 deletions(-) diff --git a/src/applications/diffusion/conduit/DiffusionBranchQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionBranchQueryConduitAPIMethod.php index 5fd440e4b2..d42c45a96b 100644 --- a/src/applications/diffusion/conduit/DiffusionBranchQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionBranchQueryConduitAPIMethod.php @@ -56,7 +56,10 @@ final class DiffusionBranchQueryConduitAPIMethod } else { $refs = id(new DiffusionLowLevelGitRefQuery()) ->setRepository($repository) - ->withIsOriginBranch(true) + ->withRefTypes( + array( + PhabricatorRepositoryRefCursor::TYPE_BRANCH, + )) ->execute(); } diff --git a/src/applications/diffusion/conduit/DiffusionTagsQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionTagsQueryConduitAPIMethod.php index ddf3a2152b..3de5793289 100644 --- a/src/applications/diffusion/conduit/DiffusionTagsQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionTagsQueryConduitAPIMethod.php @@ -72,7 +72,10 @@ final class DiffusionTagsQueryConduitAPIMethod $refs = id(new DiffusionLowLevelGitRefQuery()) ->setRepository($repository) - ->withIsTag(true) + ->withRefTypes( + array( + PhabricatorRepositoryRefCursor::TYPE_TAG, + )) ->execute(); $tags = array(); diff --git a/src/applications/diffusion/editor/DiffusionURIEditor.php b/src/applications/diffusion/editor/DiffusionURIEditor.php index 020275a367..c22b888ac5 100644 --- a/src/applications/diffusion/editor/DiffusionURIEditor.php +++ b/src/applications/diffusion/editor/DiffusionURIEditor.php @@ -463,6 +463,8 @@ final class DiffusionURIEditor break; } + $was_hosted = $repository->isHosted(); + if ($observe_uri) { $repository ->setHosted(false) @@ -477,6 +479,17 @@ final class DiffusionURIEditor $repository->save(); + $is_hosted = $repository->isHosted(); + + // If we've swapped the repository from hosted to observed or vice versa, + // reset all the cluster version clocks. + if ($was_hosted != $is_hosted) { + $cluster_engine = id(new DiffusionRepositoryClusterEngine()) + ->setViewer($this->getActor()) + ->setRepository($repository) + ->synchronizeWorkingCopyAfterHostingChange(); + } + return $xactions; } diff --git a/src/applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php index 0c723bd9e6..9cf210632d 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php @@ -137,12 +137,28 @@ final class DiffusionRepositoryStorageManagementPanel $version = idx($versions, $device->getPHID()); if ($version) { $version_number = $version->getRepositoryVersion(); - $version_number = phutil_tag( - 'a', - array( - 'href' => "/diffusion/pushlog/view/{$version_number}/", - ), - $version_number); + + $href = null; + if ($repository->isHosted()) { + $href = "/diffusion/pushlog/view/{$version_number}/"; + } else { + $commit = id(new DiffusionCommitQuery()) + ->setViewer($viewer) + ->withIDs(array($version_number)) + ->executeOne(); + if ($commit) { + $href = $commit->getURI(); + } + } + + if ($href) { + $version_number = phutil_tag( + 'a', + array( + 'href' => $href, + ), + $version_number); + } } else { $version_number = '-'; } diff --git a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php index b271c16741..fad8d4019e 100644 --- a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php +++ b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php @@ -82,6 +82,53 @@ final class DiffusionRepositoryClusterEngine extends Phobject { } + /** + * @task sync + */ + public function synchronizeWorkingCopyAfterHostingChange() { + if (!$this->shouldEnableSynchronization()) { + return; + } + + $repository = $this->getRepository(); + $repository_phid = $repository->getPHID(); + + $versions = PhabricatorRepositoryWorkingCopyVersion::loadVersions( + $repository_phid); + $versions = mpull($versions, null, 'getDevicePHID'); + + // After converting a hosted repository to observed, or vice versa, we + // need to reset version numbers because the clocks for observed and hosted + // repositories run on different units. + + // We identify all the cluster leaders and reset their version to 0. + // We identify all the cluster followers and demote them. + + // This allows the cluter to start over again at version 0 but keep the + // same leaders. + + if ($versions) { + $max_version = (int)max(mpull($versions, 'getRepositoryVersion')); + foreach ($versions as $version) { + $device_phid = $version->getDevicePHID(); + + if ($version->getRepositoryVersion() == $max_version) { + PhabricatorRepositoryWorkingCopyVersion::updateVersion( + $repository_phid, + $device_phid, + 0); + } else { + PhabricatorRepositoryWorkingCopyVersion::demoteDevice( + $repository_phid, + $device_phid); + } + } + } + + return $this; + } + + /** * @task sync */ @@ -149,14 +196,18 @@ final class DiffusionRepositoryClusterEngine extends Phobject { $max_version = (int)max(mpull($versions, 'getRepositoryVersion')); if ($max_version > $this_version) { - $fetchable = array(); - foreach ($versions as $version) { - if ($version->getRepositoryVersion() == $max_version) { - $fetchable[] = $version->getDevicePHID(); + if ($repository->isHosted()) { + $fetchable = array(); + foreach ($versions as $version) { + if ($version->getRepositoryVersion() == $max_version) { + $fetchable[] = $version->getDevicePHID(); + } } - } - $this->synchronizeWorkingCopyFromDevices($fetchable); + $this->synchronizeWorkingCopyFromDevices($fetchable); + } else { + $this->synchornizeWorkingCopyFromRemote(); + } PhabricatorRepositoryWorkingCopyVersion::updateVersion( $repository_phid, @@ -329,6 +380,47 @@ final class DiffusionRepositoryClusterEngine extends Phobject { } + public function synchronizeWorkingCopyAfterDiscovery($new_version) { + if (!$this->shouldEnableSynchronization()) { + return; + } + + $repository = $this->getRepository(); + $repository_phid = $repository->getPHID(); + if ($repository->isHosted()) { + return; + } + + $viewer = $this->getViewer(); + + $device = AlmanacKeys::getLiveDevice(); + $device_phid = $device->getPHID(); + + // NOTE: We are not holding a lock here because this method is only called + // from PhabricatorRepositoryDiscoveryEngine, which already holds a device + // lock. Even if we do race here and record an older version, the + // consequences are mild: we only do extra work to correct it later. + + $versions = PhabricatorRepositoryWorkingCopyVersion::loadVersions( + $repository_phid); + $versions = mpull($versions, null, 'getDevicePHID'); + + $this_version = idx($versions, $device_phid); + if ($this_version) { + $this_version = (int)$this_version->getRepositoryVersion(); + } else { + $this_version = -1; + } + + if ($new_version > $this_version) { + PhabricatorRepositoryWorkingCopyVersion::updateVersion( + $repository_phid, + $device_phid, + $new_version); + } + } + + /** * @task sync */ @@ -471,13 +563,6 @@ final class DiffusionRepositoryClusterEngine extends Phobject { return false; } - // TODO: It may eventually make sense to try to version and synchronize - // observed repositories (so that daemons don't do reads against out-of - // date hosts), but don't bother for now. - if (!$repository->isHosted()) { - return false; - } - $device = AlmanacKeys::getLiveDevice(); if (!$device) { return false; @@ -487,6 +572,50 @@ final class DiffusionRepositoryClusterEngine extends Phobject { } + /** + * @task internal + */ + private function synchornizeWorkingCopyFromRemote() { + $repository = $this->getRepository(); + $device = AlmanacKeys::getLiveDevice(); + + $local_path = $repository->getLocalPath(); + $fetch_uri = $repository->getRemoteURIEnvelope(); + + if ($repository->isGit()) { + $this->requireWorkingCopy(); + + $argv = array( + 'fetch --prune -- %P %s', + $fetch_uri, + '+refs/*:refs/*', + ); + } else { + throw new Exception(pht('Remote sync only supported for git!')); + } + + $future = DiffusionCommandEngine::newCommandEngine($repository) + ->setArgv($argv) + ->setSudoAsDaemon(true) + ->setCredentialPHID($repository->getCredentialPHID()) + ->setProtocol($repository->getRemoteProtocol()) + ->newFuture(); + + $future->setCWD($local_path); + + try { + $future->resolvex(); + } catch (Exception $ex) { + $this->logLine( + pht( + 'Synchronization of "%s" from remote failed: %s', + $device->getName(), + $ex->getMessage())); + throw $ex; + } + } + + /** * @task internal */ @@ -560,17 +689,7 @@ final class DiffusionRepositoryClusterEngine extends Phobject { $local_path = $repository->getLocalPath(); if ($repository->isGit()) { - if (!Filesystem::pathExists($local_path)) { - throw new Exception( - pht( - 'Repository "%s" does not have a working copy on this device '. - 'yet, so it can not be synchronized. Wait for the daemons to '. - 'construct one or run `bin/repository update %s` on this host '. - '("%s") to build it explicitly.', - $repository->getDisplayName(), - $repository->getMonogram(), - $device->getName())); - } + $this->requireWorkingCopy(); $argv = array( 'fetch --prune -- %s %s', @@ -622,4 +741,24 @@ final class DiffusionRepositoryClusterEngine extends Phobject { } return $this; } + + private function requireWorkingCopy() { + $repository = $this->getRepository(); + $local_path = $repository->getLocalPath(); + + if (!Filesystem::pathExists($local_path)) { + $device = AlmanacKeys::getLiveDevice(); + + throw new Exception( + pht( + 'Repository "%s" does not have a working copy on this device '. + 'yet, so it can not be synchronized. Wait for the daemons to '. + 'construct one or run `bin/repository update %s` on this host '. + '("%s") to build it explicitly.', + $repository->getDisplayName(), + $repository->getMonogram(), + $device->getName())); + } + } + } diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelGitRefQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelGitRefQuery.php index 1f17f5fe9f..038d833670 100644 --- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelGitRefQuery.php +++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelGitRefQuery.php @@ -6,30 +6,33 @@ */ final class DiffusionLowLevelGitRefQuery extends DiffusionLowLevelQuery { - private $isTag; - private $isOriginBranch; + private $refTypes; - public function withIsTag($is_tag) { - $this->isTag = $is_tag; - return $this; - } - - public function withIsOriginBranch($is_origin_branch) { - $this->isOriginBranch = $is_origin_branch; + public function withRefTypes(array $ref_types) { + $this->refTypes = $ref_types; return $this; } protected function executeQuery() { + $ref_types = $this->refTypes; + if ($ref_types) { + $type_branch = PhabricatorRepositoryRefCursor::TYPE_BRANCH; + $type_tag = PhabricatorRepositoryRefCursor::TYPE_TAG; + + $ref_types = array_fuse($ref_types); + + $with_branches = isset($ref_types[$type_branch]); + $with_tags = isset($ref_types[$type_tag]); + } else { + $with_branches = true; + $with_tags = true; + } + $repository = $this->getRepository(); $prefixes = array(); - $any = ($this->isTag || $this->isOriginBranch); - if (!$any) { - throw new Exception(pht('Specify types of refs to query.')); - } - - if ($this->isOriginBranch) { + if ($with_branches) { if ($repository->isWorkingCopyBare()) { $prefix = 'refs/heads/'; } else { @@ -39,7 +42,7 @@ final class DiffusionLowLevelGitRefQuery extends DiffusionLowLevelQuery { $prefixes[] = $prefix; } - if ($this->isTag) { + if ($with_tags) { $prefixes[] = 'refs/tags/'; } diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php index 4e9ed246d9..8f9493a67a 100644 --- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php +++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php @@ -66,8 +66,11 @@ final class DiffusionLowLevelResolveRefsQuery // First, resolve branches and tags. $ref_map = id(new DiffusionLowLevelGitRefQuery()) ->setRepository($repository) - ->withIsTag(true) - ->withIsOriginBranch(true) + ->withRefTypes( + array( + PhabricatorRepositoryRefCursor::TYPE_BRANCH, + PhabricatorRepositoryRefCursor::TYPE_TAG, + )) ->execute(); $ref_map = mgroup($ref_map, 'getShortName'); diff --git a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php index 78a40986e3..9e0e13c720 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php @@ -63,6 +63,7 @@ final class PhabricatorRepositoryDiscoveryEngine private function discoverCommitsWithLock() { $repository = $this->getRepository(); + $viewer = $this->getViewer(); $vcs = $repository->getVersionControlSystem(); switch ($vcs) { @@ -104,6 +105,14 @@ final class PhabricatorRepositoryDiscoveryEngine $this->commitCache[$ref->getIdentifier()] = true; } + $version = $this->getObservedVersion($repository); + if ($version !== null) { + id(new DiffusionRepositoryClusterEngine()) + ->setViewer($viewer) + ->setRepository($repository) + ->synchronizeWorkingCopyAfterDiscovery($version); + } + return $refs; } @@ -121,9 +130,15 @@ final class PhabricatorRepositoryDiscoveryEngine $this->verifyGitOrigin($repository); } + // TODO: This should also import tags, but some of the logic is still + // branch-specific today. + $branches = id(new DiffusionLowLevelGitRefQuery()) ->setRepository($repository) - ->withIsOriginBranch(true) + ->withRefTypes( + array( + PhabricatorRepositoryRefCursor::TYPE_BRANCH, + )) ->execute(); if (!$branches) { @@ -642,4 +657,49 @@ final class PhabricatorRepositoryDiscoveryEngine return true; } + + private function getObservedVersion(PhabricatorRepository $repository) { + if ($repository->isHosted()) { + return null; + } + + if ($repository->isGit()) { + return $this->getGitObservedVersion($repository); + } + + return null; + } + + private function getGitObservedVersion(PhabricatorRepository $repository) { + $refs = id(new DiffusionLowLevelGitRefQuery()) + ->setRepository($repository) + ->execute(); + if (!$refs) { + return null; + } + + // In Git, the observed version is the most recently discovered commit + // at any repository HEAD. It's possible for this to regress temporarily + // if a branch is pushed and then deleted. This is acceptable because it + // doesn't do anything meaningfully bad and will fix itself on the next + // push. + + $ref_identifiers = mpull($refs, 'getCommitIdentifier'); + $ref_identifiers = array_fuse($ref_identifiers); + + $version = queryfx_one( + $repository->establishConnection('w'), + 'SELECT MAX(id) version FROM %T WHERE repositoryID = %d + AND commitIdentifier IN (%Ls)', + id(new PhabricatorRepositoryCommit())->getTableName(), + $repository->getID(), + $ref_identifiers); + + if (!$version) { + return null; + } + + return (int)$version['version']; + } + } diff --git a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php index 9e151ecd81..3d7dce90fa 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php @@ -108,27 +108,27 @@ final class PhabricatorRepositoryPullEngine } else { $this->executeSubversionCreate(); } - } else { - if (!$repository->isHosted()) { - $this->logPull( - pht( - 'Updating the working copy for repository "%s".', - $repository->getDisplayName())); - if ($is_git) { - $this->verifyGitOrigin($repository); - $this->executeGitUpdate(); - } else if ($is_hg) { - $this->executeMercurialUpdate(); - } + } + + id(new DiffusionRepositoryClusterEngine()) + ->setViewer($viewer) + ->setRepository($repository) + ->synchronizeWorkingCopyBeforeRead(); + + if (!$repository->isHosted()) { + $this->logPull( + pht( + 'Updating the working copy for repository "%s".', + $repository->getDisplayName())); + if ($is_git) { + $this->verifyGitOrigin($repository); + $this->executeGitUpdate(); + } else if ($is_hg) { + $this->executeMercurialUpdate(); } } if ($repository->isHosted()) { - id(new DiffusionRepositoryClusterEngine()) - ->setViewer($viewer) - ->setRepository($repository) - ->synchronizeWorkingCopyBeforeRead(); - if ($is_git) { $this->installGitHook(); } else if ($is_svn) { diff --git a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php index 3909cc83b0..da89db75e0 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php @@ -452,7 +452,10 @@ final class PhabricatorRepositoryRefEngine private function loadGitBranchPositions(PhabricatorRepository $repository) { return id(new DiffusionLowLevelGitRefQuery()) ->setRepository($repository) - ->withIsOriginBranch(true) + ->withRefTypes( + array( + PhabricatorRepositoryRefCursor::TYPE_BRANCH, + )) ->execute(); } @@ -463,7 +466,10 @@ final class PhabricatorRepositoryRefEngine private function loadGitTagPositions(PhabricatorRepository $repository) { return id(new DiffusionLowLevelGitRefQuery()) ->setRepository($repository) - ->withIsTag(true) + ->withRefTypes( + array( + PhabricatorRepositoryRefCursor::TYPE_TAG, + )) ->execute(); } From fb1cc8cc58a46522e3cee8f05dedd9e6d853fab6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 30 May 2016 10:02:16 -0700 Subject: [PATCH 04/24] Remove Lamson documentation Summary: Fixes T11054. This project's website has been down for more than a year: It hasn't received any new commits for three years (March, 2013): Instructions like this are a good candidate for community ownership rather than upstream maintenance. I don't think we'd accept these instructions upstream today. Test Plan: `grep -i lamson` Reviewers: chad Reviewed By: chad Maniphest Tasks: T11054 Differential Revision: https://secure.phabricator.com/D15990 --- .../configuring_inbound_email.diviner | 36 ------------------- 1 file changed, 36 deletions(-) diff --git a/src/docs/user/configuration/configuring_inbound_email.diviner b/src/docs/user/configuration/configuring_inbound_email.diviner index 4b20881f58..5b47a17831 100644 --- a/src/docs/user/configuration/configuring_inbound_email.diviner +++ b/src/docs/user/configuration/configuring_inbound_email.diviner @@ -216,39 +216,3 @@ Finally, edit `/etc/mail/virtusertable` and add an entry like this: That will forward all mail to @yourdomain.com to the Phabricator processing script. Run `sudo /etc/mail/make` or similar and then restart sendmail with `sudo /etc/init.d/sendmail restart`. - -= Local MTA: Configuring Lamson = - -Before you can configure Lamson, you need to install Mailparse. See the section -"Installing Mailparse" above. - -In contrast to Sendmail, Lamson is relatively easy to configure. It is fairly -minimal, and is suitable for a development or testing environment. Lamson -listens for incoming SMTP mails and passes the content directly to Phabricator. - -To get started, follow the provided instructions -() to set up an instance. -One likely deployment issue is that binding to port 25 requires root -privileges. Lamson is capable of starting as root then dropping privileges, but -you must supply `-uid` and `-gid` arguments to do so, as demonstrated by -Step 8 in Lamson's deployment tutorial (located here: -). - -The Lamson handler code itself is very concise; it merely needs to pass the -content of the email to Phabricator: - - import logging, subprocess - from lamson.routing import route, stateless - from lamson import view - - PHABRICATOR_ROOT = "/path/to/phabricator" - PHABRICATOR_ENV = "custom/myconf" - LOGGING_ENABLED = True - - @route("(address)@(host)", address=".+") - @stateless - def START(message, address=None, host=None): - if LOGGING_ENABLED: - logging.debug("%s", message.original) - process = subprocess.Popen([PHABRICATOR_ROOT + "scripts/mail/mail_handler.php",PHABRICATOR_ENV],stdin=subprocess.PIPE) - process.communicate(message.original) From 92ea4fb098cb44b78fd601894fbbac67772f55b5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 30 May 2016 08:44:17 -0700 Subject: [PATCH 05/24] Stop two special cache writes in read-only mode Summary: Ref T10769. The user availability cache write shouldn't happen in read-only mode, nor should the Differential parse cache write. (We might want to turn off the availbility feature completely since it's potentially expensive if we can't cache it, but I think we're OK for now.) Test Plan: In read-only mode: - Browsed as a user with an out-of-date availability cache. - Loaded an older revision without cached parse data. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10769 Differential Revision: https://secure.phabricator.com/D15988 --- .../differential/parser/DifferentialChangesetParser.php | 4 ++++ src/applications/people/storage/PhabricatorUser.php | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index 9fc0adf537..e49538fd91 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -458,6 +458,10 @@ final class DifferentialChangesetParser extends Phobject { } public function saveCache() { + if (PhabricatorEnv::isReadOnly()) { + return false; + } + if ($this->highlightErrors) { return false; } diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 464c14492f..85b2590d33 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -968,6 +968,10 @@ final class PhabricatorUser * @task availability */ public function writeAvailabilityCache(array $availability, $ttl) { + if (PhabricatorEnv::isReadOnly()) { + return $this; + } + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); queryfx( $this->establishConnection('w'), From e64dfbcbd85fd816c69864b1f206aeb83dbaaff2 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Tue, 31 May 2016 12:04:44 -0700 Subject: [PATCH 06/24] Fix top margin on infoview + two-column Summary: When you save settings or don't have reviewers, we show an info box inside an object box. This rule is squashing the margin. Blame seems to indicate I added it for Differential updates, but in re-visiting Differential, I'm unable to trigger any broken CSS with this change. But keep an eye out? Test Plan: Badges, Settings, Differential Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D15992 --- resources/celerity/map.php | 4 ++-- webroot/rsrc/css/phui/phui-two-column-view.css | 4 ---- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 3bd1fd002a..6381ac0a3c 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -156,7 +156,7 @@ return array( 'rsrc/css/phui/phui-status.css' => 'd5263e49', 'rsrc/css/phui/phui-tag-view.css' => '6bbd83e2', 'rsrc/css/phui/phui-timeline-view.css' => '6e342216', - 'rsrc/css/phui/phui-two-column-view.css' => 'b9538af1', + 'rsrc/css/phui/phui-two-column-view.css' => 'ce672be4', 'rsrc/css/phui/workboards/phui-workboard-color.css' => 'ac6fe6a7', 'rsrc/css/phui/workboards/phui-workboard.css' => 'e6d89647', 'rsrc/css/phui/workboards/phui-workcard.css' => '0c62d7c5', @@ -859,7 +859,7 @@ return array( 'phui-tag-view-css' => '6bbd83e2', 'phui-theme-css' => '027ba77e', 'phui-timeline-view-css' => '6e342216', - 'phui-two-column-view-css' => 'b9538af1', + 'phui-two-column-view-css' => 'ce672be4', 'phui-workboard-color-css' => 'ac6fe6a7', 'phui-workboard-view-css' => 'e6d89647', 'phui-workcard-view-css' => '0c62d7c5', diff --git a/webroot/rsrc/css/phui/phui-two-column-view.css b/webroot/rsrc/css/phui/phui-two-column-view.css index 4d71c3d5ba..9aa4c8047c 100644 --- a/webroot/rsrc/css/phui/phui-two-column-view.css +++ b/webroot/rsrc/css/phui/phui-two-column-view.css @@ -203,10 +203,6 @@ padding: 16px; } -.phui-two-column-view .phui-two-column-row .phui-object-box - .phui-info-view { - margin: 0; -} .phui-two-column-view .phui-box-blue-property .phui-header-shell + .phui-info-view { From b23c85b16915d94cebf37b9cefdc981702ae3f8c Mon Sep 17 00:00:00 2001 From: Chad Little Date: Tue, 31 May 2016 12:15:23 -0700 Subject: [PATCH 07/24] Add role=dialog to all dialogs Summary: Seen some complaints about usability here, adding role=dialog to improve when these trigger. Test Plan: Turn on Voiceover, tab over to log out link, here proper dialog title and text before highlighted submit button. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D15993 --- src/view/AphrontDialogView.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/view/AphrontDialogView.php b/src/view/AphrontDialogView.php index 26dee0c398..218839ef6e 100644 --- a/src/view/AphrontDialogView.php +++ b/src/view/AphrontDialogView.php @@ -276,6 +276,7 @@ final class AphrontDialogView $attributes = array( 'class' => implode(' ', $classes), 'sigil' => 'jx-dialog', + 'role' => 'dialog', ); $form_attributes = array( From b256f2d7b2db27318728a63b7c2565241f87452c Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 27 May 2016 05:58:46 -0700 Subject: [PATCH 08/24] Prepare UserPreferences for transactions Summary: Ref T4103. This give preferences a PHID, policy/transaction interfaces, a transaction table, and a Query class. This doesn't actually change how they're edited, yet. Test Plan: - Ran migrations. - Inspected database for date created, date modified, PHIDs. - Changed some of my preferences. - Deleted a user's preferences, verified they reset properly. - Set some preferences as a new user, got a new row. - Destroyed a user, verified their preferences were destroyed. - Sent Conpherence messages. - Send mail. - Tried to edit another user's settings. - Tried to edit a bot's settings as a non-admin. - Edited a bot's settings as an admin (technically, none of the editable settings are actually stored in the settings table, currently). Reviewers: chad Reviewed By: chad Maniphest Tasks: T4103 Differential Revision: https://secure.phabricator.com/D15991 --- .../autopatches/20160531.pref.01.xaction.sql | 19 +++ .../20160531.pref.02.datecreatecol.sql | 2 + .../20160531.pref.03.datemodcol.sql | 2 + .../20160531.pref.04.datecreateval.sql | 2 + .../20160531.pref.05.datemodval.sql | 2 + .../autopatches/20160531.pref.06.phidcol.sql | 2 + .../autopatches/20160531.pref.07.phidval.php | 17 +++ src/__phutil_library_map__.php | 13 +- .../conpherence/editor/ConpherenceEditor.php | 14 ++- .../feed/PhabricatorFeedStoryPublisher.php | 7 +- .../storage/PhabricatorMetaMTAMail.php | 7 +- .../people/storage/PhabricatorUser.php | 16 +-- .../PhabricatorUserPreferencesPHIDType.php | 40 ++++++ .../query/PhabricatorUserPreferencesQuery.php | 118 +++++++++++++++++ .../storage/PhabricatorUserPreferences.php | 119 +++++++++++++++++- .../PhabricatorUserPreferencesTransaction.php | 18 +++ 16 files changed, 379 insertions(+), 19 deletions(-) create mode 100644 resources/sql/autopatches/20160531.pref.01.xaction.sql create mode 100644 resources/sql/autopatches/20160531.pref.02.datecreatecol.sql create mode 100644 resources/sql/autopatches/20160531.pref.03.datemodcol.sql create mode 100644 resources/sql/autopatches/20160531.pref.04.datecreateval.sql create mode 100644 resources/sql/autopatches/20160531.pref.05.datemodval.sql create mode 100644 resources/sql/autopatches/20160531.pref.06.phidcol.sql create mode 100644 resources/sql/autopatches/20160531.pref.07.phidval.php create mode 100644 src/applications/settings/phid/PhabricatorUserPreferencesPHIDType.php create mode 100644 src/applications/settings/query/PhabricatorUserPreferencesQuery.php create mode 100644 src/applications/settings/storage/PhabricatorUserPreferencesTransaction.php diff --git a/resources/sql/autopatches/20160531.pref.01.xaction.sql b/resources/sql/autopatches/20160531.pref.01.xaction.sql new file mode 100644 index 0000000000..0ff33f4b59 --- /dev/null +++ b/resources/sql/autopatches/20160531.pref.01.xaction.sql @@ -0,0 +1,19 @@ +CREATE TABLE {$NAMESPACE}_user.user_preferencestransaction ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + phid VARBINARY(64) NOT NULL, + authorPHID VARBINARY(64) NOT NULL, + objectPHID VARBINARY(64) NOT NULL, + viewPolicy VARBINARY(64) NOT NULL, + editPolicy VARBINARY(64) NOT NULL, + commentPHID VARBINARY(64) DEFAULT NULL, + commentVersion INT UNSIGNED NOT NULL, + transactionType VARCHAR(32) COLLATE {$COLLATE_TEXT} NOT NULL, + oldValue LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL, + newValue LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL, + contentSource LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL, + metadata LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + UNIQUE KEY `key_phid` (`phid`), + KEY `key_object` (`objectPHID`) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20160531.pref.02.datecreatecol.sql b/resources/sql/autopatches/20160531.pref.02.datecreatecol.sql new file mode 100644 index 0000000000..5f583fc972 --- /dev/null +++ b/resources/sql/autopatches/20160531.pref.02.datecreatecol.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_user.user_preferences + ADD dateCreated INT UNSIGNED NOT NULL; diff --git a/resources/sql/autopatches/20160531.pref.03.datemodcol.sql b/resources/sql/autopatches/20160531.pref.03.datemodcol.sql new file mode 100644 index 0000000000..bd9ebc96f7 --- /dev/null +++ b/resources/sql/autopatches/20160531.pref.03.datemodcol.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_user.user_preferences + ADD dateModified INT UNSIGNED NOT NULL; diff --git a/resources/sql/autopatches/20160531.pref.04.datecreateval.sql b/resources/sql/autopatches/20160531.pref.04.datecreateval.sql new file mode 100644 index 0000000000..fcaa8e0e0d --- /dev/null +++ b/resources/sql/autopatches/20160531.pref.04.datecreateval.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_user.user_preferences + SET dateCreated = UNIX_TIMESTAMP() WHERE dateCreated = 0; diff --git a/resources/sql/autopatches/20160531.pref.05.datemodval.sql b/resources/sql/autopatches/20160531.pref.05.datemodval.sql new file mode 100644 index 0000000000..8571509782 --- /dev/null +++ b/resources/sql/autopatches/20160531.pref.05.datemodval.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_user.user_preferences + SET dateModified = UNIX_TIMESTAMP() WHERE dateModified = 0; diff --git a/resources/sql/autopatches/20160531.pref.06.phidcol.sql b/resources/sql/autopatches/20160531.pref.06.phidcol.sql new file mode 100644 index 0000000000..ff6ac80010 --- /dev/null +++ b/resources/sql/autopatches/20160531.pref.06.phidcol.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_user.user_preferences + ADD phid VARBINARY(64) NOT NULL; diff --git a/resources/sql/autopatches/20160531.pref.07.phidval.php b/resources/sql/autopatches/20160531.pref.07.phidval.php new file mode 100644 index 0000000000..d5cb614c09 --- /dev/null +++ b/resources/sql/autopatches/20160531.pref.07.phidval.php @@ -0,0 +1,17 @@ +establishConnection('w'); + +foreach (new LiskMigrationIterator($table) as $row) { + if ($row->getPHID() !== '') { + continue; + } + + queryfx( + $conn_w, + 'UPDATE %T SET phid = %s WHERE id = %d', + $table->getTableName(), + $table->generatePHID(), + $row->getID()); +} diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 6c5ac74685..f3f92e6998 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3592,6 +3592,9 @@ phutil_register_library_map(array( 'PhabricatorUserLogView' => 'applications/people/view/PhabricatorUserLogView.php', 'PhabricatorUserPHIDResolver' => 'applications/phid/resolver/PhabricatorUserPHIDResolver.php', 'PhabricatorUserPreferences' => 'applications/settings/storage/PhabricatorUserPreferences.php', + 'PhabricatorUserPreferencesPHIDType' => 'applications/settings/phid/PhabricatorUserPreferencesPHIDType.php', + 'PhabricatorUserPreferencesQuery' => 'applications/settings/query/PhabricatorUserPreferencesQuery.php', + 'PhabricatorUserPreferencesTransaction' => 'applications/settings/storage/PhabricatorUserPreferencesTransaction.php', 'PhabricatorUserProfile' => 'applications/people/storage/PhabricatorUserProfile.php', 'PhabricatorUserProfileEditor' => 'applications/people/editor/PhabricatorUserProfileEditor.php', 'PhabricatorUserRealNameField' => 'applications/people/customfield/PhabricatorUserRealNameField.php', @@ -8342,7 +8345,15 @@ phutil_register_library_map(array( ), 'PhabricatorUserLogView' => 'AphrontView', 'PhabricatorUserPHIDResolver' => 'PhabricatorPHIDResolver', - 'PhabricatorUserPreferences' => 'PhabricatorUserDAO', + 'PhabricatorUserPreferences' => array( + 'PhabricatorUserDAO', + 'PhabricatorPolicyInterface', + 'PhabricatorDestructibleInterface', + 'PhabricatorApplicationTransactionInterface', + ), + 'PhabricatorUserPreferencesPHIDType' => 'PhabricatorPHIDType', + 'PhabricatorUserPreferencesQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'PhabricatorUserPreferencesTransaction' => 'PhabricatorApplicationTransaction', 'PhabricatorUserProfile' => 'PhabricatorUserDAO', 'PhabricatorUserProfileEditor' => 'PhabricatorApplicationTransactionEditor', 'PhabricatorUserRealNameField' => 'PhabricatorUserCustomField', diff --git a/src/applications/conpherence/editor/ConpherenceEditor.php b/src/applications/conpherence/editor/ConpherenceEditor.php index 9f2e3e2b2a..c68bb429a3 100644 --- a/src/applications/conpherence/editor/ConpherenceEditor.php +++ b/src/applications/conpherence/editor/ConpherenceEditor.php @@ -533,13 +533,20 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor { protected function getMailTo(PhabricatorLiskDAO $object) { $to_phids = array(); + $participants = $object->getParticipants(); - if (empty($participants)) { + if (!$participants) { return $to_phids; } - $preferences = id(new PhabricatorUserPreferences()) - ->loadAllWhere('userPHID in (%Ls)', array_keys($participants)); + + $participant_phids = mpull($participants, 'getParticipantPHID'); + + $preferences = id(new PhabricatorUserPreferencesQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withUserPHIDs($participant_phids) + ->execute(); $preferences = mpull($preferences, null, 'getUserPHID'); + foreach ($participants as $phid => $participant) { $default = ConpherenceSettings::EMAIL_ALWAYS; $preference = idx($preferences, $phid); @@ -557,6 +564,7 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor { $to_phids[] = $phid; } } + return $to_phids; } diff --git a/src/applications/feed/PhabricatorFeedStoryPublisher.php b/src/applications/feed/PhabricatorFeedStoryPublisher.php index 5a3ec6ea87..be476012c3 100644 --- a/src/applications/feed/PhabricatorFeedStoryPublisher.php +++ b/src/applications/feed/PhabricatorFeedStoryPublisher.php @@ -207,9 +207,10 @@ final class PhabricatorFeedStoryPublisher extends Phobject { $tags = $this->getMailTags(); if ($tags) { - $all_prefs = id(new PhabricatorUserPreferences())->loadAllWhere( - 'userPHID in (%Ls)', - $phids); + $all_prefs = id(new PhabricatorUserPreferencesQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withUserPHIDs($phids) + ->execute(); $all_prefs = mpull($all_prefs, null, 'getUserPHID'); } diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index 1ae5884228..95740ecbd4 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -940,9 +940,10 @@ final class PhabricatorMetaMTAMail } } - $all_prefs = id(new PhabricatorUserPreferences())->loadAllWhere( - 'userPHID in (%Ls)', - $actor_phids); + $all_prefs = id(new PhabricatorUserPreferencesQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withUserPHIDs($actor_phids) + ->execute(); $all_prefs = mpull($all_prefs, null, 'getUserPHID'); $value_email = PhabricatorUserPreferences::MAILTAG_PREFERENCE_EMAIL; diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 85b2590d33..855392fc62 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -494,9 +494,10 @@ final class PhabricatorUser $preferences = null; if ($this->getPHID()) { - $preferences = id(new PhabricatorUserPreferences())->loadOneWhere( - 'userPHID = %s', - $this->getPHID()); + $preferences = id(new PhabricatorUserPreferencesQuery()) + ->setViewer($this) + ->withUsers(array($this)) + ->executeOne(); } if (!$preferences) { @@ -1293,11 +1294,12 @@ final class PhabricatorUser $external->delete(); } - $prefs = id(new PhabricatorUserPreferences())->loadAllWhere( - 'userPHID = %s', - $this->getPHID()); + $prefs = id(new PhabricatorUserPreferencesQuery()) + ->setViewer($engine->getViewer()) + ->withUsers(array($this)) + ->execute(); foreach ($prefs as $pref) { - $pref->delete(); + $engine->destroyObject($pref); } $profiles = id(new PhabricatorUserProfile())->loadAllWhere( diff --git a/src/applications/settings/phid/PhabricatorUserPreferencesPHIDType.php b/src/applications/settings/phid/PhabricatorUserPreferencesPHIDType.php new file mode 100644 index 0000000000..8bd341b8c6 --- /dev/null +++ b/src/applications/settings/phid/PhabricatorUserPreferencesPHIDType.php @@ -0,0 +1,40 @@ +withPHIDs($phids); + } + + public function loadHandles( + PhabricatorHandleQuery $query, + array $handles, + array $objects) { + + $viewer = $query->getViewer(); + foreach ($handles as $phid => $handle) { + $preferences = $objects[$phid]; + + $handle->setName(pht('Settings %d', $preferences->getID())); + } + } + +} diff --git a/src/applications/settings/query/PhabricatorUserPreferencesQuery.php b/src/applications/settings/query/PhabricatorUserPreferencesQuery.php new file mode 100644 index 0000000000..fc3a332c57 --- /dev/null +++ b/src/applications/settings/query/PhabricatorUserPreferencesQuery.php @@ -0,0 +1,118 @@ +ids = $ids; + return $this; + } + + public function withPHIDs(array $phids) { + $this->phids = $phids; + return $this; + } + + public function withUserPHIDs(array $phids) { + $this->userPHIDs = $phids; + return $this; + } + + public function withUsers(array $users) { + assert_instances_of($users, 'PhabricatorUser'); + $this->users = mpull($users, null, 'getPHID'); + $this->withUserPHIDs(array_keys($this->users)); + return $this; + } + + public function newResultObject() { + return new PhabricatorUserPreferences(); + } + + protected function loadPage() { + return $this->loadStandardPage($this->newResultObject()); + } + + protected function willFilterPage(array $prefs) { + $user_phids = mpull($prefs, 'getUserPHID'); + $user_phids = array_filter($user_phids); + + // If some of the preferences are attached to users, try to use any objects + // we were handed first. If we're missing some, load them. + + if ($user_phids) { + $users = $this->users; + + $user_phids = array_fuse($user_phids); + $load_phids = array_diff_key($user_phids, $users); + $load_phids = array_keys($load_phids); + + if ($load_phids) { + $load_users = id(new PhabricatorPeopleQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs($load_phids) + ->execute(); + $load_users = mpull($load_users, null, 'getPHID'); + $users += $load_users; + } + } else { + $users = array(); + } + + foreach ($prefs as $key => $pref) { + $user_phid = $pref->getUserPHID(); + if (!$user_phid) { + $pref->attachUser(null); + continue; + } + + $user = idx($users, $user_phid); + if (!$user) { + $this->didRejectResult($pref); + unset($prefs[$key]); + continue; + } + + $pref->attachUser($user); + } + + return $prefs; + } + + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); + + if ($this->ids !== null) { + $where[] = qsprintf( + $conn, + 'id IN (%Ld)', + $this->ids); + } + + if ($this->phids !== null) { + $where[] = qsprintf( + $conn, + 'phid IN (%Ls)', + $this->phids); + } + + if ($this->userPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'userPHID IN (%Ls)', + $this->userPHIDs); + } + + return $where; + } + + public function getQueryApplicationClass() { + return 'PhabricatorSettingsApplication'; + } + +} diff --git a/src/applications/settings/storage/PhabricatorUserPreferences.php b/src/applications/settings/storage/PhabricatorUserPreferences.php index c14cc9b910..f90f82c4aa 100644 --- a/src/applications/settings/storage/PhabricatorUserPreferences.php +++ b/src/applications/settings/storage/PhabricatorUserPreferences.php @@ -1,6 +1,11 @@ true, self::CONFIG_SERIALIZATION => array( 'preferences' => self::SERIALIZATION_JSON, ), - self::CONFIG_TIMESTAMPS => false, self::CONFIG_KEY_SCHEMA => array( 'userPHID' => array( 'columns' => array('userPHID'), @@ -66,6 +73,11 @@ final class PhabricatorUserPreferences extends PhabricatorUserDAO { ) + parent::getConfiguration(); } + public function generatePHID() { + return PhabricatorPHID::generateNewPHID( + PhabricatorUserPreferencesPHIDType::TYPECONST); + } + public function getPreference($key, $default = null) { return idx($this->preferences, $key, $default); } @@ -115,4 +127,107 @@ final class PhabricatorUserPreferences extends PhabricatorUserDAO { return preg_replace('([^a-z0-9 ,"./]+)i', '', $monospaced); } + public function attachUser(PhabricatorUser $user = null) { + $this->user = $user; + return $this; + } + + public function getUser() { + return $this->assertAttached($this->user); + } + + public function hasManagedUser() { + $user_phid = $this->getUserPHID(); + if (!$user_phid) { + return false; + } + + $user = $this->getUser(); + if ($user->getIsSystemAgent() || $user->getIsMailingList()) { + return true; + } + + return false; + } + + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + + + public function getCapabilities() { + return array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + ); + } + + public function getPolicy($capability) { + switch ($capability) { + case PhabricatorPolicyCapability::CAN_VIEW: + $user_phid = $this->getUserPHID(); + if ($user_phid) { + return $user_phid; + } + + return PhabricatorPolicies::getMostOpenPolicy(); + case PhabricatorPolicyCapability::CAN_EDIT: + if ($this->hasManagedUser()) { + return PhabricatorPolicies::POLICY_ADMIN; + } + + $user_phid = $this->getUserPHID(); + if ($user_phid) { + return $user_phid; + } + + return PhabricatorPolicies::POLICY_ADMIN; + } + } + + public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { + if ($this->hasManagedUser()) { + if ($viewer->getIsAdmin()) { + return true; + } + } + + return false; + } + + public function describeAutomaticCapability($capability) { + return null; + } + + +/* -( PhabricatorDestructibleInterface )----------------------------------- */ + + + public function destroyObjectPermanently( + PhabricatorDestructionEngine $engine) { + $this->delete(); + } + + +/* -( PhabricatorApplicationTransactionInterface )------------------------- */ + + + public function getApplicationTransactionEditor() { + // TODO: Implement. + throw new PhutilMethodNotImplementedException(); + } + + public function getApplicationTransactionObject() { + return $this; + } + + public function getApplicationTransactionTemplate() { + return new PhabricatorUserPreferencesTransaction(); + } + + public function willRenderTimeline( + PhabricatorApplicationTransactionView $timeline, + AphrontRequest $request) { + return $timeline; + } + } diff --git a/src/applications/settings/storage/PhabricatorUserPreferencesTransaction.php b/src/applications/settings/storage/PhabricatorUserPreferencesTransaction.php new file mode 100644 index 0000000000..2ffe86d85c --- /dev/null +++ b/src/applications/settings/storage/PhabricatorUserPreferencesTransaction.php @@ -0,0 +1,18 @@ + Date: Tue, 31 May 2016 20:25:28 +0000 Subject: [PATCH 09/24] Lighten up red and green in diffs Summary: Because new+old stack, these colors were darker than intended. Lightening them up a little bit. Test Plan: Review a diff with new + old code and addtion subtractions. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D15994 --- resources/celerity/map.php | 12 ++++++------ .../css/application/differential/changeset-view.css | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 6381ac0a3c..a7b2065b79 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'core.pkg.css' => '8aeacc63', 'core.pkg.js' => '3f15fa62', 'darkconsole.pkg.js' => 'e7393ebb', - 'differential.pkg.css' => '33da0633', + 'differential.pkg.css' => 'a3a7e5df', 'differential.pkg.js' => '4b7d8f19', 'diffusion.pkg.css' => '91c5d3a6', 'diffusion.pkg.js' => '3a9a8bfa', @@ -57,7 +57,7 @@ return array( 'rsrc/css/application/dashboard/dashboard.css' => 'bc6f2127', 'rsrc/css/application/diff/inline-comment-summary.css' => '51efda3a', 'rsrc/css/application/differential/add-comment.css' => 'c47f8c40', - 'rsrc/css/application/differential/changeset-view.css' => '7bcbe615', + 'rsrc/css/application/differential/changeset-view.css' => 'febd2372', 'rsrc/css/application/differential/core.css' => '5b7b8ff4', 'rsrc/css/application/differential/phui-inline-comment.css' => '5953c28e', 'rsrc/css/application/differential/revision-comment.css' => '14b8565a', @@ -552,7 +552,7 @@ return array( 'conpherence-update-css' => 'faf6be09', 'conpherence-widget-pane-css' => '775eaaba', 'd3' => 'a11a5ff2', - 'differential-changeset-view-css' => '7bcbe615', + 'differential-changeset-view-css' => 'febd2372', 'differential-core-view-css' => '5b7b8ff4', 'differential-inline-comment-editor' => '64a5550f', 'differential-revision-add-comment-css' => 'c47f8c40', @@ -1513,9 +1513,6 @@ return array( 'javelin-stratcom', 'javelin-util', ), - '7bcbe615' => array( - 'phui-inline-comment-view-css', - ), '7cbe244b' => array( 'javelin-install', 'javelin-util', @@ -2203,6 +2200,9 @@ return array( 'fea0eb47' => array( 'javelin-install', ), + 'febd2372' => array( + 'phui-inline-comment-view-css', + ), ), 'packages' => array( 'core.pkg.css' => array( diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index e97841f3a3..4c3d9f415d 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -102,7 +102,7 @@ } .differential-diff td.old { - background: rgba(251, 175, 175, 0.3); + background: rgba(251, 175, 175, .3); } .differential-diff td.new { @@ -119,12 +119,12 @@ .differential-diff td.old-full, .differential-diff td.old span.bright { - background: rgba(251, 175, 175, 0.8); + background: rgba(251, 175, 175, .7); } .differential-diff td.new-full, .differential-diff td.new span.bright { - background: rgba(151, 234, 151, .8); + background: rgba(151, 234, 151, .6); } .differential-diff td.copy { From 5e6716399cd6e8448609aa58dde19baae7913ff4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 27 May 2016 05:53:30 -0700 Subject: [PATCH 10/24] Make Settings modular and allow them to be EditEngine'd Summary: Ref T4103. This starts breaking out settings in a modern way to prepare for global defaults. Test Plan: - Edited diff settings. - Saw them take effect in primary settings pane. - Set stuff to new automatic defaults. - Tried to edit another user's settings. - Edited a bot's settings as an administrator. {F1669077} Reviewers: chad Reviewed By: chad Maniphest Tasks: T4103 Differential Revision: https://secure.phabricator.com/D15995 --- src/__phutil_library_map__.php | 16 +++ .../people/storage/PhabricatorUser.php | 1 + .../PhabricatorSettingsApplication.php | 2 + .../PhabricatorSettingsEditController.php | 35 +++++ .../editor/PhabricatorSettingsEditEngine.php | 91 ++++++++++++ .../PhabricatorUserPreferencesEditor.php | 132 ++++++++++++++++++ .../PhabricatorUserPreferencesPHIDType.php | 1 - .../PhabricatorOlderInlinesSetting.php | 35 +++++ .../setting/PhabricatorSelectSetting.php | 55 ++++++++ .../settings/setting/PhabricatorSetting.php | 96 +++++++++++++ .../PhabricatorShowFiletreeSetting.php | 34 +++++ .../PhabricatorUnifiedDiffsSetting.php | 35 +++++ .../storage/PhabricatorUserPreferences.php | 18 ++- .../PhabricatorUserPreferencesTransaction.php | 4 + .../form/control/AphrontFormSelectControl.php | 15 +- 15 files changed, 566 insertions(+), 4 deletions(-) create mode 100644 src/applications/settings/controller/PhabricatorSettingsEditController.php create mode 100644 src/applications/settings/editor/PhabricatorSettingsEditEngine.php create mode 100644 src/applications/settings/editor/PhabricatorUserPreferencesEditor.php create mode 100644 src/applications/settings/setting/PhabricatorOlderInlinesSetting.php create mode 100644 src/applications/settings/setting/PhabricatorSelectSetting.php create mode 100644 src/applications/settings/setting/PhabricatorSetting.php create mode 100644 src/applications/settings/setting/PhabricatorShowFiletreeSetting.php create mode 100644 src/applications/settings/setting/PhabricatorUnifiedDiffsSetting.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f3f92e6998..0980e48611 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2822,6 +2822,7 @@ phutil_register_library_map(array( 'PhabricatorObjectSelectorDialog' => 'view/control/PhabricatorObjectSelectorDialog.php', 'PhabricatorOffsetPagedQuery' => 'infrastructure/query/PhabricatorOffsetPagedQuery.php', 'PhabricatorOldWorldContentSource' => 'infrastructure/contentsource/PhabricatorOldWorldContentSource.php', + 'PhabricatorOlderInlinesSetting' => 'applications/settings/setting/PhabricatorOlderInlinesSetting.php', 'PhabricatorOneTimeTriggerClock' => 'infrastructure/daemon/workers/clock/PhabricatorOneTimeTriggerClock.php', 'PhabricatorOpcodeCacheSpec' => 'applications/cache/spec/PhabricatorOpcodeCacheSpec.php', 'PhabricatorOwnerPathQuery' => 'applications/owners/query/PhabricatorOwnerPathQuery.php', @@ -3348,11 +3349,15 @@ phutil_register_library_map(array( 'PhabricatorSecurityConfigOptions' => 'applications/config/option/PhabricatorSecurityConfigOptions.php', 'PhabricatorSecuritySetupCheck' => 'applications/config/check/PhabricatorSecuritySetupCheck.php', 'PhabricatorSelectEditField' => 'applications/transactions/editfield/PhabricatorSelectEditField.php', + 'PhabricatorSelectSetting' => 'applications/settings/setting/PhabricatorSelectSetting.php', 'PhabricatorSendGridConfigOptions' => 'applications/config/option/PhabricatorSendGridConfigOptions.php', 'PhabricatorSessionsSettingsPanel' => 'applications/settings/panel/PhabricatorSessionsSettingsPanel.php', + 'PhabricatorSetting' => 'applications/settings/setting/PhabricatorSetting.php', 'PhabricatorSettingsAddEmailAction' => 'applications/settings/action/PhabricatorSettingsAddEmailAction.php', 'PhabricatorSettingsAdjustController' => 'applications/settings/controller/PhabricatorSettingsAdjustController.php', 'PhabricatorSettingsApplication' => 'applications/settings/application/PhabricatorSettingsApplication.php', + 'PhabricatorSettingsEditController' => 'applications/settings/controller/PhabricatorSettingsEditController.php', + 'PhabricatorSettingsEditEngine' => 'applications/settings/editor/PhabricatorSettingsEditEngine.php', 'PhabricatorSettingsMainController' => 'applications/settings/controller/PhabricatorSettingsMainController.php', 'PhabricatorSettingsMainMenuBarExtension' => 'applications/settings/extension/PhabricatorSettingsMainMenuBarExtension.php', 'PhabricatorSettingsPanel' => 'applications/settings/panel/PhabricatorSettingsPanel.php', @@ -3363,6 +3368,7 @@ phutil_register_library_map(array( 'PhabricatorSetupIssueUIExample' => 'applications/uiexample/examples/PhabricatorSetupIssueUIExample.php', 'PhabricatorSetupIssueView' => 'applications/config/view/PhabricatorSetupIssueView.php', 'PhabricatorShortSite' => 'aphront/site/PhabricatorShortSite.php', + 'PhabricatorShowFiletreeSetting' => 'applications/settings/setting/PhabricatorShowFiletreeSetting.php', 'PhabricatorSimpleEditType' => 'applications/transactions/edittype/PhabricatorSimpleEditType.php', 'PhabricatorSite' => 'aphront/site/PhabricatorSite.php', 'PhabricatorSlowvoteApplication' => 'applications/slowvote/application/PhabricatorSlowvoteApplication.php', @@ -3568,6 +3574,7 @@ phutil_register_library_map(array( 'PhabricatorUIExampleRenderController' => 'applications/uiexample/controller/PhabricatorUIExampleRenderController.php', 'PhabricatorUIExamplesApplication' => 'applications/uiexample/application/PhabricatorUIExamplesApplication.php', 'PhabricatorUSEnglishTranslation' => 'infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php', + 'PhabricatorUnifiedDiffsSetting' => 'applications/settings/setting/PhabricatorUnifiedDiffsSetting.php', 'PhabricatorUnitTestContentSource' => 'infrastructure/contentsource/PhabricatorUnitTestContentSource.php', 'PhabricatorUnitsTestCase' => 'view/__tests__/PhabricatorUnitsTestCase.php', 'PhabricatorUnknownContentSource' => 'infrastructure/contentsource/PhabricatorUnknownContentSource.php', @@ -3592,6 +3599,7 @@ phutil_register_library_map(array( 'PhabricatorUserLogView' => 'applications/people/view/PhabricatorUserLogView.php', 'PhabricatorUserPHIDResolver' => 'applications/phid/resolver/PhabricatorUserPHIDResolver.php', 'PhabricatorUserPreferences' => 'applications/settings/storage/PhabricatorUserPreferences.php', + 'PhabricatorUserPreferencesEditor' => 'applications/settings/editor/PhabricatorUserPreferencesEditor.php', 'PhabricatorUserPreferencesPHIDType' => 'applications/settings/phid/PhabricatorUserPreferencesPHIDType.php', 'PhabricatorUserPreferencesQuery' => 'applications/settings/query/PhabricatorUserPreferencesQuery.php', 'PhabricatorUserPreferencesTransaction' => 'applications/settings/storage/PhabricatorUserPreferencesTransaction.php', @@ -7413,6 +7421,7 @@ phutil_register_library_map(array( 'PhabricatorObjectSelectorDialog' => 'Phobject', 'PhabricatorOffsetPagedQuery' => 'PhabricatorQuery', 'PhabricatorOldWorldContentSource' => 'PhabricatorContentSource', + 'PhabricatorOlderInlinesSetting' => 'PhabricatorSelectSetting', 'PhabricatorOneTimeTriggerClock' => 'PhabricatorTriggerClock', 'PhabricatorOpcodeCacheSpec' => 'PhabricatorCacheSpec', 'PhabricatorOwnerPathQuery' => 'Phobject', @@ -8064,11 +8073,15 @@ phutil_register_library_map(array( 'PhabricatorSecurityConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorSecuritySetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorSelectEditField' => 'PhabricatorEditField', + 'PhabricatorSelectSetting' => 'PhabricatorSetting', 'PhabricatorSendGridConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorSessionsSettingsPanel' => 'PhabricatorSettingsPanel', + 'PhabricatorSetting' => 'Phobject', 'PhabricatorSettingsAddEmailAction' => 'PhabricatorSystemAction', 'PhabricatorSettingsAdjustController' => 'PhabricatorController', 'PhabricatorSettingsApplication' => 'PhabricatorApplication', + 'PhabricatorSettingsEditController' => 'PhabricatorController', + 'PhabricatorSettingsEditEngine' => 'PhabricatorEditEngine', 'PhabricatorSettingsMainController' => 'PhabricatorController', 'PhabricatorSettingsMainMenuBarExtension' => 'PhabricatorMainMenuBarExtension', 'PhabricatorSettingsPanel' => 'Phobject', @@ -8079,6 +8092,7 @@ phutil_register_library_map(array( 'PhabricatorSetupIssueUIExample' => 'PhabricatorUIExample', 'PhabricatorSetupIssueView' => 'AphrontView', 'PhabricatorShortSite' => 'PhabricatorSite', + 'PhabricatorShowFiletreeSetting' => 'PhabricatorSelectSetting', 'PhabricatorSimpleEditType' => 'PhabricatorEditType', 'PhabricatorSite' => 'AphrontSite', 'PhabricatorSlowvoteApplication' => 'PhabricatorApplication', @@ -8305,6 +8319,7 @@ phutil_register_library_map(array( 'PhabricatorUIExampleRenderController' => 'PhabricatorController', 'PhabricatorUIExamplesApplication' => 'PhabricatorApplication', 'PhabricatorUSEnglishTranslation' => 'PhutilTranslation', + 'PhabricatorUnifiedDiffsSetting' => 'PhabricatorSelectSetting', 'PhabricatorUnitTestContentSource' => 'PhabricatorContentSource', 'PhabricatorUnitsTestCase' => 'PhabricatorTestCase', 'PhabricatorUnknownContentSource' => 'PhabricatorContentSource', @@ -8351,6 +8366,7 @@ phutil_register_library_map(array( 'PhabricatorDestructibleInterface', 'PhabricatorApplicationTransactionInterface', ), + 'PhabricatorUserPreferencesEditor' => 'AlmanacEditor', 'PhabricatorUserPreferencesPHIDType' => 'PhabricatorPHIDType', 'PhabricatorUserPreferencesQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorUserPreferencesTransaction' => 'PhabricatorApplicationTransaction', diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 855392fc62..9a65c44470 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -503,6 +503,7 @@ final class PhabricatorUser if (!$preferences) { $preferences = new PhabricatorUserPreferences(); $preferences->setUserPHID($this->getPHID()); + $preferences->attachUser($this); $default_dict = array( PhabricatorUserPreferences::PREFERENCE_TITLES => 'glyph', diff --git a/src/applications/settings/application/PhabricatorSettingsApplication.php b/src/applications/settings/application/PhabricatorSettingsApplication.php index 66bce7205f..4dba9a2e25 100644 --- a/src/applications/settings/application/PhabricatorSettingsApplication.php +++ b/src/applications/settings/application/PhabricatorSettingsApplication.php @@ -34,6 +34,8 @@ final class PhabricatorSettingsApplication extends PhabricatorApplication { 'adjust/' => 'PhabricatorSettingsAdjustController', 'timezone/(?P[^/]+)/' => 'PhabricatorSettingsTimezoneController', + '(?Puser)/(?P[^/]+)/(?:panel/(?P[^/]+)/)?' + => 'PhabricatorSettingsEditController', ), ); } diff --git a/src/applications/settings/controller/PhabricatorSettingsEditController.php b/src/applications/settings/controller/PhabricatorSettingsEditController.php new file mode 100644 index 0000000000..aad58437db --- /dev/null +++ b/src/applications/settings/controller/PhabricatorSettingsEditController.php @@ -0,0 +1,35 @@ +getViewer(); + + $engine = id(new PhabricatorSettingsEditEngine()) + ->setController($this); + + switch ($request->getURIData('type')) { + case 'user': + $user = id(new PhabricatorPeopleQuery()) + ->setViewer($viewer) + ->withUsernames(array($request->getURIData('username'))) + ->executeOne(); + + $preferences = $user->loadPreferences(); + + PhabricatorPolicyFilter::requireCapability( + $viewer, + $preferences, + PhabricatorPolicyCapability::CAN_EDIT); + + $engine->setTargetObject($preferences); + break; + default: + return new Aphront404Response(); + } + + return $engine->buildResponse(); + } + +} diff --git a/src/applications/settings/editor/PhabricatorSettingsEditEngine.php b/src/applications/settings/editor/PhabricatorSettingsEditEngine.php new file mode 100644 index 0000000000..0db42708fb --- /dev/null +++ b/src/applications/settings/editor/PhabricatorSettingsEditEngine.php @@ -0,0 +1,91 @@ +getViewer()->getUsername().'/'; + } + + protected function getCreateNewObjectPolicy() { + return PhabricatorPolicies::POLICY_ADMIN; + } + + protected function buildCustomEditFields($object) { + $viewer = $this->getViewer(); + $settings = PhabricatorSetting::getAllEnabledSettings($viewer); + + $fields = array(); + foreach ($settings as $setting) { + foreach ($setting->newCustomEditFields($object) as $field) { + $fields[] = $field; + } + } + + return $fields; + } + +} diff --git a/src/applications/settings/editor/PhabricatorUserPreferencesEditor.php b/src/applications/settings/editor/PhabricatorUserPreferencesEditor.php new file mode 100644 index 0000000000..6be467de8f --- /dev/null +++ b/src/applications/settings/editor/PhabricatorUserPreferencesEditor.php @@ -0,0 +1,132 @@ +getMetadataValue( + PhabricatorUserPreferencesTransaction::PROPERTY_SETTING); + + switch ($xaction->getTransactionType()) { + case PhabricatorUserPreferencesTransaction::TYPE_SETTING: + return $object->getPreference($setting_key); + } + + return parent::getCustomTransactionOldValue($object, $xaction); + } + + protected function getCustomTransactionNewValue( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + $actor = $this->getActor(); + + $setting_key = $xaction->getMetadataValue( + PhabricatorUserPreferencesTransaction::PROPERTY_SETTING); + + $settings = PhabricatorSetting::getAllEnabledSettings($actor); + $setting = $settings[$setting_key]; + + switch ($xaction->getTransactionType()) { + case PhabricatorUserPreferencesTransaction::TYPE_SETTING: + $value = $xaction->getNewValue(); + $value = $setting->getTransactionNewValue($value); + return $value; + } + + return parent::getCustomTransactionNewValue($object, $xaction); + } + + protected function applyCustomInternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + $setting_key = $xaction->getMetadataValue( + PhabricatorUserPreferencesTransaction::PROPERTY_SETTING); + + switch ($xaction->getTransactionType()) { + case PhabricatorUserPreferencesTransaction::TYPE_SETTING: + $new_value = $xaction->getNewValue(); + if ($new_value === null) { + $object->unsetPreference($setting_key); + } else { + $object->setPreference($setting_key, $new_value); + } + return; + } + + return parent::applyCustomInternalTransaction($object, $xaction); + } + + protected function applyCustomExternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case PhabricatorUserPreferencesTransaction::TYPE_SETTING: + return; + } + + return parent::applyCustomExternalTransaction($object, $xaction); + } + + protected function validateTransaction( + PhabricatorLiskDAO $object, + $type, + array $xactions) { + + $errors = parent::validateTransaction($object, $type, $xactions); + + $actor = $this->getActor(); + $settings = PhabricatorSetting::getAllEnabledSettings($actor); + + switch ($type) { + case PhabricatorUserPreferencesTransaction::TYPE_SETTING: + foreach ($xactions as $xaction) { + $setting_key = $xaction->getMetadataValue( + PhabricatorUserPreferencesTransaction::PROPERTY_SETTING); + + $setting = idx($settings, $setting_key); + if (!$setting) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht( + 'There is no known application setting with key "%s".', + $setting_key), + $xaction); + continue; + } + + try { + $setting->validateTransactionValue($xaction->getNewValue()); + } catch (Exception $ex) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + $ex->getMessage(), + $xaction); + } + } + break; + } + + return $errors; + } + +} diff --git a/src/applications/settings/phid/PhabricatorUserPreferencesPHIDType.php b/src/applications/settings/phid/PhabricatorUserPreferencesPHIDType.php index 8bd341b8c6..7022c56c33 100644 --- a/src/applications/settings/phid/PhabricatorUserPreferencesPHIDType.php +++ b/src/applications/settings/phid/PhabricatorUserPreferencesPHIDType.php @@ -32,7 +32,6 @@ final class PhabricatorUserPreferencesPHIDType extends PhabricatorPHIDType { $viewer = $query->getViewer(); foreach ($handles as $phid => $handle) { $preferences = $objects[$phid]; - $handle->setName(pht('Settings %d', $preferences->getID())); } } diff --git a/src/applications/settings/setting/PhabricatorOlderInlinesSetting.php b/src/applications/settings/setting/PhabricatorOlderInlinesSetting.php new file mode 100644 index 0000000000..e9dcfbbc2c --- /dev/null +++ b/src/applications/settings/setting/PhabricatorOlderInlinesSetting.php @@ -0,0 +1,35 @@ + pht('Enabled'), + self::VALUE_GHOST_INLINES_DISABLED => pht('Disabled'), + ); + } + + +} diff --git a/src/applications/settings/setting/PhabricatorSelectSetting.php b/src/applications/settings/setting/PhabricatorSelectSetting.php new file mode 100644 index 0000000000..5dc7323c84 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorSelectSetting.php @@ -0,0 +1,55 @@ +getSettingKey(); + $default_value = $object->getDefaultValue($setting_key); + + $options = $this->getSelectOptions(); + + if (isset($options[$default_value])) { + $default_label = pht('Default (%s)', $options[$default_value]); + } else { + $default_label = pht('Default (Unknown, "%s")', $default_value); + } + + $options = array( + '' => $default_label, + ) + $options; + + return $this->newEditField($object, new PhabricatorSelectEditField()) + ->setOptions($options); + } + + final public function validateTransactionValue($value) { + if (!strlen($value)) { + return; + } + + $options = $this->getSelectOptions(); + + if (!isset($options[$value])) { + throw new Exception( + pht( + 'Value "%s" is not valid for setting "%s": valid values are %s.', + $value, + $this->getSettingName(), + implode(', ', array_keys($options)))); + } + + return; + } + + public function getTransactionNewValue($value) { + if (!strlen($value)) { + return null; + } + + return (string)$value; + } + +} diff --git a/src/applications/settings/setting/PhabricatorSetting.php b/src/applications/settings/setting/PhabricatorSetting.php new file mode 100644 index 0000000000..c267e1aca6 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorSetting.php @@ -0,0 +1,96 @@ +viewer = $viewer; + return $this; + } + + public function getViewer() { + return $this->viewer; + } + + abstract public function getSettingName(); + + protected function getControlInstructions() { + return null; + } + + protected function isEnabledForViewer(PhabricatorUser $viewer) { + return true; + } + + public function getSettingDefaultValue() { + return null; + } + + final public function getSettingKey() { + return $this->getPhobjectClassConstant('SETTINGKEY'); + } + + public static function getAllSettings() { + return id(new PhutilClassMapQuery()) + ->setAncestorClass(__CLASS__) + ->setUniqueMethod('getSettingKey') + ->execute(); + } + + public static function getAllEnabledSettings(PhabricatorUser $viewer) { + $settings = self::getAllSettings(); + foreach ($settings as $key => $setting) { + if (!$setting->isEnabledForViewer($viewer)) { + unset($settings[$key]); + } + } + return $settings; + } + + final public function newCustomEditFields($object) { + $fields = array(); + + $field = $this->newCustomEditField($object); + if ($field) { + $fields[] = $field; + } + + return $fields; + } + + protected function newCustomEditField($object) { + return null; + } + + protected function newEditField($object, PhabricatorEditField $template) { + $setting_property = PhabricatorUserPreferencesTransaction::PROPERTY_SETTING; + $setting_key = $this->getSettingKey(); + $value = $object->getPreference($setting_key); + $xaction_type = PhabricatorUserPreferencesTransaction::TYPE_SETTING; + $label = $this->getSettingName(); + + $template + ->setKey($setting_key) + ->setLabel($label) + ->setValue($value) + ->setTransactionType($xaction_type) + ->setMetadataValue($setting_property, $setting_key); + + $instructions = $this->getControlInstructions(); + if (strlen($instructions)) { + $template->setControlInstructions($instructions); + } + + return $template; + } + + public function validateTransactionValue($value) { + return; + } + + public function getTransactionNewValue($value) { + return $value; + } + +} diff --git a/src/applications/settings/setting/PhabricatorShowFiletreeSetting.php b/src/applications/settings/setting/PhabricatorShowFiletreeSetting.php new file mode 100644 index 0000000000..0721a1c637 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorShowFiletreeSetting.php @@ -0,0 +1,34 @@ + pht('Disable Filetree'), + self::VALUE_ENABLE_FILETREE => pht('Enable Filetree'), + ); + } + +} diff --git a/src/applications/settings/setting/PhabricatorUnifiedDiffsSetting.php b/src/applications/settings/setting/PhabricatorUnifiedDiffsSetting.php new file mode 100644 index 0000000000..96c00bb170 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorUnifiedDiffsSetting.php @@ -0,0 +1,35 @@ + pht('On Small Screens'), + self::VALUE_ALWAYS_UNIFIED => pht('Always'), + ); + } + + +} diff --git a/src/applications/settings/storage/PhabricatorUserPreferences.php b/src/applications/settings/storage/PhabricatorUserPreferences.php index f90f82c4aa..7805daf771 100644 --- a/src/applications/settings/storage/PhabricatorUserPreferences.php +++ b/src/applications/settings/storage/PhabricatorUserPreferences.php @@ -92,6 +92,21 @@ final class PhabricatorUserPreferences return $this; } + public function getDefaultValue($key) { + $setting = self::getSettingObject($key); + + if (!$setting) { + return null; + } + + return $setting->getSettingDefaultValue(); + } + + private static function getSettingObject($key) { + $settings = PhabricatorSetting::getAllSettings(); + return idx($settings, $key); + } + public function getPinnedApplications(array $apps, PhabricatorUser $viewer) { $pref_pinned = self::PREFERENCE_APP_PINNED; $pinned = $this->getPreference($pref_pinned); @@ -212,8 +227,7 @@ final class PhabricatorUserPreferences public function getApplicationTransactionEditor() { - // TODO: Implement. - throw new PhutilMethodNotImplementedException(); + return new PhabricatorUserPreferencesEditor(); } public function getApplicationTransactionObject() { diff --git a/src/applications/settings/storage/PhabricatorUserPreferencesTransaction.php b/src/applications/settings/storage/PhabricatorUserPreferencesTransaction.php index 2ffe86d85c..6378ee29d3 100644 --- a/src/applications/settings/storage/PhabricatorUserPreferencesTransaction.php +++ b/src/applications/settings/storage/PhabricatorUserPreferencesTransaction.php @@ -3,6 +3,10 @@ final class PhabricatorUserPreferencesTransaction extends PhabricatorApplicationTransaction { + const TYPE_SETTING = 'setting'; + + const PROPERTY_SETTING = 'setting.key'; + public function getApplicationName() { return 'user'; } diff --git a/src/view/form/control/AphrontFormSelectControl.php b/src/view/form/control/AphrontFormSelectControl.php index 17766ab435..8799fcfd91 100644 --- a/src/view/form/control/AphrontFormSelectControl.php +++ b/src/view/form/control/AphrontFormSelectControl.php @@ -56,6 +56,7 @@ final class AphrontFormSelectControl extends AphrontFormControl { $disabled = array_fuse($disabled); $tags = array(); + $already_selected = false; foreach ($options as $value => $thing) { if (is_array($thing)) { $tags[] = phutil_tag( @@ -65,10 +66,22 @@ final class AphrontFormSelectControl extends AphrontFormControl { ), self::renderOptions($selected, $thing)); } else { + // When there are a list of options including similar values like + // "0" and "" (the empty string), only select the first matching + // value. Ideally this should be more precise about matching, but we + // have 2,000 of these controls at this point so hold that for a + // broader rewrite. + if (!$already_selected && ($value == $selected)) { + $is_selected = 'selected'; + $already_selected = true; + } else { + $is_selected = null; + } + $tags[] = phutil_tag( 'option', array( - 'selected' => ($value == $selected) ? 'selected' : null, + 'selected' => $is_selected, 'value' => $value, 'disabled' => isset($disabled[$value]) ? 'disabled' : null, ), From ba505c03f92b7224a953f150d01dbd9246e51205 Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Tue, 31 May 2016 23:36:15 +0000 Subject: [PATCH 11/24] Fix typo in link to docs Test Plan: click new link, get to the right page Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D15996 --- .../search/engine/PhabricatorSearchEngineAPIMethod.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php b/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php index 7757e27d68..d050e48bff 100644 --- a/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php +++ b/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php @@ -68,7 +68,7 @@ abstract class PhabricatorSearchEngineAPIMethod 'This is a standard **ApplicationSearch** method which will let you '. 'list, query, or search for objects. For documentation on these '. 'endpoints, see **[[ %s | Conduit API: Using Search Endpoints ]]**.', - PhabricatorEnv::getDoclink('Conduit API: Using Edit Endpoints')); + PhabricatorEnv::getDoclink('Conduit API: Using Search Endpoints')); } final public function getMethodDocumentation() { From 9b27b5c7dab37503c63c2a5c61e2178d038367e7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 31 May 2016 16:06:18 -0700 Subject: [PATCH 12/24] Convert "Display Preferences" to modular settings Summary: Ref T4103. Just porting these directly for now, no attempt to organize things yet. Test Plan: {F1669263} Reviewers: chad Reviewed By: chad Maniphest Tasks: T4103 Differential Revision: https://secure.phabricator.com/D15997 --- src/__phutil_library_map__.php | 14 +++++++ .../CelerityDefaultPostprocessor.php | 2 +- .../PhabricatorAccessibilitySetting.php | 39 ++++++++++++++++++ .../PhabricatorEditorMultipleSetting.php | 32 +++++++++++++++ .../setting/PhabricatorEditorSetting.php | 33 +++++++++++++++ .../PhabricatorMonospacedFontSetting.php | 40 +++++++++++++++++++ .../PhabricatorMonospacedTextareasSetting.php | 34 ++++++++++++++++ .../setting/PhabricatorStringSetting.php | 18 +++++++++ .../setting/PhabricatorTitleGlyphsSetting.php | 33 +++++++++++++++ src/view/page/PhabricatorStandardPageView.php | 2 +- 10 files changed, 245 insertions(+), 2 deletions(-) create mode 100644 src/applications/settings/setting/PhabricatorAccessibilitySetting.php create mode 100644 src/applications/settings/setting/PhabricatorEditorMultipleSetting.php create mode 100644 src/applications/settings/setting/PhabricatorEditorSetting.php create mode 100644 src/applications/settings/setting/PhabricatorMonospacedFontSetting.php create mode 100644 src/applications/settings/setting/PhabricatorMonospacedTextareasSetting.php create mode 100644 src/applications/settings/setting/PhabricatorStringSetting.php create mode 100644 src/applications/settings/setting/PhabricatorTitleGlyphsSetting.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0980e48611..df5588479d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1711,6 +1711,7 @@ phutil_register_library_map(array( 'PhabricatorAccessControlTestCase' => 'applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php', 'PhabricatorAccessLog' => 'infrastructure/log/PhabricatorAccessLog.php', 'PhabricatorAccessLogConfigOptions' => 'applications/config/option/PhabricatorAccessLogConfigOptions.php', + 'PhabricatorAccessibilitySetting' => 'applications/settings/setting/PhabricatorAccessibilitySetting.php', 'PhabricatorAccountSettingsPanel' => 'applications/settings/panel/PhabricatorAccountSettingsPanel.php', 'PhabricatorActionListView' => 'view/layout/PhabricatorActionListView.php', 'PhabricatorActionView' => 'view/layout/PhabricatorActionView.php', @@ -2373,6 +2374,8 @@ phutil_register_library_map(array( 'PhabricatorEditPage' => 'applications/transactions/editengine/PhabricatorEditPage.php', 'PhabricatorEditType' => 'applications/transactions/edittype/PhabricatorEditType.php', 'PhabricatorEditor' => 'infrastructure/PhabricatorEditor.php', + 'PhabricatorEditorMultipleSetting' => 'applications/settings/setting/PhabricatorEditorMultipleSetting.php', + 'PhabricatorEditorSetting' => 'applications/settings/setting/PhabricatorEditorSetting.php', 'PhabricatorElasticFulltextStorageEngine' => 'applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php', 'PhabricatorElasticSearchSetupCheck' => 'applications/config/check/PhabricatorElasticSearchSetupCheck.php', 'PhabricatorEmailAddressesSettingsPanel' => 'applications/settings/panel/PhabricatorEmailAddressesSettingsPanel.php', @@ -2732,6 +2735,8 @@ phutil_register_library_map(array( 'PhabricatorMetaMTASendGridReceiveController' => 'applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php', 'PhabricatorMetaMTAWorker' => 'applications/metamta/PhabricatorMetaMTAWorker.php', 'PhabricatorMetronomicTriggerClock' => 'infrastructure/daemon/workers/clock/PhabricatorMetronomicTriggerClock.php', + 'PhabricatorMonospacedFontSetting' => 'applications/settings/setting/PhabricatorMonospacedFontSetting.php', + 'PhabricatorMonospacedTextareasSetting' => 'applications/settings/setting/PhabricatorMonospacedTextareasSetting.php', 'PhabricatorMotivatorProfilePanel' => 'applications/search/profilepanel/PhabricatorMotivatorProfilePanel.php', 'PhabricatorMultiColumnUIExample' => 'applications/uiexample/examples/PhabricatorMultiColumnUIExample.php', 'PhabricatorMultiFactorSettingsPanel' => 'applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php', @@ -3463,6 +3468,7 @@ phutil_register_library_map(array( 'PhabricatorStorageSetupCheck' => 'applications/config/check/PhabricatorStorageSetupCheck.php', 'PhabricatorStreamingProtocolAdapter' => 'infrastructure/daemon/bot/adapter/PhabricatorStreamingProtocolAdapter.php', 'PhabricatorStringListEditField' => 'applications/transactions/editfield/PhabricatorStringListEditField.php', + 'PhabricatorStringSetting' => 'applications/settings/setting/PhabricatorStringSetting.php', 'PhabricatorSubscribableInterface' => 'applications/subscriptions/interface/PhabricatorSubscribableInterface.php', 'PhabricatorSubscribedToObjectEdgeType' => 'applications/transactions/edges/PhabricatorSubscribedToObjectEdgeType.php', 'PhabricatorSubscribersEditField' => 'applications/transactions/editfield/PhabricatorSubscribersEditField.php', @@ -3522,6 +3528,7 @@ phutil_register_library_map(array( 'PhabricatorTimeGuard' => 'infrastructure/time/PhabricatorTimeGuard.php', 'PhabricatorTimeTestCase' => 'infrastructure/time/__tests__/PhabricatorTimeTestCase.php', 'PhabricatorTimezoneSetupCheck' => 'applications/config/check/PhabricatorTimezoneSetupCheck.php', + 'PhabricatorTitleGlyphsSetting' => 'applications/settings/setting/PhabricatorTitleGlyphsSetting.php', 'PhabricatorToken' => 'applications/tokens/storage/PhabricatorToken.php', 'PhabricatorTokenController' => 'applications/tokens/controller/PhabricatorTokenController.php', 'PhabricatorTokenCount' => 'applications/tokens/storage/PhabricatorTokenCount.php', @@ -6137,6 +6144,7 @@ phutil_register_library_map(array( 'PhabricatorAccessControlTestCase' => 'PhabricatorTestCase', 'PhabricatorAccessLog' => 'Phobject', 'PhabricatorAccessLogConfigOptions' => 'PhabricatorApplicationConfigOptions', + 'PhabricatorAccessibilitySetting' => 'PhabricatorSelectSetting', 'PhabricatorAccountSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorActionListView' => 'AphrontView', 'PhabricatorActionView' => 'AphrontView', @@ -6914,6 +6922,8 @@ phutil_register_library_map(array( 'PhabricatorEditPage' => 'Phobject', 'PhabricatorEditType' => 'Phobject', 'PhabricatorEditor' => 'Phobject', + 'PhabricatorEditorMultipleSetting' => 'PhabricatorSelectSetting', + 'PhabricatorEditorSetting' => 'PhabricatorStringSetting', 'PhabricatorElasticFulltextStorageEngine' => 'PhabricatorFulltextStorageEngine', 'PhabricatorElasticSearchSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorEmailAddressesSettingsPanel' => 'PhabricatorSettingsPanel', @@ -7318,6 +7328,8 @@ phutil_register_library_map(array( 'PhabricatorMetaMTASendGridReceiveController' => 'PhabricatorMetaMTAController', 'PhabricatorMetaMTAWorker' => 'PhabricatorWorker', 'PhabricatorMetronomicTriggerClock' => 'PhabricatorTriggerClock', + 'PhabricatorMonospacedFontSetting' => 'PhabricatorStringSetting', + 'PhabricatorMonospacedTextareasSetting' => 'PhabricatorSelectSetting', 'PhabricatorMotivatorProfilePanel' => 'PhabricatorProfilePanel', 'PhabricatorMultiColumnUIExample' => 'PhabricatorUIExample', 'PhabricatorMultiFactorSettingsPanel' => 'PhabricatorSettingsPanel', @@ -8204,6 +8216,7 @@ phutil_register_library_map(array( 'PhabricatorStorageSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorStreamingProtocolAdapter' => 'PhabricatorProtocolAdapter', 'PhabricatorStringListEditField' => 'PhabricatorEditField', + 'PhabricatorStringSetting' => 'PhabricatorSetting', 'PhabricatorSubscribedToObjectEdgeType' => 'PhabricatorEdgeType', 'PhabricatorSubscribersEditField' => 'PhabricatorTokenizerEditField', 'PhabricatorSubscribersQuery' => 'PhabricatorQuery', @@ -8262,6 +8275,7 @@ phutil_register_library_map(array( 'PhabricatorTimeGuard' => 'Phobject', 'PhabricatorTimeTestCase' => 'PhabricatorTestCase', 'PhabricatorTimezoneSetupCheck' => 'PhabricatorSetupCheck', + 'PhabricatorTitleGlyphsSetting' => 'PhabricatorSelectSetting', 'PhabricatorToken' => array( 'PhabricatorTokenDAO', 'PhabricatorPolicyInterface', diff --git a/src/applications/celerity/postprocessor/CelerityDefaultPostprocessor.php b/src/applications/celerity/postprocessor/CelerityDefaultPostprocessor.php index 7d87d28ff2..7eb3b248c1 100644 --- a/src/applications/celerity/postprocessor/CelerityDefaultPostprocessor.php +++ b/src/applications/celerity/postprocessor/CelerityDefaultPostprocessor.php @@ -10,7 +10,7 @@ final class CelerityDefaultPostprocessor } public function getPostprocessorName() { - return pht('Use Default Colors'); + return pht('Use Standard Colors'); } public function buildDefaultPostprocessor() { diff --git a/src/applications/settings/setting/PhabricatorAccessibilitySetting.php b/src/applications/settings/setting/PhabricatorAccessibilitySetting.php new file mode 100644 index 0000000000..a1c273dfd7 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorAccessibilitySetting.php @@ -0,0 +1,39 @@ + pht('Supported, Separated by Spaces'), + self::VALUE_SINGLE => pht('Not Supported'), + ); + } + +} diff --git a/src/applications/settings/setting/PhabricatorEditorSetting.php b/src/applications/settings/setting/PhabricatorEditorSetting.php new file mode 100644 index 0000000000..d0059a69c6 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorEditorSetting.php @@ -0,0 +1,33 @@ + pht('Use Variable-Width Font'), + self::VALUE_TEXT_MONOSPACED => pht('Use Monospaced Font'), + ); + } + + +} diff --git a/src/applications/settings/setting/PhabricatorStringSetting.php b/src/applications/settings/setting/PhabricatorStringSetting.php new file mode 100644 index 0000000000..1d4d226ba5 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorStringSetting.php @@ -0,0 +1,18 @@ +newEditField($object, new PhabricatorTextEditField()); + } + + public function getTransactionNewValue($value) { + if (!strlen($value)) { + return null; + } + + return (string)$value; + } + +} diff --git a/src/applications/settings/setting/PhabricatorTitleGlyphsSetting.php b/src/applications/settings/setting/PhabricatorTitleGlyphsSetting.php new file mode 100644 index 0000000000..f2f0b6ec8d --- /dev/null +++ b/src/applications/settings/setting/PhabricatorTitleGlyphsSetting.php @@ -0,0 +1,33 @@ + pht("Use Unicode Glyphs: \xE2\x9A\x99"), + self::VALUE_TITLE_TEXT => pht('Use Plain Text: [Differential]'), + ); + } + +} diff --git a/src/view/page/PhabricatorStandardPageView.php b/src/view/page/PhabricatorStandardPageView.php index 25779dad98..e859163272 100644 --- a/src/view/page/PhabricatorStandardPageView.php +++ b/src/view/page/PhabricatorStandardPageView.php @@ -374,7 +374,7 @@ final class PhabricatorStandardPageView extends PhabricatorBarePageView // We can't print this normally because escaping quotation marks will // break the CSS. Instead, filter it strictly and then mark it as safe. $monospaced = new PhutilSafeHTML( - PhabricatorUserPreferences::filterMonospacedCSSRule( + PhabricatorMonospacedFontSetting::filterMonospacedCSSRule( $monospaced)); $font_css = hsprintf( From 4458fb6f8f33ca7c1dc1a0ffd782cd1aac2bcdf0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 1 Jun 2016 04:52:38 -0700 Subject: [PATCH 13/24] Correct tooltip label for open audit count Summary: Fixes T11071. This was a copy/paste error from D14638. Test Plan: {F1669989} Reviewers: chad Reviewed By: chad Maniphest Tasks: T11071 Differential Revision: https://secure.phabricator.com/D15998 --- .../audit/application/PhabricatorAuditApplication.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/applications/audit/application/PhabricatorAuditApplication.php b/src/applications/audit/application/PhabricatorAuditApplication.php index 66280a16a0..932f2ca651 100644 --- a/src/applications/audit/application/PhabricatorAuditApplication.php +++ b/src/applications/audit/application/PhabricatorAuditApplication.php @@ -60,7 +60,7 @@ final class PhabricatorAuditApplication extends PhabricatorApplication { $count = count($commits); if ($count >= $limit) { - $count_str = pht('%s+ Problem Commit(s)', new PhutilNumber($limit - 1)); + $count_str = pht('%s+ Problem Commits', new PhutilNumber($limit - 1)); } else { $count_str = pht('%s Problem Commit(s)', new PhutilNumber($count)); } @@ -80,9 +80,13 @@ final class PhabricatorAuditApplication extends PhabricatorApplication { $count = count($commits); if ($count >= $limit) { - $count_str = pht('%s+ Problem Commit(s)', new PhutilNumber($limit - 1)); + $count_str = pht( + '%s+ Commits Awaiting Audit', + new PhutilNumber($limit - 1)); } else { - $count_str = pht('%s Problem Commit(s)', new PhutilNumber($count)); + $count_str = pht( + '%s Commit(s) Awaiting Audit', + new PhutilNumber($count)); } $type = PhabricatorApplicationStatusView::TYPE_WARNING; From 7126025fe637a4d931645f05f9ad125381600755 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Wed, 1 Jun 2016 12:33:26 -0700 Subject: [PATCH 14/24] Add information dialogs to adding project members if unsupported Summary: If you try to join or add members to a parent project, we currently return 404. This instead adds an informational dialog. Fixes T11055 Test Plan: Click on Join Project and Add Members while on a Parent Project or Milestone. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T11055 Differential Revision: https://secure.phabricator.com/D16000 --- .../PhabricatorProjectMembersAddController.php | 11 ++++++++--- .../PhabricatorProjectUpdateController.php | 14 ++++++++++---- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/applications/project/controller/PhabricatorProjectMembersAddController.php b/src/applications/project/controller/PhabricatorProjectMembersAddController.php index bd1631ee92..a3b89e46fd 100644 --- a/src/applications/project/controller/PhabricatorProjectMembersAddController.php +++ b/src/applications/project/controller/PhabricatorProjectMembersAddController.php @@ -21,12 +21,17 @@ final class PhabricatorProjectMembersAddController } $this->setProject($project); + $done_uri = "/project/members/{$id}/"; if (!$project->supportsEditMembers()) { - return new Aphront404Response(); - } + $copy = pht('Parent projects and milestones do not support adding '. + 'members. You can add members directly to any non-parent subproject.'); - $done_uri = "/project/members/{$id}/"; + return $this->newDialog() + ->setTitle(pht('Unsupported Project')) + ->appendParagraph($copy) + ->addCancelButton($done_uri); + } if ($request->isFormPost()) { $member_phids = $request->getArr('memberPHIDs'); diff --git a/src/applications/project/controller/PhabricatorProjectUpdateController.php b/src/applications/project/controller/PhabricatorProjectUpdateController.php index 762343f485..9f8c204c55 100644 --- a/src/applications/project/controller/PhabricatorProjectUpdateController.php +++ b/src/applications/project/controller/PhabricatorProjectUpdateController.php @@ -32,12 +32,18 @@ final class PhabricatorProjectUpdateController return new Aphront404Response(); } - if (!$project->supportsEditMembers()) { - return new Aphront404Response(); - } - $done_uri = "/project/members/{$id}/"; + if (!$project->supportsEditMembers()) { + $copy = pht('Parent projects and milestones do not support adding '. + 'members. You can add members directly to any non-parent subproject.'); + + return $this->newDialog() + ->setTitle(pht('Unsupported Project')) + ->appendParagraph($copy) + ->addCancelButton($done_uri); + } + if ($request->isFormPost()) { $edge_action = null; switch ($action) { From 0b1e8d0296eb7047353d00a0a771fc095296b1a4 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Wed, 1 Jun 2016 13:19:21 -0700 Subject: [PATCH 15/24] Fix margin on blank members boxes on projects Summary: Possible side effect of fixing other info views yesterday. Removes bottom margin on empty member boxes. Test Plan: Review various projects with and without members. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D16002 --- resources/celerity/map.php | 4 ++-- webroot/rsrc/css/phui/phui-two-column-view.css | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index a7b2065b79..d83da3a0ea 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -156,7 +156,7 @@ return array( 'rsrc/css/phui/phui-status.css' => 'd5263e49', 'rsrc/css/phui/phui-tag-view.css' => '6bbd83e2', 'rsrc/css/phui/phui-timeline-view.css' => '6e342216', - 'rsrc/css/phui/phui-two-column-view.css' => 'ce672be4', + 'rsrc/css/phui/phui-two-column-view.css' => '9fb86c85', 'rsrc/css/phui/workboards/phui-workboard-color.css' => 'ac6fe6a7', 'rsrc/css/phui/workboards/phui-workboard.css' => 'e6d89647', 'rsrc/css/phui/workboards/phui-workcard.css' => '0c62d7c5', @@ -859,7 +859,7 @@ return array( 'phui-tag-view-css' => '6bbd83e2', 'phui-theme-css' => '027ba77e', 'phui-timeline-view-css' => '6e342216', - 'phui-two-column-view-css' => 'ce672be4', + 'phui-two-column-view-css' => '9fb86c85', 'phui-workboard-color-css' => 'ac6fe6a7', 'phui-workboard-view-css' => 'e6d89647', 'phui-workcard-view-css' => '0c62d7c5', diff --git a/webroot/rsrc/css/phui/phui-two-column-view.css b/webroot/rsrc/css/phui/phui-two-column-view.css index 9aa4c8047c..3fc3ba6836 100644 --- a/webroot/rsrc/css/phui/phui-two-column-view.css +++ b/webroot/rsrc/css/phui/phui-two-column-view.css @@ -203,6 +203,11 @@ padding: 16px; } +.phui-two-column-view .phui-side-column .phui-object-item-empty + .phui-info-view { + margin-bottom: 0; +} + .phui-two-column-view .phui-box-blue-property .phui-header-shell + .phui-info-view { From 8b7f8cb61f47894e885838c2051121048d34b4d7 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Wed, 1 Jun 2016 13:49:02 -0700 Subject: [PATCH 16/24] Clean up Calendar sidebar on profiles Summary: Nip/Tuck. Test Plan: With and without calendar events. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D16003 --- resources/celerity/map.php | 4 ++-- .../rsrc/css/phui/calendar/phui-calendar-list.css | 15 +++++++-------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index d83da3a0ea..e462f6d017 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -116,7 +116,7 @@ return array( 'rsrc/css/layout/phabricator-side-menu-view.css' => 'dd849797', 'rsrc/css/layout/phabricator-source-code-view.css' => 'cbeef983', 'rsrc/css/phui/calendar/phui-calendar-day.css' => 'd1cf6f93', - 'rsrc/css/phui/calendar/phui-calendar-list.css' => 'e0866209', + 'rsrc/css/phui/calendar/phui-calendar-list.css' => '56e6381a', 'rsrc/css/phui/calendar/phui-calendar-month.css' => '476be7e0', 'rsrc/css/phui/calendar/phui-calendar.css' => 'ccabe893', 'rsrc/css/phui/phui-action-list.css' => 'c5eba19d', @@ -822,7 +822,7 @@ return array( 'phui-button-css' => 'a64a8de6', 'phui-calendar-css' => 'ccabe893', 'phui-calendar-day-css' => 'd1cf6f93', - 'phui-calendar-list-css' => 'e0866209', + 'phui-calendar-list-css' => '56e6381a', 'phui-calendar-month-css' => '476be7e0', 'phui-chart-css' => '6bf6f78e', 'phui-crumbs-view-css' => '6b813619', diff --git a/webroot/rsrc/css/phui/calendar/phui-calendar-list.css b/webroot/rsrc/css/phui/calendar/phui-calendar-list.css index f34fa3cb73..9580343415 100644 --- a/webroot/rsrc/css/phui/calendar/phui-calendar-list.css +++ b/webroot/rsrc/css/phui/calendar/phui-calendar-list.css @@ -6,21 +6,20 @@ width: auto; } -.phui-calendar-list-container .phui-object-box { +.phui-calendar-list-container.calendar-day-view-sidebar .phui-object-box { border-bottom: none; margin: 0; + padding: 0 12px 12px; } -.phui-calendar-list-container:last-child .phui-object-box { - border-bottom: 1px solid {$blueborder}; -} - -.phui-calendar-list-container .phui-object-box .phui-header-shell h1 { - padding: 6px 0; +.project-view-home .phui-box-grey .phui-calendar-list-container + .phui-header-shell { + padding: 8px 0; + background: #fff; } .phui-calendar-list { - padding: 16px 12px; + padding: 16px 4px; } .phui-calendar-list-item { From 39cb5e72118a2774e478a277a4cbbff7086422dc Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 22 May 2016 09:13:45 -0700 Subject: [PATCH 17/24] Improve some Phame custom domain remarkup and link behaviors Summary: Ref T6299. This makes more of the links point to the right places. Not covered yet: - Projects and subscribers don't point to the right place (this is a little tricky to fix, I think). - `[[ #anchor ]]`s won't do the right thing in, uh, email, I guess, since `uri.here` is not set. This is also a little tricky. Possibly we should just remove subscribers (although also kind of tricky). Test Plan: On a custom-domain blog, observed that fewer things were broken. Reviewers: chad Reviewed By: chad Maniphest Tasks: T6299 Differential Revision: https://secure.phabricator.com/D16007 --- .../markup/PhabricatorMentionRemarkupRule.php | 4 ++++ .../post/PhamePostViewController.php | 12 ++++++++---- .../phame/view/PhamePostListView.php | 14 ++++++++++++-- .../markup/PhabricatorMarkupEngine.php | 19 +++++++++++++++---- .../markup/view/PHUIRemarkupView.php | 2 +- 5 files changed, 40 insertions(+), 11 deletions(-) diff --git a/src/applications/people/markup/PhabricatorMentionRemarkupRule.php b/src/applications/people/markup/PhabricatorMentionRemarkupRule.php index 42efb05603..8049866118 100644 --- a/src/applications/people/markup/PhabricatorMentionRemarkupRule.php +++ b/src/applications/people/markup/PhabricatorMentionRemarkupRule.php @@ -136,6 +136,10 @@ final class PhabricatorMentionRemarkupRule extends PhutilRemarkupRule { ), '@'.$user->getUserName()); } else { + if ($engine->getConfig('uri.full')) { + $user_href = PhabricatorEnv::getURI($user_href); + } + $tag = id(new PHUITagView()) ->setType(PHUITagView::TYPE_PERSON) ->setPHID($user->getPHID()) diff --git a/src/applications/phame/controller/post/PhamePostViewController.php b/src/applications/phame/controller/post/PhamePostViewController.php index 5579e82656..fdbaf139c2 100644 --- a/src/applications/phame/controller/post/PhamePostViewController.php +++ b/src/applications/phame/controller/post/PhamePostViewController.php @@ -78,10 +78,14 @@ final class PhamePostViewController ->executeOne(); $blogger_profile = $blogger->loadUserProfile(); + + $author_uri = '/p/'.$blogger->getUsername().'/'; + $author_uri = PhabricatorEnv::getURI($author_uri); + $author = phutil_tag( 'a', array( - 'href' => '/p/'.$blogger->getUsername().'/', + 'href' => $author_uri, ), $blogger->getUsername()); @@ -105,7 +109,7 @@ final class PhamePostViewController $blogger_profile->getTitle(), )) ->setImage($blogger->getProfileImageURI()) - ->setImageHref('/p/'.$blogger->getUsername()); + ->setImageHref($author_uri); $timeline = $this->buildTransactionTimeline( $post, @@ -128,10 +132,10 @@ final class PhamePostViewController $next_view = new PhameNextPostView(); if ($next) { - $next_view->setNext($next->getTitle(), $next->getViewURI()); + $next_view->setNext($next->getTitle(), $next->getLiveURI()); } if ($prev) { - $next_view->setPrevious($prev->getTitle(), $prev->getViewURI()); + $next_view->setPrevious($prev->getTitle(), $prev->getLiveURI()); } $document->setFoot($next_view); diff --git a/src/applications/phame/view/PhamePostListView.php b/src/applications/phame/view/PhamePostListView.php index fe6da36541..1932b597a1 100644 --- a/src/applications/phame/view/PhamePostListView.php +++ b/src/applications/phame/view/PhamePostListView.php @@ -62,8 +62,19 @@ final class PhamePostListView extends AphrontTagView { $list = array(); foreach ($posts as $post) { - $blogger = $handles[$post->getBloggerPHID()]->renderLink(); + $blogger_name = $handles[$post->getBloggerPHID()]->getName(); $blogger_uri = $handles[$post->getBloggerPHID()]->getURI(); + $blogger_uri = PhabricatorEnv::getURI($blogger_uri); + + // Render a link manually to make sure we point at the correct domain. + $blogger = phutil_tag( + 'a', + array( + 'href' => $blogger_uri, + ), + $blogger_name); + $blogger = phutil_tag('strong', array(), $blogger); + $blogger_image = $handles[$post->getBloggerPHID()]->getImageURI(); $phame_post = null; @@ -74,7 +85,6 @@ final class PhamePostListView extends AphrontTagView { $phame_post = phutil_tag('em', array(), pht('(Empty Post)')); } - $blogger = phutil_tag('strong', array(), $blogger); $date = phabricator_datetime($post->getDatePublished(), $viewer); $blog = $post->getBlog(); diff --git a/src/infrastructure/markup/PhabricatorMarkupEngine.php b/src/infrastructure/markup/PhabricatorMarkupEngine.php index dc3809f873..7c19cac57f 100644 --- a/src/infrastructure/markup/PhabricatorMarkupEngine.php +++ b/src/infrastructure/markup/PhabricatorMarkupEngine.php @@ -351,10 +351,13 @@ final class PhabricatorMarkupEngine extends Phobject { * @task engine */ public static function newPhameMarkupEngine() { - return self::newMarkupEngine(array( - 'macros' => false, - 'uri.full' => true, - )); + return self::newMarkupEngine( + array( + 'macros' => false, + 'uri.full' => true, + 'uri.same-window' => true, + 'uri.base' => PhabricatorEnv::getURI('/'), + )); } @@ -487,6 +490,14 @@ final class PhabricatorMarkupEngine extends Phobject { $engine->setConfig('uri.full', $options['uri.full']); + if (isset($options['uri.base'])) { + $engine->setConfig('uri.base', $options['uri.base']); + } + + if (isset($options['uri.same-window'])) { + $engine->setConfig('uri.same-window', $options['uri.same-window']); + } + $rules = array(); $rules[] = new PhutilRemarkupEscapeRemarkupRule(); $rules[] = new PhutilRemarkupMonospaceRule(); diff --git a/src/infrastructure/markup/view/PHUIRemarkupView.php b/src/infrastructure/markup/view/PHUIRemarkupView.php index e30c09ce7c..e5772e9d84 100644 --- a/src/infrastructure/markup/view/PHUIRemarkupView.php +++ b/src/infrastructure/markup/view/PHUIRemarkupView.php @@ -82,7 +82,7 @@ final class PHUIRemarkupView extends AphrontView { $engine_key = PhabricatorHash::digestForIndex($engine_key); $cache = PhabricatorCaches::getRequestCache(); - $cache_key = "remarkup.engine({$viewer}, {$engine_key})"; + $cache_key = "remarkup.engine({$viewer_key}, {$engine_key})"; $engine = $cache->getKey($cache_key); if (!$engine) { From 7fe1a6840e5779bb5facc624c65340040e7ecc02 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 1 Jun 2016 09:21:04 -0700 Subject: [PATCH 18/24] Modularize all straightforward settings Summary: Ref T4103. This tackles all the easy stuff. Not yet handled: - Translation, pronoun, timezone: these are weird and stored on the User object instead of in settings. - Conpherence default: actually just missed this one, it's normal. - 1000 dropdowns for email notification preferences (messy, technically). Test Plan: wow look at all these settings {F1670442} Reviewers: chad Reviewed By: chad Maniphest Tasks: T4103 Differential Revision: https://secure.phabricator.com/D15999 --- src/__phutil_library_map__.php | 18 ++++++++ .../setting/PhabricatorDarkConsoleSetting.php | 34 ++++++++++++++ .../setting/PhabricatorDateFormatSetting.php | 34 ++++++++++++++ .../setting/PhabricatorEmailFormatSetting.php | 32 ++++++++++++++ .../PhabricatorEmailNotificationsSetting.php | 36 +++++++++++++++ .../PhabricatorEmailRePrefixSetting.php | 40 +++++++++++++++++ .../PhabricatorEmailSelfActionsSetting.php | 32 ++++++++++++++ .../PhabricatorEmailVarySubjectsSetting.php | 44 +++++++++++++++++++ .../setting/PhabricatorTimeFormatSetting.php | 32 ++++++++++++++ .../PhabricatorWeekStartDaySetting.php | 33 ++++++++++++++ 10 files changed, 335 insertions(+) create mode 100644 src/applications/settings/setting/PhabricatorDarkConsoleSetting.php create mode 100644 src/applications/settings/setting/PhabricatorDateFormatSetting.php create mode 100644 src/applications/settings/setting/PhabricatorEmailFormatSetting.php create mode 100644 src/applications/settings/setting/PhabricatorEmailNotificationsSetting.php create mode 100644 src/applications/settings/setting/PhabricatorEmailRePrefixSetting.php create mode 100644 src/applications/settings/setting/PhabricatorEmailSelfActionsSetting.php create mode 100644 src/applications/settings/setting/PhabricatorEmailVarySubjectsSetting.php create mode 100644 src/applications/settings/setting/PhabricatorTimeFormatSetting.php create mode 100644 src/applications/settings/setting/PhabricatorWeekStartDaySetting.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index df5588479d..7cac57cbc5 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2239,6 +2239,7 @@ phutil_register_library_map(array( 'PhabricatorDaemonsApplication' => 'applications/daemon/application/PhabricatorDaemonsApplication.php', 'PhabricatorDaemonsSetupCheck' => 'applications/config/check/PhabricatorDaemonsSetupCheck.php', 'PhabricatorDailyRoutineTriggerClock' => 'infrastructure/daemon/workers/clock/PhabricatorDailyRoutineTriggerClock.php', + 'PhabricatorDarkConsoleSetting' => 'applications/settings/setting/PhabricatorDarkConsoleSetting.php', 'PhabricatorDashboard' => 'applications/dashboard/storage/PhabricatorDashboard.php', 'PhabricatorDashboardAddPanelController' => 'applications/dashboard/controller/PhabricatorDashboardAddPanelController.php', 'PhabricatorDashboardApplication' => 'applications/dashboard/application/PhabricatorDashboardApplication.php', @@ -2296,6 +2297,7 @@ phutil_register_library_map(array( 'PhabricatorDatabaseSetupCheck' => 'applications/config/check/PhabricatorDatabaseSetupCheck.php', 'PhabricatorDatasourceEditField' => 'applications/transactions/editfield/PhabricatorDatasourceEditField.php', 'PhabricatorDatasourceEditType' => 'applications/transactions/edittype/PhabricatorDatasourceEditType.php', + 'PhabricatorDateFormatSetting' => 'applications/settings/setting/PhabricatorDateFormatSetting.php', 'PhabricatorDateTimeSettingsPanel' => 'applications/settings/panel/PhabricatorDateTimeSettingsPanel.php', 'PhabricatorDebugController' => 'applications/system/controller/PhabricatorDebugController.php', 'PhabricatorDefaultRequestExceptionHandler' => 'aphront/handler/PhabricatorDefaultRequestExceptionHandler.php', @@ -2380,9 +2382,14 @@ phutil_register_library_map(array( 'PhabricatorElasticSearchSetupCheck' => 'applications/config/check/PhabricatorElasticSearchSetupCheck.php', 'PhabricatorEmailAddressesSettingsPanel' => 'applications/settings/panel/PhabricatorEmailAddressesSettingsPanel.php', 'PhabricatorEmailContentSource' => 'applications/metamta/contentsource/PhabricatorEmailContentSource.php', + 'PhabricatorEmailFormatSetting' => 'applications/settings/setting/PhabricatorEmailFormatSetting.php', 'PhabricatorEmailFormatSettingsPanel' => 'applications/settings/panel/PhabricatorEmailFormatSettingsPanel.php', 'PhabricatorEmailLoginController' => 'applications/auth/controller/PhabricatorEmailLoginController.php', + 'PhabricatorEmailNotificationsSetting' => 'applications/settings/setting/PhabricatorEmailNotificationsSetting.php', 'PhabricatorEmailPreferencesSettingsPanel' => 'applications/settings/panel/PhabricatorEmailPreferencesSettingsPanel.php', + 'PhabricatorEmailRePrefixSetting' => 'applications/settings/setting/PhabricatorEmailRePrefixSetting.php', + 'PhabricatorEmailSelfActionsSetting' => 'applications/settings/setting/PhabricatorEmailSelfActionsSetting.php', + 'PhabricatorEmailVarySubjectsSetting' => 'applications/settings/setting/PhabricatorEmailVarySubjectsSetting.php', 'PhabricatorEmailVerificationController' => 'applications/auth/controller/PhabricatorEmailVerificationController.php', 'PhabricatorEmbedFileRemarkupRule' => 'applications/files/markup/PhabricatorEmbedFileRemarkupRule.php', 'PhabricatorEmojiRemarkupRule' => 'applications/macro/markup/PhabricatorEmojiRemarkupRule.php', @@ -3525,6 +3532,7 @@ phutil_register_library_map(array( 'PhabricatorTextAreaEditField' => 'applications/transactions/editfield/PhabricatorTextAreaEditField.php', 'PhabricatorTextEditField' => 'applications/transactions/editfield/PhabricatorTextEditField.php', 'PhabricatorTime' => 'infrastructure/time/PhabricatorTime.php', + 'PhabricatorTimeFormatSetting' => 'applications/settings/setting/PhabricatorTimeFormatSetting.php', 'PhabricatorTimeGuard' => 'infrastructure/time/PhabricatorTimeGuard.php', 'PhabricatorTimeTestCase' => 'infrastructure/time/__tests__/PhabricatorTimeTestCase.php', 'PhabricatorTimezoneSetupCheck' => 'applications/config/check/PhabricatorTimezoneSetupCheck.php', @@ -3629,6 +3637,7 @@ phutil_register_library_map(array( 'PhabricatorViewerDatasource' => 'applications/people/typeahead/PhabricatorViewerDatasource.php', 'PhabricatorWatcherHasObjectEdgeType' => 'applications/transactions/edges/PhabricatorWatcherHasObjectEdgeType.php', 'PhabricatorWebContentSource' => 'infrastructure/contentsource/PhabricatorWebContentSource.php', + 'PhabricatorWeekStartDaySetting' => 'applications/settings/setting/PhabricatorWeekStartDaySetting.php', 'PhabricatorWordPressAuthProvider' => 'applications/auth/provider/PhabricatorWordPressAuthProvider.php', 'PhabricatorWorker' => 'infrastructure/daemon/workers/PhabricatorWorker.php', 'PhabricatorWorkerActiveTask' => 'infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php', @@ -6763,6 +6772,7 @@ phutil_register_library_map(array( 'PhabricatorDaemonsApplication' => 'PhabricatorApplication', 'PhabricatorDaemonsSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorDailyRoutineTriggerClock' => 'PhabricatorTriggerClock', + 'PhabricatorDarkConsoleSetting' => 'PhabricatorSelectSetting', 'PhabricatorDashboard' => array( 'PhabricatorDashboardDAO', 'PhabricatorApplicationTransactionInterface', @@ -6838,6 +6848,7 @@ phutil_register_library_map(array( 'PhabricatorDatabaseSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorDatasourceEditField' => 'PhabricatorTokenizerEditField', 'PhabricatorDatasourceEditType' => 'PhabricatorPHIDListEditType', + 'PhabricatorDateFormatSetting' => 'PhabricatorSelectSetting', 'PhabricatorDateTimeSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorDebugController' => 'PhabricatorController', 'PhabricatorDefaultRequestExceptionHandler' => 'PhabricatorRequestExceptionHandler', @@ -6928,9 +6939,14 @@ phutil_register_library_map(array( 'PhabricatorElasticSearchSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorEmailAddressesSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorEmailContentSource' => 'PhabricatorContentSource', + 'PhabricatorEmailFormatSetting' => 'PhabricatorSelectSetting', 'PhabricatorEmailFormatSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorEmailLoginController' => 'PhabricatorAuthController', + 'PhabricatorEmailNotificationsSetting' => 'PhabricatorSelectSetting', 'PhabricatorEmailPreferencesSettingsPanel' => 'PhabricatorSettingsPanel', + 'PhabricatorEmailRePrefixSetting' => 'PhabricatorSelectSetting', + 'PhabricatorEmailSelfActionsSetting' => 'PhabricatorSelectSetting', + 'PhabricatorEmailVarySubjectsSetting' => 'PhabricatorSelectSetting', 'PhabricatorEmailVerificationController' => 'PhabricatorAuthController', 'PhabricatorEmbedFileRemarkupRule' => 'PhabricatorObjectRemarkupRule', 'PhabricatorEmojiRemarkupRule' => 'PhutilRemarkupRule', @@ -8272,6 +8288,7 @@ phutil_register_library_map(array( 'PhabricatorTextAreaEditField' => 'PhabricatorEditField', 'PhabricatorTextEditField' => 'PhabricatorEditField', 'PhabricatorTime' => 'Phobject', + 'PhabricatorTimeFormatSetting' => 'PhabricatorSelectSetting', 'PhabricatorTimeGuard' => 'Phobject', 'PhabricatorTimeTestCase' => 'PhabricatorTestCase', 'PhabricatorTimezoneSetupCheck' => 'PhabricatorSetupCheck', @@ -8403,6 +8420,7 @@ phutil_register_library_map(array( 'PhabricatorViewerDatasource' => 'PhabricatorTypeaheadDatasource', 'PhabricatorWatcherHasObjectEdgeType' => 'PhabricatorEdgeType', 'PhabricatorWebContentSource' => 'PhabricatorContentSource', + 'PhabricatorWeekStartDaySetting' => 'PhabricatorSelectSetting', 'PhabricatorWordPressAuthProvider' => 'PhabricatorOAuth2AuthProvider', 'PhabricatorWorker' => 'Phobject', 'PhabricatorWorkerActiveTask' => 'PhabricatorWorkerTask', diff --git a/src/applications/settings/setting/PhabricatorDarkConsoleSetting.php b/src/applications/settings/setting/PhabricatorDarkConsoleSetting.php new file mode 100644 index 0000000000..a95b221e63 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorDarkConsoleSetting.php @@ -0,0 +1,34 @@ + pht('Disable DarkConsole'), + self::VALUE_DARKCONSOLE_ENABLED => pht('Enable DarkConsole'), + ); + } + + +} diff --git a/src/applications/settings/setting/PhabricatorDateFormatSetting.php b/src/applications/settings/setting/PhabricatorDateFormatSetting.php new file mode 100644 index 0000000000..b2289c4872 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorDateFormatSetting.php @@ -0,0 +1,34 @@ + pht('ISO 8601: 2000-02-28'), + self::VALUE_FORMAT_US => pht('US: 2/28/2000'), + self::VALUE_FORMAT_EUROPE => pht('Europe: 28-02-2000'), + ); + } + + +} diff --git a/src/applications/settings/setting/PhabricatorEmailFormatSetting.php b/src/applications/settings/setting/PhabricatorEmailFormatSetting.php new file mode 100644 index 0000000000..f3adf78694 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorEmailFormatSetting.php @@ -0,0 +1,32 @@ + pht('Send HTML Email'), + self::VALUE_TEXT_EMAIL => pht('Send Plain Text Email'), + ); + } + +} diff --git a/src/applications/settings/setting/PhabricatorEmailNotificationsSetting.php b/src/applications/settings/setting/PhabricatorEmailNotificationsSetting.php new file mode 100644 index 0000000000..e1aab295d8 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorEmailNotificationsSetting.php @@ -0,0 +1,36 @@ + pht('Enable Email Notifications'), + self::VALUE_NO_MAIL => pht('Disable Email Notifications'), + ); + } + +} diff --git a/src/applications/settings/setting/PhabricatorEmailRePrefixSetting.php b/src/applications/settings/setting/PhabricatorEmailRePrefixSetting.php new file mode 100644 index 0000000000..596697e420 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorEmailRePrefixSetting.php @@ -0,0 +1,40 @@ + pht('Enable "Re:" Prefix'), + self::VALUE_NO_PREFIX => pht('Disable "Re:" Prefix'), + ); + } + +} diff --git a/src/applications/settings/setting/PhabricatorEmailSelfActionsSetting.php b/src/applications/settings/setting/PhabricatorEmailSelfActionsSetting.php new file mode 100644 index 0000000000..c42954082c --- /dev/null +++ b/src/applications/settings/setting/PhabricatorEmailSelfActionsSetting.php @@ -0,0 +1,32 @@ + pht('Enable Self Action Mail'), + self::VALUE_NO_SELF => pht('Disable Self Action Mail'), + ); + } + +} diff --git a/src/applications/settings/setting/PhabricatorEmailVarySubjectsSetting.php b/src/applications/settings/setting/PhabricatorEmailVarySubjectsSetting.php new file mode 100644 index 0000000000..10bfafff08 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorEmailVarySubjectsSetting.php @@ -0,0 +1,44 @@ + pht('Enable "Re:" Prefix'), + self::VALUE_STATIC_SUBJECTS => pht('Disable "Re:" Prefix'), + ); + } + +} diff --git a/src/applications/settings/setting/PhabricatorTimeFormatSetting.php b/src/applications/settings/setting/PhabricatorTimeFormatSetting.php new file mode 100644 index 0000000000..cd9c4d02d9 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorTimeFormatSetting.php @@ -0,0 +1,32 @@ + pht('12 Hour, 2:34 PM'), + self::VALUE_FORMAT_24HOUR => pht('24 Hour, 14:34'), + ); + } + + +} diff --git a/src/applications/settings/setting/PhabricatorWeekStartDaySetting.php b/src/applications/settings/setting/PhabricatorWeekStartDaySetting.php new file mode 100644 index 0000000000..8d3bb93c07 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorWeekStartDaySetting.php @@ -0,0 +1,33 @@ + pht('Sunday'), + 1 => pht('Monday'), + 2 => pht('Tuesday'), + 3 => pht('Wednesday'), + 4 => pht('Thursday'), + 5 => pht('Friday'), + 6 => pht('Saturday'), + ); + } + +} From 9180f429eb9f403918a2c4da5300331f9adfb223 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 1 Jun 2016 12:14:39 -0700 Subject: [PATCH 19/24] Provide a general-purpose, modular user cache for settings and other similar data Summary: Ref T4103. Currently, we issue a `SELECT * FROM user_preferences ... WHERE userPHID = ...` on every page to load the viewer's settings. There are several other questionable data accesses on every page too, most of which could benefit from improved caching strategies (see T4103#178122). This query will soon get more expensive, since it may need to load several objects (e.g., the user's settings and their "role profile" settings). Although we could put that data on the User and do both in one query, it's nicer to put it on the Preferences object ("This inherits from profile X") which means we need to do several queries. Rather than paying a greater price, we can cheat this stuff into the existing query where we load the user's session by providing a user cache table and doing some JOIN magic. This lets us issue one query and try to get cache hits on a bunch of caches cheaply (well, we'll be in trouble at the MySQL JOIN limit of 61 tables, but have some headroom). For now, just get it working: - Add the table. - Try to get user settings "for free" when we load the session. - If we miss, fill user settings into the cache on-demand. - We only use this in one place (DarkConsole) for now. I'll use it more widely in the next diff. Test Plan: - Loaded page as logged-in user. - Loaded page as logged-out user. - Examined session query to see cache joins. - Changed settings, saw database cache fill. - Toggled DarkConsole on and off. Reviewers: chad Reviewed By: chad Maniphest Tasks: T4103 Differential Revision: https://secure.phabricator.com/D16001 --- .../autopatches/20160601.user.01.cache.sql | 11 ++ src/__phutil_library_map__.php | 8 +- .../engine/PhabricatorAuthSessionEngine.php | 89 +++++++++++++- .../base/controller/PhabricatorController.php | 5 +- .../people/cache/PhabricatorUserCacheType.php | 70 +++++++++++ .../PhabricatorUserPreferencesCacheType.php | 31 +++++ .../people/storage/PhabricatorUser.php | 73 ++++++++++++ .../people/storage/PhabricatorUserCache.php | 110 ++++++++++++++++++ .../PhabricatorUserPreferencesEditor.php | 24 +++- .../storage/PhabricatorUserPreferences.php | 11 ++ 10 files changed, 422 insertions(+), 10 deletions(-) create mode 100644 resources/sql/autopatches/20160601.user.01.cache.sql create mode 100644 src/applications/people/cache/PhabricatorUserCacheType.php create mode 100644 src/applications/people/cache/PhabricatorUserPreferencesCacheType.php create mode 100644 src/applications/people/storage/PhabricatorUserCache.php diff --git a/resources/sql/autopatches/20160601.user.01.cache.sql b/resources/sql/autopatches/20160601.user.01.cache.sql new file mode 100644 index 0000000000..bb3386b02e --- /dev/null +++ b/resources/sql/autopatches/20160601.user.01.cache.sql @@ -0,0 +1,11 @@ +CREATE TABLE {$NAMESPACE}_user.user_cache ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + userPHID VARBINARY(64) NOT NULL, + cacheIndex BINARY(12) NOT NULL, + cacheKey VARCHAR(255) NOT NULL COLLATE {$COLLATE_TEXT}, + cacheData LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}, + cacheType VARCHAR(32) NOT NULL COLLATE {$COLLATE_TEXT}, + UNIQUE KEY `key_usercache` (userPHID, cacheIndex), + KEY `key_cachekey` (cacheIndex), + KEY `key_cachetype` (cacheType) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 7cac57cbc5..259c5301bf 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3596,6 +3596,8 @@ phutil_register_library_map(array( 'PhabricatorUnsubscribedFromObjectEdgeType' => 'applications/transactions/edges/PhabricatorUnsubscribedFromObjectEdgeType.php', 'PhabricatorUser' => 'applications/people/storage/PhabricatorUser.php', 'PhabricatorUserBlurbField' => 'applications/people/customfield/PhabricatorUserBlurbField.php', + 'PhabricatorUserCache' => 'applications/people/storage/PhabricatorUserCache.php', + 'PhabricatorUserCacheType' => 'applications/people/cache/PhabricatorUserCacheType.php', 'PhabricatorUserCardView' => 'applications/people/view/PhabricatorUserCardView.php', 'PhabricatorUserConfigOptions' => 'applications/people/config/PhabricatorUserConfigOptions.php', 'PhabricatorUserConfiguredCustomField' => 'applications/people/customfield/PhabricatorUserConfiguredCustomField.php', @@ -3614,6 +3616,7 @@ phutil_register_library_map(array( 'PhabricatorUserLogView' => 'applications/people/view/PhabricatorUserLogView.php', 'PhabricatorUserPHIDResolver' => 'applications/phid/resolver/PhabricatorUserPHIDResolver.php', 'PhabricatorUserPreferences' => 'applications/settings/storage/PhabricatorUserPreferences.php', + 'PhabricatorUserPreferencesCacheType' => 'applications/people/cache/PhabricatorUserPreferencesCacheType.php', 'PhabricatorUserPreferencesEditor' => 'applications/settings/editor/PhabricatorUserPreferencesEditor.php', 'PhabricatorUserPreferencesPHIDType' => 'applications/settings/phid/PhabricatorUserPreferencesPHIDType.php', 'PhabricatorUserPreferencesQuery' => 'applications/settings/query/PhabricatorUserPreferencesQuery.php', @@ -8368,6 +8371,8 @@ phutil_register_library_map(array( 'PhabricatorConduitResultInterface', ), 'PhabricatorUserBlurbField' => 'PhabricatorUserCustomField', + 'PhabricatorUserCache' => 'PhabricatorUserDAO', + 'PhabricatorUserCacheType' => 'Phobject', 'PhabricatorUserCardView' => 'AphrontTagView', 'PhabricatorUserConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorUserConfiguredCustomField' => array( @@ -8397,7 +8402,8 @@ phutil_register_library_map(array( 'PhabricatorDestructibleInterface', 'PhabricatorApplicationTransactionInterface', ), - 'PhabricatorUserPreferencesEditor' => 'AlmanacEditor', + 'PhabricatorUserPreferencesCacheType' => 'PhabricatorUserCacheType', + 'PhabricatorUserPreferencesEditor' => 'PhabricatorApplicationTransactionEditor', 'PhabricatorUserPreferencesPHIDType' => 'PhabricatorPHIDType', 'PhabricatorUserPreferencesQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorUserPreferencesTransaction' => 'PhabricatorApplicationTransaction', diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php index 1547d6bea8..a3ee910629 100644 --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -7,6 +7,7 @@ * @task hisec High Security * @task partial Partial Sessions * @task onetime One Time Login URIs + * @task cache User Cache */ final class PhabricatorAuthSessionEngine extends Phobject { @@ -111,9 +112,8 @@ final class PhabricatorAuthSessionEngine extends Phobject { $conn_r = $session_table->establishConnection('r'); $session_key = PhabricatorHash::digest($session_token); - // NOTE: We're being clever here because this happens on every page load, - // and by joining we can save a query. This might be getting too clever - // for its own good, though... + $cache_parts = $this->getUserCacheQueryParts($conn_r); + list($cache_selects, $cache_joins, $cache_map) = $cache_parts; $info = queryfx_one( $conn_r, @@ -125,12 +125,15 @@ final class PhabricatorAuthSessionEngine extends Phobject { s.isPartial AS s_isPartial, s.signedLegalpadDocuments as s_signedLegalpadDocuments, u.* + %Q FROM %T u JOIN %T s ON u.phid = s.userPHID - AND s.type = %s AND s.sessionKey = %s', + AND s.type = %s AND s.sessionKey = %s %Q', + $cache_selects, $user_table->getTableName(), $session_table->getTableName(), $session_type, - $session_key); + $session_key, + $cache_joins); if (!$info) { return null; @@ -141,14 +144,26 @@ final class PhabricatorAuthSessionEngine extends Phobject { 'sessionKey' => $session_key, 'type' => $session_type, ); + + $cache_raw = array_fill_keys($cache_map, null); foreach ($info as $key => $value) { if (strncmp($key, 's_', 2) === 0) { unset($info[$key]); $session_dict[substr($key, 2)] = $value; + continue; + } + + if (isset($cache_map[$key])) { + unset($info[$key]); + $cache_raw[$cache_map[$key]] = $value; + continue; } } $user = $user_table->loadFromArray($info); + + $user->attachRawCacheData($cache_raw); + switch ($session_type) { case PhabricatorAuthSession::TYPE_WEB: // Explicitly prevent bots and mailing lists from establishing web @@ -732,4 +747,68 @@ final class PhabricatorAuthSessionEngine extends Phobject { return PhabricatorHash::digest(implode(':', $parts)); } + +/* -( User Cache )--------------------------------------------------------- */ + + + /** + * @task cache + */ + private function getUserCacheQueryParts(AphrontDatabaseConnection $conn) { + $cache_selects = array(); + $cache_joins = array(); + $cache_map = array(); + + $keys = array(); + + $cache_types = PhabricatorUserCacheType::getAllCacheTypes(); + foreach ($cache_types as $cache_type) { + foreach ($cache_type->getAutoloadKeys() as $autoload_key) { + $keys[] = $autoload_key; + } + } + + $cache_table = id(new PhabricatorUserCache())->getTableName(); + + $cache_idx = 1; + foreach ($keys as $key) { + $join_as = 'ucache_'.$cache_idx; + $select_as = 'ucache_'.$cache_idx.'_v'; + + $cache_selects[] = qsprintf( + $conn, + '%T.cacheData %T', + $join_as, + $select_as); + + $cache_joins[] = qsprintf( + $conn, + 'LEFT JOIN %T AS %T ON u.phid = %T.userPHID + AND %T.cacheIndex = %s', + $cache_table, + $join_as, + $join_as, + $join_as, + PhabricatorHash::digestForIndex($key)); + + $cache_map[$select_as] = $key; + + $cache_idx++; + } + + if ($cache_selects) { + $cache_selects = ', '.implode(', ', $cache_selects); + } else { + $cache_selects = ''; + } + + if ($cache_joins) { + $cache_joins = implode(' ', $cache_joins); + } else { + $cache_joins = ''; + } + + return array($cache_selects, $cache_joins, $cache_map); + } + } diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php index 5d8ff7ba60..83e223d195 100644 --- a/src/applications/base/controller/PhabricatorController.php +++ b/src/applications/base/controller/PhabricatorController.php @@ -106,10 +106,9 @@ abstract class PhabricatorController extends AphrontController { PhabricatorEnv::setLocaleCode($user->getTranslation()); - $preferences = $user->loadPreferences(); if (PhabricatorEnv::getEnvConfig('darkconsole.enabled')) { - $dark_console = PhabricatorUserPreferences::PREFERENCE_DARK_CONSOLE; - if ($preferences->getPreference($dark_console) || + $dark_console = PhabricatorDarkConsoleSetting::SETTINGKEY; + if ($user->getUserSetting($dark_console) || PhabricatorEnv::getEnvConfig('darkconsole.always-on')) { $console = new DarkConsoleCore(); $request->getApplicationConfiguration()->setConsole($console); diff --git a/src/applications/people/cache/PhabricatorUserCacheType.php b/src/applications/people/cache/PhabricatorUserCacheType.php new file mode 100644 index 0000000000..f2304b830d --- /dev/null +++ b/src/applications/people/cache/PhabricatorUserCacheType.php @@ -0,0 +1,70 @@ +getPhobjectClassConstant('CACHETYPE'); + } + + public static function getAllCacheTypes() { + return id(new PhutilClassMapQuery()) + ->setAncestorClass(__CLASS__) + ->setUniqueMethod('getUserCacheType') + ->execute(); + } + + public static function getCacheTypeForKey($key) { + $all = self::getAllCacheTypes(); + + foreach ($all as $type) { + if ($type->canManageKey($key)) { + return $type; + } + } + + return null; + } + + public static function requireCacheTypeForKey($key) { + $type = self::getCacheTypeForKey($key); + + if (!$type) { + throw new Exception( + pht( + 'Failed to load UserCacheType to manage key "%s". This cache type '. + 'is required.', + $key)); + } + + return $type; + } + +} diff --git a/src/applications/people/cache/PhabricatorUserPreferencesCacheType.php b/src/applications/people/cache/PhabricatorUserPreferencesCacheType.php new file mode 100644 index 0000000000..88eeb3bbd5 --- /dev/null +++ b/src/applications/people/cache/PhabricatorUserPreferencesCacheType.php @@ -0,0 +1,31 @@ +getViewer(); + + $preferences = id(new PhabricatorUserPreferencesQuery()) + ->setViewer($viewer) + ->withUserPHIDs(mpull($users, 'getPHID')) + ->execute(); + + return mpull($preferences, 'getPreferences', 'getUserPHID'); + } + +} diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 9a65c44470..fd2f69ccb6 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -5,6 +5,7 @@ * @task image-cache Profile Image Cache * @task factors Multi-Factor Authentication * @task handles Managing Handles + * @task cache User Cache */ final class PhabricatorUser extends PhabricatorUserDAO @@ -61,6 +62,8 @@ final class PhabricatorUser private $alternateCSRFString = self::ATTACHABLE; private $session = self::ATTACHABLE; + private $rawCacheData = array(); + private $usableCacheData = array(); private $authorities = array(); private $handlePool; @@ -487,6 +490,22 @@ final class PhabricatorUser '(isPrimary = 1)'); } + public function getUserSetting($key) { + $settings_key = PhabricatorUserPreferencesCacheType::KEY_PREFERENCES; + $settings = $this->requireCacheData($settings_key); + + if (array_key_exists($key, $settings)) { + return $settings[$key]; + } + + $defaults = PhabricatorSetting::getAllEnabledSettings($this); + if (isset($defaults[$key])) { + return $defaults[$key]->getSettingDefaultValue(); + } + + return null; + } + public function loadPreferences() { if ($this->preferences) { return $this->preferences; @@ -1461,4 +1480,58 @@ final class PhabricatorUser } +/* -( User Cache )--------------------------------------------------------- */ + + + /** + * @task cache + */ + public function attachRawCacheData(array $data) { + $this->rawCacheData = $data + $this->rawCacheData; + return $this; + } + + + /** + * @task cache + */ + protected function requireCacheData($key) { + if (isset($this->usableCacheData[$key])) { + return $this->usableCacheData[$key]; + } + + $type = PhabricatorUserCacheType::requireCacheTypeForKey($key); + + if (isset($this->rawCacheData[$key])) { + $raw_value = $this->rawCacheData[$key]; + + $usable_value = $type->getValueFromStorage($raw_value); + $this->usableCacheData[$key] = $usable_value; + + return $usable_value; + } + + $usable_value = $type->getDefaultValue(); + + $user_phid = $this->getPHID(); + if ($user_phid) { + $map = $type->newValueForUsers($key, array($this)); + if (array_key_exists($user_phid, $map)) { + $usable_value = $map[$user_phid]; + $raw_value = $type->getValueForStorage($usable_value); + + $this->rawCacheData[$key] = $raw_value; + PhabricatorUserCache::writeCache( + $type, + $key, + $user_phid, + $raw_value); + } + } + + $this->usableCacheData[$key] = $usable_value; + + return $usable_value; + } + } diff --git a/src/applications/people/storage/PhabricatorUserCache.php b/src/applications/people/storage/PhabricatorUserCache.php new file mode 100644 index 0000000000..7228a9dfa5 --- /dev/null +++ b/src/applications/people/storage/PhabricatorUserCache.php @@ -0,0 +1,110 @@ + false, + self::CONFIG_COLUMN_SCHEMA => array( + 'cacheIndex' => 'bytes12', + 'cacheKey' => 'text255', + 'cacheData' => 'text', + 'cacheType' => 'text32', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_usercache' => array( + 'columns' => array('userPHID', 'cacheIndex'), + 'unique' => true, + ), + 'key_cachekey' => array( + 'columns' => array('cacheIndex'), + ), + 'key_cachetype' => array( + 'columns' => array('cacheType'), + ), + ), + ) + parent::getConfiguration(); + } + + public function save() { + $this->cacheIndex = Filesystem::digestForIndex($this->getCacheKey()); + return parent::save(); + } + + public static function writeCache( + PhabricatorUserCacheType $type, + $key, + $user_phid, + $raw_value) { + + if (PhabricatorEnv::isReadOnly()) { + return; + } + + $table = new self(); + $conn_w = $table->establishConnection('w'); + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + + queryfx( + $conn_w, + 'INSERT INTO %T (userPHID, cacheIndex, cacheKey, cacheData, cacheType) + VALUES (%s, %s, %s, %s, %s) + ON DUPLICATE KEY UPDATE cacheData = VALUES(cacheData)', + $table->getTableName(), + $user_phid, + PhabricatorHash::digestForIndex($key), + $key, + $raw_value, + $type->getUserCacheType()); + + unset($unguarded); + } + + public static function clearCache($key, $user_phid) { + if (PhabricatorEnv::isReadOnly()) { + return; + } + + $table = new self(); + $conn_w = $table->establishConnection('w'); + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + + queryfx( + $conn_w, + 'DELETE FROM %T WHERE cacheIndex = %s AND userPHID = %s', + $table->getTableName(), + PhabricatorHash::digestForIndex($key), + $user_phid); + + unset($unguarded); + } + + + public static function clearCacheForAllUsers($key) { + if (PhabricatorEnv::isReadOnly()) { + return; + } + + $table = new self(); + $conn_w = $table->establishConnection('w'); + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + + queryfx( + $conn_w, + 'DELETE FROM %T WHERE cacheIndex = %s', + $table->getTableName(), + PhabricatorHash::digestForIndex($key)); + + unset($unguarded); + } + +} diff --git a/src/applications/settings/editor/PhabricatorUserPreferencesEditor.php b/src/applications/settings/editor/PhabricatorUserPreferencesEditor.php index 6be467de8f..bbb469eb01 100644 --- a/src/applications/settings/editor/PhabricatorUserPreferencesEditor.php +++ b/src/applications/settings/editor/PhabricatorUserPreferencesEditor.php @@ -1,7 +1,11 @@ getUserPHID(); + if ($user_phid) { + PhabricatorUserCache::clearCache( + PhabricatorUserPreferencesCacheType::KEY_PREFERENCES, + $user_phid); + } else { + PhabricatorUserCache::clearCacheForAllUsers( + PhabricatorUserPreferencesCacheType::KEY_PREFERENCES); + } + + + return $xactions; + } + } diff --git a/src/applications/settings/storage/PhabricatorUserPreferences.php b/src/applications/settings/storage/PhabricatorUserPreferences.php index 7805daf771..68493b90d0 100644 --- a/src/applications/settings/storage/PhabricatorUserPreferences.php +++ b/src/applications/settings/storage/PhabricatorUserPreferences.php @@ -165,6 +165,17 @@ final class PhabricatorUserPreferences return false; } + // TODO: Remove this once all edits go through the Editor. For now, some + // old edits just do direct saves so make sure we nuke the cache. + public function save() { + PhabricatorUserCache::clearCache( + PhabricatorUserPreferencesCacheType::KEY_PREFERENCES, + $this->getUserPHID()); + + return parent::save(); + } + + /* -( PhabricatorPolicyInterface )----------------------------------------- */ From edfc6a69345a05816edb0740069bd1270919056e Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 1 Jun 2016 13:59:08 -0700 Subject: [PATCH 20/24] Convert some loadPreferences() to getUserSetting() Summary: Ref T4103. This doesn't get everything, but takes care of most of the easy stuff. The tricky-ish bit here is that I need to move timezones, pronouns and translations to proper settings. I expect to pursue that next. Test Plan: - Grepped for `loadPreferences` to identify callsites. - Changed start-of-week setting, loaded Calendar, saw correct start. - Visited welcome page, read "Adjust Settings" point. - Loaded Conpherence -- I changed behavior here slightly (switching threads drops the title glyph) but it wasn't consistent to start with and this seems like a good thing to push to the next version of Conpherence. - Enabled Filetree, toggled in Differential. - Disabled Filetree, no longer visible in Differential. - Changed "Unified Diffs" preference to "Small Screens" vs "Always". - Toggled filetree in Diffusion. - Edited a task, saw sensible projects in policy dropdown. - Viewed user profile, uncollapsed/collapsed side nav, reloaded page, sticky'd. - Toggled "monospaced textareas", used a comment box, got appropriate fonts. - Toggled durable column. - Disabled title glyphs. - Changed monospaced font to 18px/36px impact. Reviewers: chad Reviewed By: chad Maniphest Tasks: T4103 Differential Revision: https://secure.phabricator.com/D16004 --- .../sql/autopatches/20140722.appname.php | 30 ++----------------- .../PhabricatorCalendarEventSearchEngine.php | 5 ++-- .../PhabricatorConfigWelcomeController.php | 10 +++++-- .../view/ConpherenceThreadListView.php | 10 +------ .../DifferentialRevisionViewController.php | 15 +++++----- .../parser/DifferentialChangesetParser.php | 9 ++++-- .../query/DifferentialInlineCommentQuery.php | 7 +++-- .../controller/DiffusionCommitController.php | 11 +++---- .../people/storage/PhabricatorUser.php | 5 ++++ .../policy/query/PhabricatorPolicyQuery.php | 3 +- .../engine/PhabricatorProfilePanelEngine.php | 3 +- .../control/PhabricatorRemarkupControl.php | 13 ++++---- src/view/page/PhabricatorStandardPageView.php | 19 ++++++------ .../phui/calendar/PHUICalendarMonthView.php | 6 ++-- 14 files changed, 62 insertions(+), 84 deletions(-) diff --git a/resources/sql/autopatches/20140722.appname.php b/resources/sql/autopatches/20140722.appname.php index 8c3e5918b2..4e617d5102 100644 --- a/resources/sql/autopatches/20140722.appname.php +++ b/resources/sql/autopatches/20140722.appname.php @@ -74,35 +74,9 @@ foreach ($applications as $application) { /* -( User preferences )--------------------------------------------------- */ -echo pht('Migrating user preferences...')."\n"; -$table = new PhabricatorUserPreferences(); -$conn_w = $table->establishConnection('w'); -$pref_pinned = PhabricatorUserPreferences::PREFERENCE_APP_PINNED; -foreach (new LiskMigrationIterator(new PhabricatorUser()) as $user) { - $user_preferences = $user->loadPreferences(); - - $old_pinned_apps = $user_preferences->getPreference($pref_pinned); - $new_pinned_apps = array(); - - if (!$old_pinned_apps) { - continue; - } - - foreach ($old_pinned_apps as $pinned_app) { - $new_pinned_apps[] = idx($map, $pinned_app, $pinned_app); - } - - $user_preferences - ->setPreference($pref_pinned, $new_pinned_apps); - - queryfx( - $conn_w, - 'UPDATE %T SET preferences = %s WHERE id = %d', - $user_preferences->getTableName(), - json_encode($user_preferences->getPreferences()), - $user_preferences->getID()); -} +// This originally migrated pinned applications in user preferences, but was +// removed to simplify preference changes after about 22 months. /* -( Dashboard installs )------------------------------------------------- */ diff --git a/src/applications/calendar/query/PhabricatorCalendarEventSearchEngine.php b/src/applications/calendar/query/PhabricatorCalendarEventSearchEngine.php index 8b321ef73e..9eb50048fb 100644 --- a/src/applications/calendar/query/PhabricatorCalendarEventSearchEngine.php +++ b/src/applications/calendar/query/PhabricatorCalendarEventSearchEngine.php @@ -151,10 +151,9 @@ final class PhabricatorCalendarEventSearchEngine $display_start = $start_day->format('U'); $display_end = $next->format('U'); - $preferences = $viewer->loadPreferences(); - $pref_week_day = PhabricatorUserPreferences::PREFERENCE_WEEK_START_DAY; + $start_of_week = $viewer->getUserSetting( + PhabricatorWeekStartDaySetting::SETTINGKEY); - $start_of_week = $preferences->getPreference($pref_week_day, 0); $end_of_week = ($start_of_week + 6) % 7; $first_of_month = $start_day->format('w'); diff --git a/src/applications/config/controller/PhabricatorConfigWelcomeController.php b/src/applications/config/controller/PhabricatorConfigWelcomeController.php index addf80e62f..a7d29b913b 100644 --- a/src/applications/config/controller/PhabricatorConfigWelcomeController.php +++ b/src/applications/config/controller/PhabricatorConfigWelcomeController.php @@ -141,8 +141,14 @@ final class PhabricatorConfigWelcomeController $content); $settings_href = PhabricatorEnv::getURI('/settings/'); - $prefs = $viewer->loadPreferences()->getPreferences(); - $have_settings = !empty($prefs); + + $preferences = id(new PhabricatorUserPreferencesQuery()) + ->setViewer($viewer) + ->withUsers(array($viewer)) + ->executeOne(); + + $have_settings = ($preferences && $preferences->getPreferences()); + if ($have_settings) { $content = pht( "=== Adjust Account Settings ===\n\n". diff --git a/src/applications/conpherence/view/ConpherenceThreadListView.php b/src/applications/conpherence/view/ConpherenceThreadListView.php index 40262c765b..a4f4a7de6d 100644 --- a/src/applications/conpherence/view/ConpherenceThreadListView.php +++ b/src/applications/conpherence/view/ConpherenceThreadListView.php @@ -72,14 +72,6 @@ final class ConpherenceThreadListView extends AphrontView { $epoch = $data['epoch']; $image = $data['image']; $dom_id = $thread->getPHID().'-nav-item'; - $glyph_pref = PhabricatorUserPreferences::PREFERENCE_TITLES; - $preferences = $user->loadPreferences(); - if ($preferences->getPreference($glyph_pref) == 'glyph') { - $glyph = id(new PhabricatorConpherenceApplication()) - ->getTitleGlyph().' '; - } else { - $glyph = null; - } return id(new ConpherenceMenuItemView()) ->setUser($user) @@ -93,7 +85,7 @@ final class ConpherenceThreadListView extends AphrontView { ->addSigil('conpherence-menu-click') ->setMetadata( array( - 'title' => $glyph.$data['title'], + 'title' => $data['title'], 'id' => $dom_id, 'threadID' => $thread->getID(), )); diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index ef333a4197..39fc6ca201 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -375,18 +375,19 @@ final class DifferentialRevisionViewController extends DifferentialController { $crumbs->addTextCrumb($object_id, '/'.$object_id); $crumbs->setBorder(true); - $prefs = $viewer->loadPreferences(); - $pref_filetree = PhabricatorUserPreferences::PREFERENCE_DIFF_FILETREE; + $filetree_on = $viewer->compareUserSetting( + PhabricatorShowFiletreeSetting::SETTINGKEY, + PhabricatorShowFiletreeSetting::VALUE_ENABLE_FILETREE); + $nav = null; - if ($prefs->getPreference($pref_filetree)) { - $collapsed = $prefs->getPreference( - PhabricatorUserPreferences::PREFERENCE_NAV_COLLAPSED, - false); + if ($filetree_on) { + $collapsed_key = PhabricatorUserPreferences::PREFERENCE_NAV_COLLAPSED; + $collapsed_value = $viewer->getUserSetting($collapsed_key); $nav = id(new DifferentialChangesetFileTreeSideNavBuilder()) ->setTitle('D'.$revision->getID()) ->setBaseURI(new PhutilURI('/D'.$revision->getID())) - ->setCollapsed((bool)$collapsed) + ->setCollapsed((bool)$collapsed_value) ->build($changesets); } diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index e49538fd91..c745d8e42a 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -150,11 +150,14 @@ final class DifferentialChangesetParser extends Phobject { } public static function getDefaultRendererForViewer(PhabricatorUser $viewer) { - $prefs = $viewer->loadPreferences(); - $pref_unified = PhabricatorUserPreferences::PREFERENCE_DIFF_UNIFIED; - if ($prefs->getPreference($pref_unified) == 'unified') { + $is_unified = $viewer->compareUserSetting( + PhabricatorUnifiedDiffsSetting::SETTINGKEY, + PhabricatorUnifiedDiffsSetting::VALUE_ALWAYS_UNIFIED); + + if ($is_unified) { return '1up'; } + return null; } diff --git a/src/applications/differential/query/DifferentialInlineCommentQuery.php b/src/applications/differential/query/DifferentialInlineCommentQuery.php index 7e986bdc60..3f8ea62e14 100644 --- a/src/applications/differential/query/DifferentialInlineCommentQuery.php +++ b/src/applications/differential/query/DifferentialInlineCommentQuery.php @@ -175,9 +175,10 @@ final class DifferentialInlineCommentQuery $viewer = $this->getViewer(); - $pref = $viewer->loadPreferences()->getPreference( - PhabricatorUserPreferences::PREFERENCE_DIFF_GHOSTS); - if ($pref == 'disabled') { + $no_ghosts = $viewer->compareUserSetting( + PhabricatorOlderInlinesSetting::SETTINGKEY, + PhabricatorOlderInlinesSetting::VALUE_GHOST_INLINES_DISABLED); + if ($no_ghosts) { return $inlines; } diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 419be17898..6cdca19953 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -325,14 +325,15 @@ final class DiffusionCommitController extends DiffusionController { $add_comment = $this->renderAddCommentPanel($commit, $audit_requests); - $prefs = $viewer->loadPreferences(); - $pref_filetree = PhabricatorUserPreferences::PREFERENCE_DIFF_FILETREE; + $filetree_on = $viewer->compareUserSetting( + PhabricatorShowFiletreeSetting::SETTINGKEY, + PhabricatorShowFiletreeSetting::VALUE_ENABLE_FILETREE); + $pref_collapse = PhabricatorUserPreferences::PREFERENCE_NAV_COLLAPSED; - $show_filetree = $prefs->getPreference($pref_filetree); - $collapsed = $prefs->getPreference($pref_collapse); + $collapsed = $viewer->getUserSetting($pref_collapse); $nav = null; - if ($show_changesets && $show_filetree) { + if ($show_changesets && $filetree_on) { $nav = id(new DifferentialChangesetFileTreeSideNavBuilder()) ->setTitle($commit->getDisplayName()) ->setBaseURI(new PhutilURI($commit->getURI())) diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index fd2f69ccb6..0c19b3acdd 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -506,6 +506,11 @@ final class PhabricatorUser return null; } + public function compareUserSetting($key, $value) { + $actual = $this->getUserSetting($key); + return ($actual == $value); + } + public function loadPreferences() { if ($this->preferences) { return $this->preferences; diff --git a/src/applications/policy/query/PhabricatorPolicyQuery.php b/src/applications/policy/query/PhabricatorPolicyQuery.php index e6e7008827..81c9bb8a51 100644 --- a/src/applications/policy/query/PhabricatorPolicyQuery.php +++ b/src/applications/policy/query/PhabricatorPolicyQuery.php @@ -201,8 +201,7 @@ final class PhabricatorPolicyQuery $default_limit = 5; // If possible, show the user's 10 most recently used projects. - $preferences = $viewer->loadPreferences(); - $favorites = $preferences->getPreference($pref_key); + $favorites = $viewer->getUserSetting($pref_key); if (!is_array($favorites)) { $favorites = array(); } diff --git a/src/applications/search/engine/PhabricatorProfilePanelEngine.php b/src/applications/search/engine/PhabricatorProfilePanelEngine.php index b92ba451d0..6e2e74f3b6 100644 --- a/src/applications/search/engine/PhabricatorProfilePanelEngine.php +++ b/src/applications/search/engine/PhabricatorProfilePanelEngine.php @@ -385,8 +385,7 @@ abstract class PhabricatorProfilePanelEngine extends Phobject { $collapse_key = PhabricatorUserPreferences::PREFERENCE_PROFILE_MENU_COLLAPSED; - $preferences = $viewer->loadPreferences(); - $is_collapsed = $preferences->getPreference($collapse_key, false); + $is_collapsed = $viewer->getUserSetting($collapse_key); if ($is_collapsed) { $nav->addClass('phui-profile-menu-collapsed'); diff --git a/src/view/form/control/PhabricatorRemarkupControl.php b/src/view/form/control/PhabricatorRemarkupControl.php index eeec4971da..f32e58fb29 100644 --- a/src/view/form/control/PhabricatorRemarkupControl.php +++ b/src/view/form/control/PhabricatorRemarkupControl.php @@ -260,15 +260,14 @@ final class PhabricatorRemarkupControl extends AphrontFormTextAreaControl { ), $buttons); - $monospaced_textareas = null; - $monospaced_textareas_class = null; + $use_monospaced = $viewer->compareUserSetting( + PhabricatorMonospacedTextareasSetting::SETTINGKEY, + PhabricatorMonospacedTextareasSetting::VALUE_TEXT_MONOSPACED); - $monospaced_textareas = $viewer - ->loadPreferences() - ->getPreference( - PhabricatorUserPreferences::PREFERENCE_MONOSPACED_TEXTAREAS); - if ($monospaced_textareas == 'enabled') { + if ($use_monospaced) { $monospaced_textareas_class = 'PhabricatorMonospaced'; + } else { + $monospaced_textareas_class = null; } $this->setCustomClass( diff --git a/src/view/page/PhabricatorStandardPageView.php b/src/view/page/PhabricatorStandardPageView.php index e859163272..e2d373ab5a 100644 --- a/src/view/page/PhabricatorStandardPageView.php +++ b/src/view/page/PhabricatorStandardPageView.php @@ -133,7 +133,7 @@ final class PhabricatorStandardPageView extends PhabricatorBarePageView public function getDurableColumnVisible() { $column_key = PhabricatorUserPreferences::PREFERENCE_CONPHERENCE_COLUMN; - return (bool)$this->getUserPreference($column_key, 0); + return (bool)$this->getUserPreference($column_key, false); } public function addQuicksandConfig(array $config) { @@ -164,12 +164,11 @@ final class PhabricatorStandardPageView extends PhabricatorBarePageView } public function getTitle() { - $glyph_key = PhabricatorUserPreferences::PREFERENCE_TITLES; - if ($this->getUserPreference($glyph_key) == 'text') { - $use_glyph = false; - } else { - $use_glyph = true; - } + $glyph_key = PhabricatorTitleGlyphsSetting::SETTINGKEY; + $glyph_on = PhabricatorTitleGlyphsSetting::VALUE_TITLE_GLYPHS; + $glyph_setting = $this->getUserPreference($glyph_key, $glyph_on); + + $use_glyph = ($glyph_setting == $glyph_on); $title = parent::getTitle(); @@ -362,8 +361,8 @@ final class PhabricatorStandardPageView extends PhabricatorBarePageView if ($request) { $user = $request->getUser(); if ($user) { - $monospaced = $user->loadPreferences()->getPreference( - PhabricatorUserPreferences::PREFERENCE_MONOSPACED); + $monospaced = $user->getUserSetting( + PhabricatorMonospacedFontSetting::SETTINGKEY); } } @@ -834,7 +833,7 @@ final class PhabricatorStandardPageView extends PhabricatorBarePageView return $default; } - return $user->loadPreferences()->getPreference($key, $default); + return $user->getUserSetting($key); } public function produceAphrontResponse() { diff --git a/src/view/phui/calendar/PHUICalendarMonthView.php b/src/view/phui/calendar/PHUICalendarMonthView.php index 396947cdbe..f53ef72016 100644 --- a/src/view/phui/calendar/PHUICalendarMonthView.php +++ b/src/view/phui/calendar/PHUICalendarMonthView.php @@ -574,10 +574,10 @@ final class PHUICalendarMonthView extends AphrontView { } private function getWeekStartAndEnd() { - $preferences = $this->getViewer()->loadPreferences(); - $pref_week_start = PhabricatorUserPreferences::PREFERENCE_WEEK_START_DAY; + $viewer = $this->getViewer(); + $week_key = PhabricatorUserPreferences::PREFERENCE_WEEK_START_DAY; - $week_start = $preferences->getPreference($pref_week_start, 0); + $week_start = $viewer->getUserSetting($week_key); $week_end = ($week_start + 6) % 7; return array($week_start, $week_end); From ebd8f3c9874b0d4b6a7b0adf6fdcf421125f0fed Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 1 Jun 2016 16:17:30 -0700 Subject: [PATCH 21/24] Make translation, timezone and pronoun into real settings Summary: Ref T4103. These are currently stored on the user, for historic/performance reasons. Since I want administrators to be able to set defaults for translations and timezones at a minimum and there's no longer a meaningful performance penalty for moving them off the user record, turn them into real preferences and then nuke the columns. Test Plan: - Set settings to unusual values. - Ran migrations. - Verified my unusual settings survived. - Created a new user. - Edited all settings with old and new UIs. - Reconciled client/server timezone disagreement. Reviewers: chad Reviewed By: chad Maniphest Tasks: T4103 Differential Revision: https://secure.phabricator.com/D16005 --- .../20160601.user.02.copyprefs.php | 59 ++++++++++++ .../20160601.user.03.removetime.sql | 2 + .../20160601.user.04.removetranslation.sql | 2 + .../20160601.user.05.removesex.sql | 2 + src/__phutil_library_map__.php | 8 ++ .../__tests__/CalendarTimeUtilTestCase.php | 4 +- .../storage/PhabricatorMetaMTAMail.php | 27 ------ .../people/storage/PhabricatorUser.php | 72 +++++++++++---- .../PhabricatorSettingsTimezoneController.php | 11 ++- .../editor/PhabricatorSettingsEditEngine.php | 6 ++ .../panel/PhabricatorAccountSettingsPanel.php | 18 +++- .../PhabricatorDateTimeSettingsPanel.php | 8 +- .../setting/PhabricatorOptionGroupSetting.php | 73 +++++++++++++++ .../setting/PhabricatorPronounSetting.php | 35 +++++++ .../setting/PhabricatorSelectSetting.php | 8 +- .../setting/PhabricatorTimezoneSetting.php | 53 +++++++++++ .../setting/PhabricatorTranslationSetting.php | 92 +++++++++++++++++++ .../__tests__/PhabricatorTimeTestCase.php | 4 +- .../PhabricatorLocalTimeTestCase.php | 4 +- 19 files changed, 422 insertions(+), 66 deletions(-) create mode 100644 resources/sql/autopatches/20160601.user.02.copyprefs.php create mode 100644 resources/sql/autopatches/20160601.user.03.removetime.sql create mode 100644 resources/sql/autopatches/20160601.user.04.removetranslation.sql create mode 100644 resources/sql/autopatches/20160601.user.05.removesex.sql create mode 100644 src/applications/settings/setting/PhabricatorOptionGroupSetting.php create mode 100644 src/applications/settings/setting/PhabricatorPronounSetting.php create mode 100644 src/applications/settings/setting/PhabricatorTimezoneSetting.php create mode 100644 src/applications/settings/setting/PhabricatorTranslationSetting.php diff --git a/resources/sql/autopatches/20160601.user.02.copyprefs.php b/resources/sql/autopatches/20160601.user.02.copyprefs.php new file mode 100644 index 0000000000..9edbc11794 --- /dev/null +++ b/resources/sql/autopatches/20160601.user.02.copyprefs.php @@ -0,0 +1,59 @@ +establishConnection('w'); +$table_name = $table->getTableName(); +$prefs_table = new PhabricatorUserPreferences(); + +foreach (new LiskRawMigrationIterator($conn_w, $table_name) as $row) { + $phid = $row['phid']; + + $pref_row = queryfx_one( + $conn_w, + 'SELECT preferences FROM %T WHERE userPHID = %s', + $prefs_table->getTableName(), + $phid); + + if ($pref_row) { + try { + $prefs = phutil_json_decode($pref_row['preferences']); + } catch (Exception $ex) { + $prefs = array(); + } + } else { + $prefs = array(); + } + + $zone = $row['timezoneIdentifier']; + if (strlen($zone)) { + $prefs[PhabricatorTimezoneSetting::SETTINGKEY] = $zone; + } + + $pronoun = $row['sex']; + if (strlen($pronoun)) { + $prefs[PhabricatorPronounSetting::SETTINGKEY] = $pronoun; + } + + $translation = $row['translation']; + if (strlen($translation)) { + $prefs[PhabricatorTranslationSetting::SETTINGKEY] = $translation; + } + + if ($prefs) { + queryfx( + $conn_w, + 'INSERT INTO %T (phid, userPHID, preferences, dateModified, dateCreated) + VALUES (%s, %s, %s, UNIX_TIMESTAMP(), UNIX_TIMESTAMP()) + ON DUPLICATE KEY UPDATE preferences = VALUES(preferences)', + $prefs_table->getTableName(), + $prefs_table->generatePHID(), + $phid, + phutil_json_encode($prefs)); + } +} + +$prefs_key = PhabricatorUserPreferencesCacheType::KEY_PREFERENCES; +PhabricatorUserCache::clearCacheForAllUsers($prefs_key); diff --git a/resources/sql/autopatches/20160601.user.03.removetime.sql b/resources/sql/autopatches/20160601.user.03.removetime.sql new file mode 100644 index 0000000000..0ccaf77cd8 --- /dev/null +++ b/resources/sql/autopatches/20160601.user.03.removetime.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_user.user + DROP COLUMN timezoneIdentifier; diff --git a/resources/sql/autopatches/20160601.user.04.removetranslation.sql b/resources/sql/autopatches/20160601.user.04.removetranslation.sql new file mode 100644 index 0000000000..273223c317 --- /dev/null +++ b/resources/sql/autopatches/20160601.user.04.removetranslation.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_user.user + DROP COLUMN translation; diff --git a/resources/sql/autopatches/20160601.user.05.removesex.sql b/resources/sql/autopatches/20160601.user.05.removesex.sql new file mode 100644 index 0000000000..5b121f3864 --- /dev/null +++ b/resources/sql/autopatches/20160601.user.05.removesex.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_user.user + DROP COLUMN sex; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 259c5301bf..c83772e87e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2837,6 +2837,7 @@ phutil_register_library_map(array( 'PhabricatorOlderInlinesSetting' => 'applications/settings/setting/PhabricatorOlderInlinesSetting.php', 'PhabricatorOneTimeTriggerClock' => 'infrastructure/daemon/workers/clock/PhabricatorOneTimeTriggerClock.php', 'PhabricatorOpcodeCacheSpec' => 'applications/cache/spec/PhabricatorOpcodeCacheSpec.php', + 'PhabricatorOptionGroupSetting' => 'applications/settings/setting/PhabricatorOptionGroupSetting.php', 'PhabricatorOwnerPathQuery' => 'applications/owners/query/PhabricatorOwnerPathQuery.php', 'PhabricatorOwnersApplication' => 'applications/owners/application/PhabricatorOwnersApplication.php', 'PhabricatorOwnersArchiveController' => 'applications/owners/controller/PhabricatorOwnersArchiveController.php', @@ -3168,6 +3169,7 @@ phutil_register_library_map(array( 'PhabricatorProjectsSearchEngineAttachment' => 'applications/project/engineextension/PhabricatorProjectsSearchEngineAttachment.php', 'PhabricatorProjectsSearchEngineExtension' => 'applications/project/engineextension/PhabricatorProjectsSearchEngineExtension.php', 'PhabricatorProjectsWatchersSearchEngineAttachment' => 'applications/project/engineextension/PhabricatorProjectsWatchersSearchEngineAttachment.php', + 'PhabricatorPronounSetting' => 'applications/settings/setting/PhabricatorPronounSetting.php', 'PhabricatorProtocolAdapter' => 'infrastructure/daemon/bot/adapter/PhabricatorProtocolAdapter.php', 'PhabricatorPygmentSetupCheck' => 'applications/config/check/PhabricatorPygmentSetupCheck.php', 'PhabricatorQuery' => 'infrastructure/query/PhabricatorQuery.php', @@ -3535,6 +3537,7 @@ phutil_register_library_map(array( 'PhabricatorTimeFormatSetting' => 'applications/settings/setting/PhabricatorTimeFormatSetting.php', 'PhabricatorTimeGuard' => 'infrastructure/time/PhabricatorTimeGuard.php', 'PhabricatorTimeTestCase' => 'infrastructure/time/__tests__/PhabricatorTimeTestCase.php', + 'PhabricatorTimezoneSetting' => 'applications/settings/setting/PhabricatorTimezoneSetting.php', 'PhabricatorTimezoneSetupCheck' => 'applications/config/check/PhabricatorTimezoneSetupCheck.php', 'PhabricatorTitleGlyphsSetting' => 'applications/settings/setting/PhabricatorTitleGlyphsSetting.php', 'PhabricatorToken' => 'applications/tokens/storage/PhabricatorToken.php', @@ -3565,6 +3568,7 @@ phutil_register_library_map(array( 'PhabricatorTransactionsDestructionEngineExtension' => 'applications/transactions/engineextension/PhabricatorTransactionsDestructionEngineExtension.php', 'PhabricatorTransactionsFulltextEngineExtension' => 'applications/transactions/engineextension/PhabricatorTransactionsFulltextEngineExtension.php', 'PhabricatorTransformedFile' => 'applications/files/storage/PhabricatorTransformedFile.php', + 'PhabricatorTranslationSetting' => 'applications/settings/setting/PhabricatorTranslationSetting.php', 'PhabricatorTranslationsConfigOptions' => 'applications/config/option/PhabricatorTranslationsConfigOptions.php', 'PhabricatorTriggerAction' => 'infrastructure/daemon/workers/action/PhabricatorTriggerAction.php', 'PhabricatorTriggerClock' => 'infrastructure/daemon/workers/clock/PhabricatorTriggerClock.php', @@ -7455,6 +7459,7 @@ phutil_register_library_map(array( 'PhabricatorOlderInlinesSetting' => 'PhabricatorSelectSetting', 'PhabricatorOneTimeTriggerClock' => 'PhabricatorTriggerClock', 'PhabricatorOpcodeCacheSpec' => 'PhabricatorCacheSpec', + 'PhabricatorOptionGroupSetting' => 'PhabricatorSetting', 'PhabricatorOwnerPathQuery' => 'Phobject', 'PhabricatorOwnersApplication' => 'PhabricatorApplication', 'PhabricatorOwnersArchiveController' => 'PhabricatorOwnersController', @@ -7858,6 +7863,7 @@ phutil_register_library_map(array( 'PhabricatorProjectsSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment', 'PhabricatorProjectsSearchEngineExtension' => 'PhabricatorSearchEngineExtension', 'PhabricatorProjectsWatchersSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment', + 'PhabricatorPronounSetting' => 'PhabricatorSelectSetting', 'PhabricatorProtocolAdapter' => 'Phobject', 'PhabricatorPygmentSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorQuery' => 'Phobject', @@ -8294,6 +8300,7 @@ phutil_register_library_map(array( 'PhabricatorTimeFormatSetting' => 'PhabricatorSelectSetting', 'PhabricatorTimeGuard' => 'Phobject', 'PhabricatorTimeTestCase' => 'PhabricatorTestCase', + 'PhabricatorTimezoneSetting' => 'PhabricatorOptionGroupSetting', 'PhabricatorTimezoneSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorTitleGlyphsSetting' => 'PhabricatorSelectSetting', 'PhabricatorToken' => array( @@ -8329,6 +8336,7 @@ phutil_register_library_map(array( 'PhabricatorTransactionsDestructionEngineExtension' => 'PhabricatorDestructionEngineExtension', 'PhabricatorTransactionsFulltextEngineExtension' => 'PhabricatorFulltextEngineExtension', 'PhabricatorTransformedFile' => 'PhabricatorFileDAO', + 'PhabricatorTranslationSetting' => 'PhabricatorOptionGroupSetting', 'PhabricatorTranslationsConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorTriggerAction' => 'Phobject', 'PhabricatorTriggerClock' => 'Phobject', diff --git a/src/applications/calendar/__tests__/CalendarTimeUtilTestCase.php b/src/applications/calendar/__tests__/CalendarTimeUtilTestCase.php index f5946ea32b..aa44acec70 100644 --- a/src/applications/calendar/__tests__/CalendarTimeUtilTestCase.php +++ b/src/applications/calendar/__tests__/CalendarTimeUtilTestCase.php @@ -4,7 +4,7 @@ final class CalendarTimeUtilTestCase extends PhabricatorTestCase { public function testTimestampsAtMidnight() { $u = new PhabricatorUser(); - $u->setTimezoneIdentifier('America/Los_Angeles'); + $u->overrideTimezoneIdentifier('America/Los_Angeles'); $days = $this->getAllDays(); foreach ($days as $day) { $data = CalendarTimeUtil::getCalendarWidgetTimestamps( @@ -19,7 +19,7 @@ final class CalendarTimeUtilTestCase extends PhabricatorTestCase { public function testTimestampsStartDay() { $u = new PhabricatorUser(); - $u->setTimezoneIdentifier('America/Los_Angeles'); + $u->overrideTimezoneIdentifier('America/Los_Angeles'); $days = $this->getAllDays(); foreach ($days as $day) { $data = CalendarTimeUtil::getTimestamps( diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index 95740ecbd4..9fb84aaa4f 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -164,33 +164,6 @@ final class PhabricatorMetaMTAMail return $this->getParam('herald-force-recipients', array()); } - public function getTranslation(array $objects) { - $default_translation = PhabricatorEnv::getEnvConfig('translation.provider'); - $return = null; - $recipients = array_merge( - idx($this->parameters, 'to', array()), - idx($this->parameters, 'cc', array())); - foreach (array_select_keys($objects, $recipients) as $object) { - $translation = null; - if ($object instanceof PhabricatorUser) { - $translation = $object->getTranslation(); - } - if (!$translation) { - $translation = $default_translation; - } - if ($return && $translation != $return) { - return $default_translation; - } - $return = $translation; - } - - if (!$return) { - $return = $default_translation; - } - - return $return; - } - public function addPHIDHeaders($name, array $phids) { $phids = array_unique($phids); foreach ($phids as $phid) { diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 0c19b3acdd..c0559b7e7d 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -5,6 +5,7 @@ * @task image-cache Profile Image Cache * @task factors Multi-Factor Authentication * @task handles Managing Handles + * @task settings Settings * @task cache User Cache */ final class PhabricatorUser @@ -26,15 +27,12 @@ final class PhabricatorUser protected $userName; protected $realName; - protected $sex; - protected $translation; protected $passwordSalt; protected $passwordHash; protected $profileImagePHID; protected $profileImageCache; protected $availabilityCache; protected $availabilityCacheTTL; - protected $timezoneIdentifier = ''; protected $consoleEnabled = 0; protected $consoleVisible = 0; @@ -68,14 +66,10 @@ final class PhabricatorUser private $authorities = array(); private $handlePool; private $csrfSalt; + private $timezoneOverride; protected function readField($field) { switch ($field) { - case 'timezoneIdentifier': - // If the user hasn't set one, guess the server's time. - return nonempty( - $this->timezoneIdentifier, - date_default_timezone_get()); // Make sure these return booleans. case 'isAdmin': return (bool)$this->isAdmin; @@ -191,8 +185,6 @@ final class PhabricatorUser self::CONFIG_COLUMN_SCHEMA => array( 'userName' => 'sort64', 'realName' => 'text128', - 'sex' => 'text4?', - 'translation' => 'text64?', 'passwordSalt' => 'text32?', 'passwordHash' => 'text128?', 'profileImagePHID' => 'phid?', @@ -204,7 +196,6 @@ final class PhabricatorUser 'isMailingList' => 'bool', 'isDisabled' => 'bool', 'isAdmin' => 'bool', - 'timezoneIdentifier' => 'text255', 'isEmailVerified' => 'uint32', 'isApproved' => 'uint32', 'accountSecret' => 'bytes64', @@ -261,11 +252,6 @@ final class PhabricatorUser return $this; } - // To satisfy PhutilPerson. - public function getSex() { - return $this->sex; - } - public function getMonogram() { return '@'.$this->getUsername(); } @@ -490,6 +476,10 @@ final class PhabricatorUser '(isPrimary = 1)'); } + +/* -( Settings )----------------------------------------------------------- */ + + public function getUserSetting($key) { $settings_key = PhabricatorUserPreferencesCacheType::KEY_PREFERENCES; $settings = $this->requireCacheData($settings_key); @@ -506,11 +496,51 @@ final class PhabricatorUser return null; } + + /** + * Test if a given setting is set to a particular value. + * + * @param const Setting key. + * @param wild Value to compare. + * @return bool True if the setting has the specified value. + * @task settings + */ public function compareUserSetting($key, $value) { $actual = $this->getUserSetting($key); return ($actual == $value); } + public function getTranslation() { + return $this->getUserSetting(PhabricatorTranslationSetting::SETTINGKEY); + } + + public function getTimezoneIdentifier() { + if ($this->timezoneOverride) { + return $this->timezoneOverride; + } + + return $this->getUserSetting(PhabricatorTimezoneSetting::SETTINGKEY); + } + + + /** + * Override the user's timezone identifier. + * + * This is primarily useful for unit tests. + * + * @param string New timezone identifier. + * @return this + * @task settings + */ + public function overrideTimezoneIdentifier($identifier) { + $this->timezoneOverride = $identifier; + return $this; + } + + public function getSex() { + return $this->getUserSetting(PhabricatorPronounSetting::SETTINGKEY); + } + public function loadPreferences() { if ($this->preferences) { return $this->preferences; @@ -1539,4 +1569,14 @@ final class PhabricatorUser return $usable_value; } + + /** + * @task cache + */ + public function clearCacheData($key) { + unset($this->rawCacheData[$key]); + unset($this->usableCacheData[$key]); + return $this; + } + } diff --git a/src/applications/settings/controller/PhabricatorSettingsTimezoneController.php b/src/applications/settings/controller/PhabricatorSettingsTimezoneController.php index a637401f1f..6b5fb39f22 100644 --- a/src/applications/settings/controller/PhabricatorSettingsTimezoneController.php +++ b/src/applications/settings/controller/PhabricatorSettingsTimezoneController.php @@ -31,6 +31,7 @@ final class PhabricatorSettingsTimezoneController $timezone = $request->getStr('timezone'); $pref_ignore = PhabricatorUserPreferences::PREFERENCE_IGNORE_OFFSET; + $pref_timezone = PhabricatorTimezoneSetting::SETTINGKEY; $preferences = $viewer->loadPreferences(); @@ -52,11 +53,11 @@ final class PhabricatorSettingsTimezoneController if (isset($options[$timezone])) { $preferences ->setPreference($pref_ignore, null) + ->setPreference($pref_timezone, $timezone) ->save(); - $viewer - ->setTimezoneIdentifier($timezone) - ->save(); + $viewer->clearCacheData( + PhabricatorUserPreferencesCacheType::KEY_PREFERENCES); } } @@ -115,9 +116,9 @@ final class PhabricatorSettingsTimezoneController $offset = $offset / 60; if ($offset >= 0) { - return pht('GMT-%d', $offset); + return pht('UTC-%d', $offset); } else { - return pht('GMT+%d', -$offset); + return pht('UTC+%d', -$offset); } } diff --git a/src/applications/settings/editor/PhabricatorSettingsEditEngine.php b/src/applications/settings/editor/PhabricatorSettingsEditEngine.php index 0db42708fb..5b077b9485 100644 --- a/src/applications/settings/editor/PhabricatorSettingsEditEngine.php +++ b/src/applications/settings/editor/PhabricatorSettingsEditEngine.php @@ -78,6 +78,12 @@ final class PhabricatorSettingsEditEngine $viewer = $this->getViewer(); $settings = PhabricatorSetting::getAllEnabledSettings($viewer); + foreach ($settings as $key => $setting) { + $setting = clone $setting; + $setting->setViewer($viewer); + $settings[$key] = $setting; + } + $fields = array(); foreach ($settings as $setting) { foreach ($setting->newCustomEditFields($object) as $field) { diff --git a/src/applications/settings/panel/PhabricatorAccountSettingsPanel.php b/src/applications/settings/panel/PhabricatorAccountSettingsPanel.php index 274e44f15e..195971d8a7 100644 --- a/src/applications/settings/panel/PhabricatorAccountSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorAccountSettingsPanel.php @@ -23,21 +23,29 @@ final class PhabricatorAccountSettingsPanel extends PhabricatorSettingsPanel { $user = $this->getUser(); $username = $user->getUsername(); + $preferences = $user->loadPreferences(); + $errors = array(); if ($request->isFormPost()) { $sex = $request->getStr('sex'); $sexes = array(PhutilPerson::SEX_MALE, PhutilPerson::SEX_FEMALE); if (in_array($sex, $sexes)) { - $user->setSex($sex); + $new_value = $sex; } else { - $user->setSex(null); + $new_value = null; } - // Checked in runtime. - $user->setTranslation($request->getStr('translation')); + $preferences->setPreference( + PhabricatorPronounSetting::SETTINGKEY, + $new_value); + + $preferences->setPreference( + PhabricatorTranslationSetting::SETTINGKEY, + $request->getStr('translation')); if (!$errors) { - $user->save(); + $preferences->save(); + return id(new AphrontRedirectResponse()) ->setURI($this->getPanelURI('?saved=true')); } diff --git a/src/applications/settings/panel/PhabricatorDateTimeSettingsPanel.php b/src/applications/settings/panel/PhabricatorDateTimeSettingsPanel.php index 6628f21187..6c21a88894 100644 --- a/src/applications/settings/panel/PhabricatorDateTimeSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorDateTimeSettingsPanel.php @@ -18,6 +18,7 @@ final class PhabricatorDateTimeSettingsPanel extends PhabricatorSettingsPanel { $user = $request->getUser(); $username = $user->getUsername(); + $pref_timezone = PhabricatorTimezoneSetting::SETTINGKEY; $pref_time = PhabricatorUserPreferences::PREFERENCE_TIME_FORMAT; $pref_date = PhabricatorUserPreferences::PREFERENCE_DATE_FORMAT; $pref_week_start = PhabricatorUserPreferences::PREFERENCE_WEEK_START_DAY; @@ -27,13 +28,12 @@ final class PhabricatorDateTimeSettingsPanel extends PhabricatorSettingsPanel { $errors = array(); if ($request->isFormPost()) { $new_timezone = $request->getStr('timezone'); - if (in_array($new_timezone, DateTimeZone::listIdentifiers(), true)) { - $user->setTimezoneIdentifier($new_timezone); - } else { + if (!in_array($new_timezone, DateTimeZone::listIdentifiers(), true)) { $errors[] = pht('The selected timezone is not a valid timezone.'); } $preferences + ->setPreference($pref_timezone, $new_timezone) ->setPreference( $pref_time, $request->getStr($pref_time)) @@ -47,7 +47,7 @@ final class PhabricatorDateTimeSettingsPanel extends PhabricatorSettingsPanel { if (!$errors) { $preferences->save(); - $user->save(); + return id(new AphrontRedirectResponse()) ->setURI($this->getPanelURI('?saved=true')); } diff --git a/src/applications/settings/setting/PhabricatorOptionGroupSetting.php b/src/applications/settings/setting/PhabricatorOptionGroupSetting.php new file mode 100644 index 0000000000..645b6bc2e1 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorOptionGroupSetting.php @@ -0,0 +1,73 @@ +getSelectOptionGroups(); + + $map = array(); + foreach ($groups as $group) { + $map += $group['options']; + } + + return $map; + } + + final protected function newCustomEditField($object) { + $setting_key = $this->getSettingKey(); + $default_value = $object->getDefaultValue($setting_key); + + $options = $this->getSelectOptionGroups(); + + $map = $this->getSelectOptionMap(); + if (isset($map[$default_value])) { + $default_label = pht('Default (%s)', $map[$default_value]); + } else { + $default_label = pht('Default (Unknown, "%s")', $default_value); + } + + $head_key = head_key($options); + $options[$head_key]['options'] = array( + '' => $default_label, + ) + $options[$head_key]['options']; + + $flat_options = array(); + foreach ($options as $group) { + $flat_options[$group['label']] = $group['options']; + } + + return $this->newEditField($object, new PhabricatorSelectEditField()) + ->setOptions($flat_options); + } + + final public function validateTransactionValue($value) { + if (!strlen($value)) { + return; + } + + $map = $this->getSelectOptionMap(); + + if (!isset($map[$value])) { + throw new Exception( + pht( + 'Value "%s" is not valid for setting "%s": valid values are %s.', + $value, + $this->getSettingName(), + implode(', ', array_keys($map)))); + } + + return; + } + + public function getTransactionNewValue($value) { + if (!strlen($value)) { + return null; + } + + return (string)$value; + } + +} diff --git a/src/applications/settings/setting/PhabricatorPronounSetting.php b/src/applications/settings/setting/PhabricatorPronounSetting.php new file mode 100644 index 0000000000..b1fad77ed0 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorPronounSetting.php @@ -0,0 +1,35 @@ +getViewer(); + $username = $viewer->getUsername(); + + $label_unknown = pht('%s updated their profile', $username); + $label_her = pht('%s updated her profile', $username); + $label_his = pht('%s updated his profile', $username); + + return array( + PhutilPerson::SEX_UNKNOWN => $label_unknown, + PhutilPerson::SEX_MALE => $label_his, + PhutilPerson::SEX_FEMALE => $label_her, + ); + } + +} diff --git a/src/applications/settings/setting/PhabricatorSelectSetting.php b/src/applications/settings/setting/PhabricatorSelectSetting.php index 5dc7323c84..44369baa46 100644 --- a/src/applications/settings/setting/PhabricatorSelectSetting.php +++ b/src/applications/settings/setting/PhabricatorSelectSetting.php @@ -17,9 +17,11 @@ abstract class PhabricatorSelectSetting $default_label = pht('Default (Unknown, "%s")', $default_value); } - $options = array( - '' => $default_label, - ) + $options; + if (empty($options[''])) { + $options = array( + '' => $default_label, + ) + $options; + } return $this->newEditField($object, new PhabricatorSelectEditField()) ->setOptions($options); diff --git a/src/applications/settings/setting/PhabricatorTimezoneSetting.php b/src/applications/settings/setting/PhabricatorTimezoneSetting.php new file mode 100644 index 0000000000..47d889900e --- /dev/null +++ b/src/applications/settings/setting/PhabricatorTimezoneSetting.php @@ -0,0 +1,53 @@ +getOffset($now) / (60 * 60)); + $groups[$offset][] = $timezone; + } + + krsort($groups); + + $option_groups = array( + array( + 'label' => pht('Default'), + 'options' => array(), + ), + ); + + foreach ($groups as $offset => $group) { + if ($offset >= 0) { + $label = pht('UTC-%d', $offset); + } else { + $label = pht('UTC+%d', -$offset); + } + + sort($group); + $option_groups[] = array( + 'label' => $label, + 'options' => array_fuse($group), + ); + } + + return $option_groups; + } + +} diff --git a/src/applications/settings/setting/PhabricatorTranslationSetting.php b/src/applications/settings/setting/PhabricatorTranslationSetting.php new file mode 100644 index 0000000000..dd0ba95a66 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorTranslationSetting.php @@ -0,0 +1,92 @@ + pht('Translations'), + 'limited' => pht('Limited Translations'), + 'silly' => pht('Silly Translations'), + 'test' => pht('Developer/Test Translations'), + ); + + $groups = array_fill_keys(array_keys($group_labels), array()); + + $translations = array(); + foreach ($locales as $locale) { + $code = $locale->getLocaleCode(); + + // Get the locale's localized name if it's available. For example, + // "Deutsch" instead of "German". This helps users who do not speak the + // current language to find the correct setting. + $raw_scope = PhabricatorEnv::beginScopedLocale($code); + $name = $locale->getLocaleName(); + unset($raw_scope); + + if ($locale->isSillyLocale()) { + if ($is_serious) { + // Omit silly locales on serious business installs. + continue; + } + $groups['silly'][$code] = $name; + continue; + } + + if ($locale->isTestLocale()) { + $groups['test'][$code] = $name; + continue; + } + + $strings = PhutilTranslation::getTranslationMapForLocale($code); + $size = count($strings); + + // If a translation is English, assume it can fall back to the default + // strings and don't caveat its completeness. + $is_english = (substr($code, 0, 3) == 'en_'); + + // Arbitrarily pick some number of available strings to promote a + // translation out of the "limited" group. The major goal is just to + // keep locales with very few strings out of the main group, so users + // aren't surprised if a locale has no upstream translations available. + if ($size > 512 || $is_english) { + $type = 'normal'; + } else { + $type = 'limited'; + } + + $groups[$type][$code] = $name; + } + + $results = array(); + foreach ($groups as $key => $group) { + $label = $group_labels[$key]; + if (!$group) { + continue; + } + + asort($group); + + $results[] = array( + 'label' => $label, + 'options' => $group, + ); + } + + return $results; + } + +} diff --git a/src/infrastructure/time/__tests__/PhabricatorTimeTestCase.php b/src/infrastructure/time/__tests__/PhabricatorTimeTestCase.php index ef2db5cd66..4c7b4a2f94 100644 --- a/src/infrastructure/time/__tests__/PhabricatorTimeTestCase.php +++ b/src/infrastructure/time/__tests__/PhabricatorTimeTestCase.php @@ -15,10 +15,10 @@ final class PhabricatorTimeTestCase extends PhabricatorTestCase { public function testParseLocalTime() { $u = new PhabricatorUser(); - $u->setTimezoneIdentifier('UTC'); + $u->overrideTimezoneIdentifier('UTC'); $v = new PhabricatorUser(); - $v->setTimezoneIdentifier('America/Los_Angeles'); + $v->overrideTimezoneIdentifier('America/Los_Angeles'); $t = 1370202281; // 2013-06-02 12:44:41 -0700 $time = PhabricatorTime::pushTime($t, 'America/Los_Angeles'); diff --git a/src/view/__tests__/PhabricatorLocalTimeTestCase.php b/src/view/__tests__/PhabricatorLocalTimeTestCase.php index f9fd31ad9a..b007197834 100644 --- a/src/view/__tests__/PhabricatorLocalTimeTestCase.php +++ b/src/view/__tests__/PhabricatorLocalTimeTestCase.php @@ -4,10 +4,10 @@ final class PhabricatorLocalTimeTestCase extends PhabricatorTestCase { public function testLocalTimeFormatting() { $user = new PhabricatorUser(); - $user->setTimezoneIdentifier('America/Los_Angeles'); + $user->overrideTimezoneIdentifier('America/Los_Angeles'); $utc = new PhabricatorUser(); - $utc->setTimezoneIdentifier('UTC'); + $utc->overrideTimezoneIdentifier('UTC'); $this->assertEqual( 'Jan 1 2000, 12:00 AM', From 24acac117b9b2371dc794efb09b6a9d2e6456aab Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 2 Jun 2016 06:41:12 -0700 Subject: [PATCH 22/24] Consider identifier types when sorting clone URIs Summary: Fixes T11082. Currently, the `/123/` and `/CALLSIGN/` versions of the URI get the same score. Also the scores are backwards. Test Plan: - Added `getPublicCloneURI()` output to repository listing. - Before patch, saw a repository with a callsign list a less-preferred ID-based URI. - After patch, saw the repository list the more-preferred callsign-based URI. Reviewers: chad Reviewed By: chad Subscribers: nikolay.metchev Maniphest Tasks: T11082 Differential Revision: https://secure.phabricator.com/D16008 --- .../storage/PhabricatorRepository.php | 5 ++++- .../storage/PhabricatorRepositoryURI.php | 17 ++++++++++++----- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index c95a4361e1..321c1ff7af 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -2061,7 +2061,10 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $clone[] = $uri; } - return msort($clone, 'getURIScore'); + $clone = msort($clone, 'getURIScore'); + $clone = array_reverse($clone); + + return $clone; } diff --git a/src/applications/repository/storage/PhabricatorRepositoryURI.php b/src/applications/repository/storage/PhabricatorRepositoryURI.php index f0326f12a9..d9dcea0a2d 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryURI.php +++ b/src/applications/repository/storage/PhabricatorRepositoryURI.php @@ -598,18 +598,25 @@ final class PhabricatorRepositoryURI $score = 0; $io_points = array( - self::IO_READWRITE => 20, - self::IO_READ => 10, + self::IO_READWRITE => 200, + self::IO_READ => 100, ); $score += idx($io_points, $this->getEffectiveIoType(), 0); $protocol_points = array( - self::BUILTIN_PROTOCOL_SSH => 3, - self::BUILTIN_PROTOCOL_HTTPS => 2, - self::BUILTIN_PROTOCOL_HTTP => 1, + self::BUILTIN_PROTOCOL_SSH => 30, + self::BUILTIN_PROTOCOL_HTTPS => 20, + self::BUILTIN_PROTOCOL_HTTP => 10, ); $score += idx($protocol_points, $this->getBuiltinProtocol(), 0); + $identifier_points = array( + self::BUILTIN_IDENTIFIER_SHORTNAME => 3, + self::BUILTIN_IDENTIFIER_CALLSIGN => 2, + self::BUILTIN_IDENTIFIER_ID => 1, + ); + $score += idx($identifier_points, $this->getBuiltinIdentifier(), 0); + return $score; } From 03e54afc146c6ed36927f540dd6e40eda681214d Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 2 Jun 2016 07:15:39 -0700 Subject: [PATCH 23/24] Give Phame blogs an explicit 404 controller Summary: Ref T11076. Ref T9897. Bad links on Phame blogs are currently made worse because we try to prompt you to login on a non-cookie domain. Instead, just 404 in a vanilla way. Do so cleanly on external domains. Test Plan: {F1672399} Reviewers: chad Reviewed By: chad Maniphest Tasks: T9897, T11076 Differential Revision: https://secure.phabricator.com/D16010 --- resources/celerity/map.php | 4 +- src/__phutil_library_map__.php | 4 ++ .../PhabricatorPhameApplication.php | 2 + .../phame/controller/PhameLiveController.php | 33 +++++++++++++- .../blog/PhameBlog404Controller.php | 14 ++++++ .../blog/PhameBlogViewController.php | 6 --- .../phame/site/Phame404Response.php | 43 +++++++++++++++++++ webroot/rsrc/css/application/phame/phame.css | 10 +++++ 8 files changed, 106 insertions(+), 10 deletions(-) create mode 100644 src/applications/phame/controller/blog/PhameBlog404Controller.php create mode 100644 src/applications/phame/site/Phame404Response.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index e462f6d017..c2bee18ab6 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -81,7 +81,7 @@ return array( 'rsrc/css/application/owners/owners-path-editor.css' => '2f00933b', 'rsrc/css/application/paste/paste.css' => '1898e534', 'rsrc/css/application/people/people-profile.css' => '2473d929', - 'rsrc/css/application/phame/phame.css' => '737792d6', + 'rsrc/css/application/phame/phame.css' => '7448a969', 'rsrc/css/application/pholio/pholio-edit.css' => 'b15fec4a', 'rsrc/css/application/pholio/pholio-inline-comments.css' => '8e545e49', 'rsrc/css/application/pholio/pholio.css' => 'ca89d380', @@ -806,7 +806,7 @@ return array( 'phabricator-uiexample-reactor-sendclass' => '1def2711', 'phabricator-uiexample-reactor-sendproperties' => 'b1f0ccee', 'phabricator-zindex-css' => '5b6fcf3f', - 'phame-css' => '737792d6', + 'phame-css' => '7448a969', 'pholio-css' => 'ca89d380', 'pholio-edit-css' => 'b15fec4a', 'pholio-inline-comments-css' => '8e545e49', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index c83772e87e..2533344e43 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3708,7 +3708,9 @@ phutil_register_library_map(array( 'PhabricatorXHProfSample' => 'applications/xhprof/storage/PhabricatorXHProfSample.php', 'PhabricatorXHProfSampleListController' => 'applications/xhprof/controller/PhabricatorXHProfSampleListController.php', 'PhabricatorYoutubeRemarkupRule' => 'infrastructure/markup/rule/PhabricatorYoutubeRemarkupRule.php', + 'Phame404Response' => 'applications/phame/site/Phame404Response.php', 'PhameBlog' => 'applications/phame/storage/PhameBlog.php', + 'PhameBlog404Controller' => 'applications/phame/controller/blog/PhameBlog404Controller.php', 'PhameBlogArchiveController' => 'applications/phame/controller/blog/PhameBlogArchiveController.php', 'PhameBlogController' => 'applications/phame/controller/blog/PhameBlogController.php', 'PhameBlogCreateCapability' => 'applications/phame/capability/PhameBlogCreateCapability.php', @@ -8508,6 +8510,7 @@ phutil_register_library_map(array( 'PhabricatorXHProfSample' => 'PhabricatorXHProfDAO', 'PhabricatorXHProfSampleListController' => 'PhabricatorXHProfController', 'PhabricatorYoutubeRemarkupRule' => 'PhutilRemarkupRule', + 'Phame404Response' => 'AphrontHTMLResponse', 'PhameBlog' => array( 'PhameDAO', 'PhabricatorPolicyInterface', @@ -8519,6 +8522,7 @@ phutil_register_library_map(array( 'PhabricatorApplicationTransactionInterface', 'PhabricatorConduitResultInterface', ), + 'PhameBlog404Controller' => 'PhameLiveController', 'PhameBlogArchiveController' => 'PhameBlogController', 'PhameBlogController' => 'PhameController', 'PhameBlogCreateCapability' => 'PhabricatorPolicyCapability', diff --git a/src/applications/phame/application/PhabricatorPhameApplication.php b/src/applications/phame/application/PhabricatorPhameApplication.php index 30d1b9cb3d..9138961214 100644 --- a/src/applications/phame/application/PhabricatorPhameApplication.php +++ b/src/applications/phame/application/PhabricatorPhameApplication.php @@ -93,7 +93,9 @@ final class PhabricatorPhameApplication extends PhabricatorApplication { '/' => array( '' => 'PhameBlogViewController', 'post/(?P\d+)/(?:(?P[^/]+)/)?' => 'PhamePostViewController', + '.*' => 'PhameBlog404Controller', ), + ); } diff --git a/src/applications/phame/controller/PhameLiveController.php b/src/applications/phame/controller/PhameLiveController.php index da7fb6da4b..57ac875465 100644 --- a/src/applications/phame/controller/PhameLiveController.php +++ b/src/applications/phame/controller/PhameLiveController.php @@ -70,6 +70,8 @@ abstract class PhameLiveController extends PhameController { $blog = $blog_query->executeOne(); if (!$blog) { + $this->isExternal = $is_external; + $this->isLive = $is_live; return new Aphront404Response(); } @@ -81,6 +83,9 @@ abstract class PhameLiveController extends PhameController { $blog = null; } + $this->isExternal = $is_external; + $this->isLive = $is_live; + if ($post_id) { $post_query = id(new PhamePostQuery()) ->setViewer($viewer) @@ -109,8 +114,6 @@ abstract class PhameLiveController extends PhameController { $post = null; } - $this->isExternal = $is_external; - $this->isLive = $is_live; $this->blog = $blog; $this->post = $post; @@ -188,4 +191,30 @@ abstract class PhameLiveController extends PhameController { return $crumbs; } + public function willSendResponse(AphrontResponse $response) { + if ($this->getIsExternal()) { + if ($response instanceof Aphront404Response) { + $page = $this->newPage() + ->setCrumbs($this->buildApplicationCrumbs()); + + $response = id(new Phame404Response()) + ->setPage($page); + } + } + + return parent::willSendResponse($response); + } + + public function newPage() { + $page = parent::newPage(); + + if ($this->getIsLive()) { + $page + ->setShowChrome(false) + ->setShowFooter(false); + } + + return $page; + } + } diff --git a/src/applications/phame/controller/blog/PhameBlog404Controller.php b/src/applications/phame/controller/blog/PhameBlog404Controller.php new file mode 100644 index 0000000000..994c4d6297 --- /dev/null +++ b/src/applications/phame/controller/blog/PhameBlog404Controller.php @@ -0,0 +1,14 @@ +setupLiveEnvironment(); + if ($response) { + return $response; + } + + return new Aphront404Response(); + } + +} diff --git a/src/applications/phame/controller/blog/PhameBlogViewController.php b/src/applications/phame/controller/blog/PhameBlogViewController.php index ea0822b856..da027d584b 100644 --- a/src/applications/phame/controller/blog/PhameBlogViewController.php +++ b/src/applications/phame/controller/blog/PhameBlogViewController.php @@ -108,12 +108,6 @@ final class PhameBlogViewController extends PhameLiveController { $about, )); - if ($is_live) { - $page - ->setShowChrome(false) - ->setShowFooter(false); - } - return $page; } diff --git a/src/applications/phame/site/Phame404Response.php b/src/applications/phame/site/Phame404Response.php new file mode 100644 index 0000000000..f511a0f9d9 --- /dev/null +++ b/src/applications/phame/site/Phame404Response.php @@ -0,0 +1,43 @@ +page = $page; + return $this; + } + + public function getPage() { + return $this->page; + } + + public function getHTTPResponseCode() { + return 404; + } + + public function buildResponseString() { + require_celerity_resource('phame-css'); + + $not_found = phutil_tag( + 'div', + array( + 'class' => 'phame-404', + ), + array( + phutil_tag('strong', array(), pht('404 Not Found')), + phutil_tag('br'), + pht('Wherever you go, there you are.'), + phutil_tag('br'), + pht('But the page you seek is elsewhere.'), + )); + + $page = $this->getPage() + ->setTitle(pht('404 Not Found')) + ->appendChild($not_found); + + return $page->render(); + } + +} diff --git a/webroot/rsrc/css/application/phame/phame.css b/webroot/rsrc/css/application/phame/phame.css index d4fdeafff0..61c3ce4361 100644 --- a/webroot/rsrc/css/application/phame/phame.css +++ b/webroot/rsrc/css/application/phame/phame.css @@ -243,3 +243,13 @@ left: 50px; position: absolute; } + +.phame-404 { + margin: 48px auto; + padding: 12px 24px; + border-radius: 6px; + min-width: 240px; + width: 50%; + color: {$darkgreytext}; + background: {$greybackground}; +} From a34b769b4fea63371cd34a8c36bf1064298675c0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 3 Jun 2016 13:33:43 -0700 Subject: [PATCH 24/24] Prevent creation of inline comments with mismatched changesetID / revisionPHID Summary: Ref T11092. With Quicksand (or, possibly, some as-yet-unknown non-Quicksand workflow) the client can get stuck with an out-of-date revision PHID. We then save comments with a `revisionPHID` from one revision and a `changesetID` from a different one. Detect and prevent this. This stops the workflow immediately when the use first clicks, so it should allow us to detect this issue if it has some other non-Quicksand cause. Test Plan: - Opened revision `D123`. - Pressed `\` to enable the sidebar and Quicksand. - Clicked a link to revision `D124`. - Added inlines. Previously, these could ghost. The exact UI behavior is difficult to describe, but in the database they end up with a `changesetID` for `D124` but the original `revisionPHID` for `D123`, presumably because state is sticking around from the first page. After this patch, an exception is thrown immediately. Additionally: - Reloaded to clear quicksand state, added comments fine. - Disabled sidebar/quicksand, added comments fine. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11092 Differential Revision: https://secure.phabricator.com/D16031 --- ...ifferentialInlineCommentEditController.php | 27 ++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/applications/differential/controller/DifferentialInlineCommentEditController.php b/src/applications/differential/controller/DifferentialInlineCommentEditController.php index 9e1f28670f..11c91fde10 100644 --- a/src/applications/differential/controller/DifferentialInlineCommentEditController.php +++ b/src/applications/differential/controller/DifferentialInlineCommentEditController.php @@ -23,13 +23,34 @@ final class DifferentialInlineCommentEditController } protected function createComment() { - // Verify revision and changeset correspond to actual objects. + // Verify revision and changeset correspond to actual objects, and are + // connected to one another. $changeset_id = $this->getChangesetID(); + $viewer = $this->getViewer(); $revision = $this->loadRevision(); - if (!id(new DifferentialChangeset())->load($changeset_id)) { - throw new Exception(pht('Invalid changeset ID!')); + $changeset = id(new DifferentialChangesetQuery()) + ->setViewer($viewer) + ->withIDs(array($changeset_id)) + ->executeOne(); + if (!$changeset) { + throw new Exception( + pht( + 'Invalid changeset ID "%s"!', + $changeset_id)); + } + + $diff = $changeset->getDiff(); + if ($diff->getRevisionID() != $revision->getID()) { + throw new Exception( + pht( + 'Changeset ID "%s" is part of diff ID "%s", but that diff '. + 'is attached to reivsion "%s", not revision "%s".', + $changeset_id, + $diff->getID(), + $diff->getRevisionID(), + $revision->getID())); } return id(new DifferentialInlineComment())