Improve ruleset for generating project hashtags

Summary:
Ref T9551. We currently use the same logic for generating project hashtags and Phriction slugs, but should be a little more conservative with project hashtags.

Stop them from generating with stuff that won't parse in a "Reviewers:" field or generally in commments (commas, colons, etc).

Test Plan:
Created a bunch of projects with nonsense in them and saw them generate pretty reasonable hashtags.

{F873456}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9551

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

resources/sql/autopatches/20141107.phriction.policy.2.php

@@ -24,12 +24,14 @@ foreach (new LiskMigrationIterator($table) as $doc) {
   $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) {

resources/sql/patches/090.forceuniqueprojectnames.php

@@ -10,13 +10,14 @@ $projects = $table->loadAll();
 $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 @@ while ($update) {
   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 @@ function rename_project($project, $projects) {
   $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) {

src/applications/project/conduit/ProjectQueryConduitAPIMethod.php

@@ -112,7 +112,7 @@ final class ProjectQueryConduitAPIMethod extends ProjectConduitAPIMethod {
     $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'];

src/applications/project/editor/PhabricatorProjectTransactionEditor.php

@@ -81,9 +81,9 @@ final class PhabricatorProjectTransactionEditor
 
     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 @@ final class PhabricatorProjectTransactionEditor
           $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 &&

src/applications/project/query/PhabricatorProjectQuery.php

@@ -7,7 +7,6 @@ final class PhabricatorProjectQuery
   private $phids;
   private $memberPHIDs;
   private $slugs;
-  private $phrictionSlugs;
   private $names;
   private $nameTokens;
   private $icons;
@@ -50,11 +49,6 @@ final class PhabricatorProjectQuery
     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 @@ final class PhabricatorProjectQuery
         $this->slugs);
     }
 
-    if ($this->phrictionSlugs !== null) {
-      $where[] = qsprintf(
-        $conn,
-        'phrictionSlug IN (%Ls)',
-        $this->phrictionSlugs);
-    }
-
     if ($this->names !== null) {
       $where[] = qsprintf(
         $conn,

src/applications/project/storage/PhabricatorProject.php

@@ -189,24 +189,11 @@ final class PhabricatorProject extends PhabricatorProjectDAO
     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

src/infrastructure/util/PhabricatorSlug.php

@@ -4,21 +4,54 @@ final class PhabricatorSlug extends Phobject {
 
   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, '_');

src/infrastructure/util/__tests__/PhabricatorSlugTestCase.php

@@ -34,6 +34,9 @@ final class PhabricatorSlugTestCase extends PhabricatorTestCase {
       '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 @@ final class PhabricatorSlugTestCase extends PhabricatorTestCase {
     }
   }
 
+  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(),