[2.x] feat(core, mentions, likes): implement IdWithDisplayNameSlugDriver

[DavideIadeluca]

Documentation PR at flarum/docs#514

Fixes #0000

Changes proposed in this pull request:
Implements a new slug driver for User. This slug driver works very similarly to the Discussion Slug driver in that that the id is used as the main mechanism for routing, but the models slug added for cosmetics.

Modifies the frontend routes extender to support adding a custom resolver class. The new UserPageResolver is needed to patch the URL in case the user has changed their username/nickname.

Example:
http://localhost/u/1-nonsense-slug-which-is-not-the-actual-user-or-nickname/likes is replaced with http://localhost/u/1-admin-username/likes

Reviewers should focus on:
Should in theory be backwards compatible. Needs a accompanying documentation PR for extension developers to start to consume the UserPageResolver when adding routes like this:

new Extend.Routes() //
    .add('user.foobar', '/u/:username/foobar', FoobarUserPage, UserPageResolver),

Screenshot

Necessity

• Has the problem that is being solved here been clearly explained?
• If applicable, have various options for solving this problem been considered?
• For core PRs, does this need to be in core, or could it be in an extension?
• Are we willing to maintain this for years / potentially forever?

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).
• Core developer confirmed locally this works as intended.
• Tests have been added, or are not appropriate here.

Required changes:

• Related documentation PR: (Remove if irrelevant)

[imorland]

The approach is clean and consistent with how DiscussionPageResolver works. A few things to address before merging:

Bug: strpos falsiness in fromSlug

if (strpos($slug, '-')) {

This is falsy when the slug starts with - (returns 0). Should be:

if (strpos($slug, '-') !== false) {

Empty display name edge case

Str::slug('') on a null/empty display_name produces a trailing -, e.g. 2-. Worth guarding:

$suffix = Str::slug($instance->display_name);
return $suffix ? $instance->id . '-' . $suffix : (string) $instance->id;

Missing fromSlug tests

Only slug generation is tested. Would be good to add cases for:

• fromSlug('2-normal', $actor) → user 2
• fromSlug('2', $actor) → user 2 (bare ID fallback)
• fromSlug('2-wrong-slug', $actor) → user 2 (ID takes precedence)

FlarumGenericRoute type

Routes.ts sets route.resolverClass but the diff doesn't show the type being updated. If resolverClass isn't already on FlarumGenericRoute, this will be an implicit any. Can you confirm?

URL canonicalization timing

On first page load, app.store.getById returns null (user not yet loaded), so replaceState won't fire until a re-render. Same behavior as DiscussionPageResolver so it's acceptable — just flagging it as a known limitation in case it comes up.

[DavideIadeluca]

Bug: strpos falsiness in fromSlug

if (strpos($slug, '-')) {
This is falsy when the slug starts with - (returns 0). Should be:

if (strpos($slug, '-') !== false) {

Followed up with quite a bit of manual testing and research and it seems like this is more of a general PHP gotcha then an issue in this implementation. Technically it's not needed here at this stage but for future robustness I've added it anyways in 44d4e60.

---

Empty display name edge case

Str::slug('') on a null/empty display_name produces a trailing -, e.g. 2-. Worth guarding:

$suffix = Str::slug($instance->display_name);
return $suffix ? $instance->id . '-' . $suffix : (string) $instance->id;

I actually had a similar fallback to whats suggested here in place at the early stages of developing this PR but removed it as $user->display_name is very safe to consume. If $user->display_name ever becomes unavailable other parts of the forum would break as well. So I'd say this is no more or less safe then what is already in the framework

---

FlarumGenericRoute type

Routes.ts sets route.resolverClass but the diff doesn't show the type being updated. If resolverClass isn't already on FlarumGenericRoute, this will be an implicit any. Can you confirm?

Double checked this one: resolverClass is already defined on RouteItem and is fully typed so it checks out from what I can tell.

---

Missing fromSlug tests

Only slug generation is tested. Would be good to add cases for:

fromSlug('2-normal', $actor) → user 2
fromSlug('2', $actor) → user 2 (bare ID fallback)
fromSlug('2-wrong-slug', $actor) → user 2 (ID takes precedence)

Yeah good shout! Why not..

Added in 9df5e67

---

URL canonicalization timing

On first page load, app.store.getById returns null (user not yet loaded), so replaceState won't fire until a re-render. Same behavior as DiscussionPageResolver so it's acceptable — just flagging it as a known limitation in case it comes up.

In most cases this doesn't apply as the user in question is already preloaded by the backend. If a route is resolved for a user which is not yet in the store then yes there is a tiny delay until the state is replaced. As you said the behaviour is similar for the DiscussionPageResolver so I think nothing more needs to be done here.