diff --git a/resources/sql/autopatches/20141107.phriction.policy.2.php b/resources/sql/autopatches/20141107.phriction.policy.2.php --- a/resources/sql/autopatches/20141107.phriction.policy.2.php +++ b/resources/sql/autopatches/20141107.phriction.policy.2.php @@ -24,12 +24,14 @@ $prefix = 'projects/'; if (($slug != $prefix) && (strncmp($slug, $prefix, strlen($prefix)) === 0)) { $parts = explode('/', $slug); - $project_slug = $parts[1].'/'; + + $project_slug = $parts[1]; + $project_slug = PhabricatorSlug::normalizeProjectSlug($project_slug); $project_slugs = array($project_slug); $project = id(new PhabricatorProjectQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withPhrictionSlugs($project_slugs) + ->withSlugs($project_slugs) ->executeOne(); if ($project) { diff --git a/resources/sql/patches/090.forceuniqueprojectnames.php b/resources/sql/patches/090.forceuniqueprojectnames.php --- a/resources/sql/patches/090.forceuniqueprojectnames.php +++ b/resources/sql/patches/090.forceuniqueprojectnames.php @@ -10,13 +10,14 @@ $slug_map = array(); foreach ($projects as $project) { - $project->setPhrictionSlug($project->getName()); - $slug = $project->getPhrictionSlug(); - if ($slug == '/') { + $slug = PhabricatorSlug::normalizeProjectSlug($project->getName()); + + if (!strlen($slug)) { $project_id = $project->getID(); echo pht("Project #%d doesn't have a meaningful name...", $project_id)."\n"; $project->setName(trim(pht('Unnamed Project %s', $project->getName()))); } + $slug_map[$slug][] = $project->getID(); } @@ -47,8 +48,8 @@ foreach ($update as $key => $project) { $id = $project->getID(); $name = $project->getName(); - $project->setPhrictionSlug($name); - $slug = $project->getPhrictionSlug(); + + $slug = PhabricatorSlug::normalizeProjectSlug($name).'/'; echo pht("Updating project #%d '%s' (%s)... ", $id, $name, $slug); try { @@ -87,8 +88,8 @@ $suffix = 2; while (true) { $new_name = $project->getName().' ('.$suffix.')'; - $project->setPhrictionSlug($new_name); - $new_slug = $project->getPhrictionSlug(); + + $new_slug = PhabricatorSlug::normalizeProjectSlug($new_name).'/'; $okay = true; foreach ($projects as $other) { diff --git a/src/applications/project/conduit/ProjectQueryConduitAPIMethod.php b/src/applications/project/conduit/ProjectQueryConduitAPIMethod.php --- a/src/applications/project/conduit/ProjectQueryConduitAPIMethod.php +++ b/src/applications/project/conduit/ProjectQueryConduitAPIMethod.php @@ -112,7 +112,7 @@ $slug_map = array(); if ($slugs) { foreach ($slugs as $slug) { - $normal = rtrim(PhabricatorSlug::normalize($slug), '/'); + $normal = PhabricatorSlug::normalizeProjectSlug($slug); foreach ($projects as $project) { if (in_array($normal, $project['slugs'])) { $slug_map[$slug] = $project['phid']; diff --git a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php --- a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php +++ b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php @@ -81,9 +81,9 @@ switch ($xaction->getTransactionType()) { case PhabricatorProjectTransaction::TYPE_NAME: - $object->setName($xaction->getNewValue()); - // TODO - this is really "setPrimarySlug" - $object->setPhrictionSlug($xaction->getNewValue()); + $name = $xaction->getNewValue(); + $object->setName($name); + $object->setPrimarySlug(PhabricatorSlug::normalizeProjectSlug($name)); return; case PhabricatorProjectTransaction::TYPE_SLUGS: return; @@ -265,9 +265,8 @@ $errors[] = $error; } - $slug_builder = clone $object; - $slug_builder->setPhrictionSlug($name); - $slug = $slug_builder->getPrimarySlug(); + $slug = PhabricatorSlug::normalizeProjectSlug($name); + $slug_used_already = id(new PhabricatorProjectSlug()) ->loadOneWhere('slug = %s', $slug); if ($slug_used_already && diff --git a/src/applications/project/query/PhabricatorProjectQuery.php b/src/applications/project/query/PhabricatorProjectQuery.php --- a/src/applications/project/query/PhabricatorProjectQuery.php +++ b/src/applications/project/query/PhabricatorProjectQuery.php @@ -7,7 +7,6 @@ private $phids; private $memberPHIDs; private $slugs; - private $phrictionSlugs; private $names; private $nameTokens; private $icons; @@ -50,11 +49,6 @@ return $this; } - public function withPhrictionSlugs(array $slugs) { - $this->phrictionSlugs = $slugs; - return $this; - } - public function withNames(array $names) { $this->names = $names; return $this; @@ -308,13 +302,6 @@ $this->slugs); } - if ($this->phrictionSlugs !== null) { - $where[] = qsprintf( - $conn, - 'phrictionSlug IN (%Ls)', - $this->phrictionSlugs); - } - if ($this->names !== null) { $where[] = qsprintf( $conn, diff --git a/src/applications/project/storage/PhabricatorProject.php b/src/applications/project/storage/PhabricatorProject.php --- a/src/applications/project/storage/PhabricatorProject.php +++ b/src/applications/project/storage/PhabricatorProject.php @@ -189,24 +189,11 @@ return $this->assertAttached($this->memberPHIDs); } - public function setPhrictionSlug($slug) { - - // NOTE: We're doing a little magic here and stripping out '/' so that - // project pages always appear at top level under projects/ even if the - // display name is "Hack / Slash" or similar (it will become - // 'hack_slash' instead of 'hack/slash'). - - $slug = str_replace('/', ' ', $slug); - $slug = PhabricatorSlug::normalize($slug); - $this->phrictionSlug = $slug; + public function setPrimarySlug($slug) { + $this->phrictionSlug = $slug.'/'; return $this; } - public function getFullPhrictionSlug() { - $slug = $this->getPhrictionSlug(); - return 'projects/'.$slug; - } - // TODO - once we sever project => phriction automagicalness, // migrate getPhrictionSlug to have no trailing slash and be called // getPrimarySlug diff --git a/src/infrastructure/util/PhabricatorSlug.php b/src/infrastructure/util/PhabricatorSlug.php --- a/src/infrastructure/util/PhabricatorSlug.php +++ b/src/infrastructure/util/PhabricatorSlug.php @@ -4,21 +4,54 @@ public static function normalizeProjectSlug($slug) { $slug = str_replace('/', ' ', $slug); - $slug = self::normalize($slug); + $slug = self::normalize($slug, $hashtag = true); return rtrim($slug, '/'); } - public static function normalize($slug) { + public static function normalize($slug, $hashtag = false) { $slug = preg_replace('@/+@', '/', $slug); $slug = trim($slug, '/'); $slug = phutil_utf8_strtolower($slug); - $slug = preg_replace("@[\\x00-\\x19#%&+=\\\\?<> ]+@", '_', $slug); + + $ban = + // Ban control characters since users can't type them and they create + // various other problems with parsing and rendering. + "\\x00-\\x19". + + // Ban characters with special meanings in URIs (and spaces), since we + // want slugs to produce nice URIs. + "#%&+=?". + " ". + + // Ban backslashes and various brackets for parsing and URI quality. + "\\\\". + "<>{}\\[\\]". + + // Ban single and double quotes since they can mess up URIs. + "'". + '"'; + + // In hashtag mode (used for Project hashtags), ban additional characters + // which cause parsing problems. + if ($hashtag) { + $ban .= '`~!@$^*,:;(|)'; + } + + $slug = preg_replace('(['.$ban.']+)', '_', $slug); $slug = preg_replace('@_+@', '_', $slug); + $parts = explode('/', $slug); + + // Convert "_-_" into "-". This is a little nicer for inputs with + // hyphens used as internal separators, and turns an input like "A - B" + // into "a-b" instead of "a_-_b"; + foreach ($parts as $key => $part) { + $parts[$key] = str_replace('_-_', '-', $part); + } + // Remove leading and trailing underscores from each component, if the // component has not been reduced to a single underscore. For example, "a?" // converts to "a", but "??" converts to "_". - $parts = explode('/', $slug); foreach ($parts as $key => $part) { if ($part != '_') { $parts[$key] = trim($part, '_'); diff --git a/src/infrastructure/util/__tests__/PhabricatorSlugTestCase.php b/src/infrastructure/util/__tests__/PhabricatorSlugTestCase.php --- a/src/infrastructure/util/__tests__/PhabricatorSlugTestCase.php +++ b/src/infrastructure/util/__tests__/PhabricatorSlugTestCase.php @@ -34,6 +34,9 @@ 'a/??/c' => 'a/_/c/', 'a/?b/c' => 'a/b/c/', 'a/b?/c' => 'a/b/c/', + 'a - b' => 'a-b/', + 'a[b]' => 'a_b/', + 'ab!' => 'ab!/', ); foreach ($slugs as $slug => $normal) { @@ -44,6 +47,24 @@ } } + public function testProjectSlugs() { + $slugs = array( + 'a:b' => 'a_b', + 'a!b' => 'a_b', + 'a - b' => 'a-b', + '' => '', + 'Demonology: HSA (Hexes, Signs, Alchemy)' => + 'demonology_hsa_hexes_signs_alchemy', + ); + + foreach ($slugs as $slug => $normal) { + $this->assertEqual( + $normal, + PhabricatorSlug::normalizeProjectSlug($slug), + pht('Hashtag normalization of "%s"', $slug)); + } + } + public function testSlugAncestry() { $slugs = array( '/' => array(),