feat: allow using utf8 characters in tag slugs (#3588)

* feat: allow using utf8 characters in slugs
url-encoded slugs are not read by the backend.
* chore: use as a slug driver
* chore: refactor tests to use data provider
* Apply fixes from StyleCI
* fix: wrong resource used
* fix: forgotten slug from slug manager in serializer
* chore(review): adapt tag slug suggestions on the UI
* chore: introduce modes for slugging
* chore: `yarn format`

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

extensions/tags/extend.php

@@ -30,6 +30,7 @@ use Flarum\Tags\LoadForumTagsRelationship;
 use Flarum\Tags\Post\DiscussionTaggedPost;
 use Flarum\Tags\Query\TagFilterGambit;
 use Flarum\Tags\Tag;
+use Flarum\Tags\Utf8SlugDriver;
 use Psr\Http\Message\ServerRequestInterface;
 
 $eagerLoadTagState = function ($query, ?ServerRequestInterface $request, array $relations) {
@@ -133,4 +134,7 @@ return [
 
     (new Extend\SimpleFlarumSearch(DiscussionSearcher::class))
         ->addGambit(TagFilterGambit::class),
+
+    (new Extend\ModelUrl(Tag::class))
+        ->addSlugDriver('default', Utf8SlugDriver::class),
 ];

extensions/tags/src/Api/Controller/ShowTagController.php

@@ -11,7 +11,9 @@ namespace Flarum\Tags\Api\Controller;
 
 use Flarum\Api\Controller\AbstractShowController;
 use Flarum\Http\RequestUtil;
+use Flarum\Http\SlugManager;
 use Flarum\Tags\Api\Serializer\TagSerializer;
+use Flarum\Tags\Tag;
 use Flarum\Tags\TagRepository;
 use Illuminate\Support\Arr;
 use Psr\Http\Message\ServerRequestInterface;
@@ -34,11 +36,17 @@ class ShowTagController extends AbstractShowController
     /**
      * @var TagRepository
      */
-    private $tags;
+    protected $tags;
 
-    public function __construct(TagRepository $tags)
+    /**
+     * @var SlugManager
+     */
+    protected $slugger;
+
+    public function __construct(TagRepository $tags, SlugManager $slugger)
     {
         $this->tags = $tags;
+        $this->slugger = $slugger;
     }
 
     /**
@@ -57,11 +65,11 @@ class ShowTagController extends AbstractShowController
             $include = array_unique(array_diff($include, ['parent.children.parent']));
         }
 
-        $tag = $this->tags
+        $tag = $this->slugger
-            ->with($include, $actor)
+            ->forResource(Tag::class)
-            ->whereVisibleTo($actor)
+            ->fromSlug($slug, $actor);
-            ->where('slug', $slug)
+
-            ->firstOrFail();
+        $tag->load($this->tags->getAuthorizedRelations($include, $actor));
 
         if ($setParentOnChildren && $tag->parent) {
             foreach ($tag->parent->children as $child) {

extensions/tags/src/Api/Serializer/TagSerializer.php

@@ -11,6 +11,8 @@ namespace Flarum\Tags\Api\Serializer;
 
 use Flarum\Api\Serializer\AbstractSerializer;
 use Flarum\Api\Serializer\DiscussionSerializer;
+use Flarum\Http\SlugManager;
+use Flarum\Tags\Tag;
 
 class TagSerializer extends AbstractSerializer
 {
@@ -20,14 +22,27 @@ class TagSerializer extends AbstractSerializer
     protected $type = 'tags';
 
     /**
-     * {@inheritdoc}
+     * @var SlugManager
+     */
+    protected $slugManager;
+
+    public function __construct(SlugManager $slugManager)
+    {
+        $this->slugManager = $slugManager;
+    }
+
+    /**
+     * Get the default set of serialized attributes for a model.
+     *
+     * @param Tag $tag
+     * @return array
      */
     protected function getDefaultAttributes($tag)
     {
         $attributes = [
             'name'               => $tag->name,
             'description'        => $tag->description,
-            'slug'               => $tag->slug,
+            'slug'               => $this->slugManager->forResource(Tag::class)->toSlug($tag),
             'color'              => $tag->color,
             'backgroundUrl'      => $tag->background_path,
             'backgroundMode'     => $tag->background_mode,

extensions/tags/src/Content/Tag.php

@@ -12,6 +12,8 @@ namespace Flarum\Tags\Content;
 use Flarum\Api\Client;
 use Flarum\Frontend\Document;
 use Flarum\Http\RequestUtil;
+use Flarum\Http\SlugManager;
+use Flarum\Tags\Tag as TagModel;
 use Flarum\Tags\TagRepository;
 use Illuminate\Contracts\View\Factory;
 use Illuminate\Support\Arr;
@@ -41,17 +43,22 @@ class Tag
     protected $translator;
 
     /**
-     * @param Client $api
+     * @var SlugManager
-     * @param Factory $view
-     * @param TagRepository $tags
-     * @param TranslatorInterface $translator
      */
-    public function __construct(Client $api, Factory $view, TagRepository $tags, TranslatorInterface $translator)
+    protected $slugger;
-    {
+
+    public function __construct(
+        Client $api,
+        Factory $view,
+        TagRepository $tags,
+        TranslatorInterface $translator,
+        SlugManager $slugger
+    ) {
         $this->api = $api;
         $this->view = $view;
         $this->tags = $tags;
         $this->translator = $translator;
+        $this->slugger = $slugger;
     }
 
     public function __invoke(Document $document, Request $request)
@@ -67,8 +74,7 @@ class Tag
 
         $sortMap = $this->getSortMap();
 
-        $tagId = $this->tags->getIdForSlug($slug);
+        $tag = $this->slugger->forResource(TagModel::class)->fromSlug($slug, $actor);
-        $tag = $this->tags->findOrFail($tagId, $actor);
 
         $params = [
             'sort' => $sort && isset($sortMap[$sort]) ? $sortMap[$sort] : '',

extensions/tags/src/Query/TagFilterGambit.php

@@ -11,24 +11,24 @@ namespace Flarum\Tags\Query;
 
 use Flarum\Filter\FilterInterface;
 use Flarum\Filter\FilterState;
+use Flarum\Http\SlugManager;
 use Flarum\Search\AbstractRegexGambit;
 use Flarum\Search\SearchState;
-use Flarum\Tags\TagRepository;
+use Flarum\Tags\Tag;
+use Flarum\User\User;
+use Illuminate\Database\Eloquent\ModelNotFoundException;
 use Illuminate\Database\Query\Builder;
 
 class TagFilterGambit extends AbstractRegexGambit implements FilterInterface
 {
     /**
-     * @var TagRepository
+     * @var SlugManager
      */
-    protected $tags;
+    protected $slugger;
 
-    /**
+    public function __construct(SlugManager $slugger)
-     * @param TagRepository $tags
-     */
-    public function __construct(TagRepository $tags)
     {
-        $this->tags = $tags;
+        $this->slugger = $slugger;
     }
 
     protected function getGambitPattern()
@@ -48,14 +48,14 @@ class TagFilterGambit extends AbstractRegexGambit implements FilterInterface
 
     public function filter(FilterState $filterState, string $filterValue, bool $negate)
     {
-        $this->constrain($filterState->getQuery(), $filterValue, $negate);
+        $this->constrain($filterState->getQuery(), $filterValue, $negate, $filterState->getActor());
     }
 
-    protected function constrain(Builder $query, $rawSlugs, $negate)
+    protected function constrain(Builder $query, $rawSlugs, $negate, User $actor)
     {
         $slugs = explode(',', trim($rawSlugs, '"'));
 
-        $query->where(function (Builder $query) use ($slugs, $negate) {
+        $query->where(function (Builder $query) use ($slugs, $negate, $actor) {
             foreach ($slugs as $slug) {
                 if ($slug === 'untagged') {
                     $query->whereIn('discussions.id', function (Builder $query) {
@@ -63,7 +63,12 @@ class TagFilterGambit extends AbstractRegexGambit implements FilterInterface
                             ->from('discussion_tag');
                     }, 'or', ! $negate);
                 } else {
-                    $id = $this->tags->getIdForSlug($slug);
+                    // @TODO: grab all IDs first instead of multiple queries.
+                    try {
+                        $id = $this->slugger->forResource(Tag::class)->fromSlug($slug, $actor)->id;
+                    } catch (ModelNotFoundException $e) {
+                        $id = null;
+                    }
 
                     $query->whereIn('discussions.id', function (Builder $query) use ($id) {
                         $query->select('discussion_id')

extensions/tags/src/TagRepository.php

@@ -32,6 +32,16 @@ class TagRepository
      * @return Builder
      */
     public function with($relations, User $actor): Builder
+    {
+        return $this->query()->with($this->getAuthorizedRelations($relations, $actor));
+    }
+
+    /**
+     * @param array|string $relations
+     * @param User $actor
+     * @return array
+     */
+    public function getAuthorizedRelations($relations, User $actor): array
     {
         $relations = is_string($relations) ? explode(',', $relations) : $relations;
         $relationsArray = [];
@@ -46,7 +56,7 @@ class TagRepository
             }
         }
 
-        return $this->query()->with($relationsArray);
+        return $relationsArray;
     }
 
     /**

extensions/tags/src/Utf8SlugDriver.php

@@ -0,0 +1,53 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Tags;
+
+use Flarum\Database\AbstractModel;
+use Flarum\Http\SlugDriverInterface;
+use Flarum\User\User;
+
+/**
+ * @implements SlugDriverInterface<Tag>
+ */
+class Utf8SlugDriver implements SlugDriverInterface
+{
+    /**
+     * @var TagRepository
+     */
+    protected $repository;
+
+    public function __construct(TagRepository $repository)
+    {
+        $this->repository = $repository;
+    }
+
+    /**
+     * @param Tag $instance
+     * @return string
+     */
+    public function toSlug(AbstractModel $instance): string
+    {
+        return $instance->slug;
+    }
+
+    /**
+     * @param string $slug
+     * @param User $actor
+     * @return Tag
+     */
+    public function fromSlug(string $slug, User $actor): AbstractModel
+    {
+        return $this->repository
+            ->query()
+            ->where('slug', urldecode($slug))
+            ->whereVisibleTo($actor)
+            ->firstOrFail();
+    }
+}

extensions/tags/tests/integration/api/discussions/ListTest.php

@@ -9,7 +9,6 @@
 
 namespace Flarum\Tags\Tests\integration\api\discussions;
 
-use Flarum\Group\Group;
 use Flarum\Tags\Tests\integration\RetrievesRepresentativeTags;
 use Flarum\Testing\integration\RetrievesAuthorizedUsers;
 use Flarum\Testing\integration\TestCase;
@@ -33,12 +32,25 @@ class ListTest extends TestCase
             'tags' => $this->tags(),
             'users' => [
                 $this->normalUser(),
+                [
+                    'id' => 3,
+                    'username' => 'normal3',
+                    'password' => '$2y$10$LO59tiT7uggl6Oe23o/O6.utnF6ipngYjvMvaxo1TciKqBttDNKim', // BCrypt hash for "too-obscure"
+                    'email' => 'normal3@machine.local',
+                    'is_email_confirmed' => 1,
+                ]
+            ],
+            'groups' => [
+                ['id' => 100, 'name_singular' => 'acme', 'name_plural' => 'acme']
+            ],
+            'group_user' => [
+                ['group_id' => 100, 'user_id' => 2]
             ],
             'group_permission' => [
-                ['group_id' => Group::MEMBER_ID, 'permission' => 'tag5.viewForum'],
+                ['group_id' => 100, 'permission' => 'tag5.viewForum'],
-                ['group_id' => Group::MEMBER_ID, 'permission' => 'tag8.viewForum'],
+                ['group_id' => 100, 'permission' => 'tag8.viewForum'],
-                ['group_id' => Group::MEMBER_ID, 'permission' => 'tag11.viewForum'],
+                ['group_id' => 100, 'permission' => 'tag11.viewForum'],
-                ['group_id' => Group::MEMBER_ID, 'permission' => 'tag13.viewForum'],
+                ['group_id' => 100, 'permission' => 'tag13.viewForum'],
             ],
             'discussions' => [
                 ['id' => 1, 'title' => 'no tags', 'user_id' => 1, 'comment_count' => 1],
@@ -149,59 +161,22 @@ class ListTest extends TestCase
     }
 
     /**
+     * @dataProvider seeWhereAllowedWhenMoreTagsAreRequiredThanAvailableDataProvider
      * @test
      */
-    public function admin_can_see_where_allowed_when_more_primary_tags_are_required_than_available()
+    public function actor_can_see_where_allowed_when_more_tags_are_required_than_available(string $type, int $actorId, array $expectedDiscussions)
     {
-        $this->actor_can_see_where_allowed_when_more_primary_tags_are_required_than_available(1, ['1', '2', '3', '4', '5', '6']);
+        if ($type === 'secondary') {
-    }
+            $this->setting('flarum-tags.min_secondary_tags', 1);
-
+            $this->database()->table('tags')
-    /**
+                ->where(['position' => null, 'parent_id' => null])
-     * @test
+                ->update(['position' => 200]);
-     */
+        } elseif ($type === 'primary') {
-    public function user_can_see_where_allowed_when_more_primary_tags_are_required_than_available()
+            $this->setting('flarum-tags.min_primary_tags', 100);
-    {
+            $this->database()->table('tags')
-        $this->actor_can_see_where_allowed_when_more_primary_tags_are_required_than_available(2, ['1', '2', '3', '4']);
+                ->where('position', '!=', null)
-    }
+                ->update(['position' => null]);
-
+        }
-    /**
-     * @test
-     */
-    public function guest_can_see_where_allowed_when_more_primary_tags_are_required_than_available()
-    {
-        $this->actor_can_see_where_allowed_when_more_primary_tags_are_required_than_available(0, ['1', '2']);
-    }
-
-    /**
-     * @test
-     */
-    public function admin_can_see_where_allowed_when_more_secondary_tags_are_required_than_available()
-    {
-        $this->actor_can_see_where_allowed_when_more_secondary_tags_are_required_than_available(1, ['1', '2', '3', '4', '5', '6']);
-    }
-
-    /**
-     * @test
-     */
-    public function user_can_see_where_allowed_when_more_secondary_tags_are_required_than_available()
-    {
-        $this->actor_can_see_where_allowed_when_more_secondary_tags_are_required_than_available(2, ['1', '2', '3', '4']);
-    }
-
-    /**
-     * @test
-     */
-    public function guest_can_see_where_allowed_when_more_secondary_tags_are_required_than_available()
-    {
-        $this->actor_can_see_where_allowed_when_more_secondary_tags_are_required_than_available(0, ['1', '2']);
-    }
-
-    public function actor_can_see_where_allowed_when_more_primary_tags_are_required_than_available(int $actorId, array $expectedDiscussions)
-    {
-        $this->setting('flarum-tags.min_primary_tags', 100);
-        $this->database()->table('tags')
-            ->where('position', '!=', null)
-            ->update(['position' => null]);
 
         if ($actorId) {
             $reqParams = [
@@ -221,21 +196,37 @@ class ListTest extends TestCase
         $this->assertEqualsCanonicalizing($expectedDiscussions, $ids);
     }
 
-    public function actor_can_see_where_allowed_when_more_secondary_tags_are_required_than_available(int $actorId, array $expectedDiscussions)
+    public function seeWhereAllowedWhenMoreTagsAreRequiredThanAvailableDataProvider(): array
     {
-        $this->setting('flarum-tags.min_secondary_tags', 1);
+        return [
-        $this->database()->table('tags')
+            // admin_can_see_where_allowed_when_more_primary_tags_are_required_than_available
-            ->where(['position' => null, 'parent_id' => null])
+            ['primary', 1, ['1', '2', '3', '4', '5', '6']],
-            ->update(['position' => 200]);
+            // user_can_see_where_allowed_when_more_primary_tags_are_required_than_available
-
+            ['primary', 2, ['1', '2', '3', '4']],
-        if ($actorId) {
+            // guest_can_see_where_allowed_when_more_primary_tags_are_required_than_available
-            $reqParams = [
+            ['primary', 0, ['1', '2']],
-                'authenticatedAs' => $actorId
+            // admin_can_see_where_allowed_when_more_secondary_tags_are_required_than_available
-            ];
+            ['secondary', 1, ['1', '2', '3', '4', '5', '6']],
-        }
+            // user_can_see_where_allowed_when_more_secondary_tags_are_required_than_available
+            ['secondary', 2, ['1', '2', '3', '4']],
+            // guest_can_see_where_allowed_when_more_secondary_tags_are_required_than_available
+            ['secondary', 0, ['1', '2']],
+        ];
+    }
 
+    /**
+     * @dataProvider filterByTagsDataProvider
+     * @test
+     */
+    public function can_filter_by_authorized_tags(int $authenticatedAs, string $tags, array $expectedDiscussionIds)
+    {
         $response = $this->send(
-            $this->request('GET', '/api/discussions', $reqParams ?? [])
+            $this->request('GET', '/api/discussions', compact('authenticatedAs'))
+                ->withQueryParams([
+                    'filter' => [
+                        'tag' => $tags
+                    ]
+                ])
         );
 
         $this->assertEquals(200, $response->getStatusCode());
@@ -243,6 +234,31 @@ class ListTest extends TestCase
         $data = json_decode($response->getBody()->getContents(), true)['data'];
 
         $ids = Arr::pluck($data, 'id');
-        $this->assertEqualsCanonicalizing($expectedDiscussions, $ids);
+        $this->assertEqualsCanonicalizing($expectedDiscussionIds, array_map('intval', $ids));
+    }
+
+    public function filterByTagsDataProvider(): array
+    {
+        return [
+            // Admin can filter by any tag.
+            [1, 'primary-1,primary-2', [2, 3, 4]],
+            [1, 'primary-1,primary-2,primary-restricted', [2, 3, 4, 5]],
+            [1, 'primary-2-restricted-child-1', [6]],
+            [1, 'untagged', [1]],
+
+            // Normal user can only filter by tags he has access to.
+            [3, 'primary-2-restricted-child-1', []],
+            [3, 'primary-1', [2]],
+            [3, 'primary-1,primary-2', [2]],
+            [3, 'untagged', [1]],
+
+            // Authorized User can only filter by tags he has access to.
+            [2, 'primary-2-restricted-child-1', []],
+            [2, 'primary-2-restricted-child-1,primary-restricted-child-restricted', []],
+            [2, 'primary-1', [2, 4]],
+            [2, 'primary-1,primary-2', [2, 3, 4]],
+            [2, 'secondary-restricted', [4]],
+            [2, 'untagged', [1]],
+        ];
     }
 }

extensions/tags/tests/integration/api/tags/ListTest.php

@@ -83,7 +83,7 @@ class ListTest extends TestCase
     }
 
     /**
-     * @dataProvider listTagsIncludes
+     * @dataProvider listTagsIncludesDataProvider
      * @test
      */
     public function user_sees_where_allowed_with_included_tags(string $include, array $expectedIncludes)
@@ -127,7 +127,7 @@ class ListTest extends TestCase
         $this->assertEquals(['1', '2', '3', '4', '9', '10'], $ids);
     }
 
-    public function listTagsIncludes(): array
+    public function listTagsIncludesDataProvider(): array
     {
         return [
             ['children', ['3', '4']],

extensions/tags/tests/integration/api/tags/ShowTest.php

@@ -41,6 +41,50 @@ class ShowTest extends TestCase
         ]);
     }
 
+    /** @test */
+    public function can_show_tag_with_url_decoded_utf8_slug()
+    {
+        $this->prepareDatabase([
+            'tags' => [
+                ['id' => 155, 'name' => '测试', 'slug' => '测试', 'position' => 0, 'parent_id' => null]
+            ]
+        ]);
+
+        $response = $this->send(
+            $this->request('GET', '/api/tags/测试')
+        );
+
+        $this->assertEquals(200, $response->getStatusCode());
+
+        $response2 = $this->send(
+            $this->request('GET', '/t/测试')
+        );
+
+        $this->assertEquals(200, $response2->getStatusCode());
+    }
+
+    /** @test */
+    public function can_show_tag_with_url_encoded_utf8_slug()
+    {
+        $this->prepareDatabase([
+            'tags' => [
+                ['id' => 155, 'name' => '测试', 'slug' => '测试', 'position' => 0, 'parent_id' => null]
+            ]
+        ]);
+
+        $response = $this->send(
+            $this->request('GET', '/api/tags/'.urlencode('测试'))
+        );
+
+        $this->assertEquals(200, $response->getStatusCode());
+
+        $response2 = $this->send(
+            $this->request('GET', '/t/'.urlencode('测试'))
+        );
+
+        $this->assertEquals(200, $response2->getStatusCode());
+    }
+
     /**
      * @dataProvider showTagIncludes
      * @test

framework/core/js/src/common/utils/string.ts

@@ -6,19 +6,38 @@ export function truncate(string: string, length: number, start: number = 0): str
 }
 
 /**
- * Create a slug out of the given string. Non-alphanumeric characters are
+ * Create a slug out of the given string depending on the selected mode.
- * converted to hyphens.
+ * Invalid characters are converted to hyphens.
  *
  * NOTE: This method does not use the comparably sophisticated transliteration
  * mechanism that is employed in the backend. Therefore, it should only be used
  * to *suggest* slugs that can be overridden by the user.
  */
-export function slug(string: string): string {
+export function slug(string: string, mode: SluggingMode = SluggingMode.ALPHANUMERIC): string {
-  return string
+  switch (mode) {
-    .toLowerCase()
+    case SluggingMode.UTF8:
-    .replace(/[^a-z0-9]/gi, '-')
+      return (
-    .replace(/-+/g, '-')
+        string
-    .replace(/-$|^-/g, '');
+          .toLowerCase()
+          // Match non-word characters (take UTF8 into consideration) and replace with a dash.
+          .replace(/[^\p{L}\p{N}\p{M}]/giu, '-')
+          .replace(/-+/g, '-')
+          .replace(/-$|^-/g, '')
+      );
+
+    case SluggingMode.ALPHANUMERIC:
+    default:
+      return string
+        .toLowerCase()
+        .replace(/[^a-z0-9]/gi, '-')
+        .replace(/-+/g, '-')
+        .replace(/-$|^-/g, '');
+  }
+}
+
+enum SluggingMode {
+  ALPHANUMERIC = 'alphanum',
+  UTF8 = 'utf8',
 }
 
 /**