Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Slug Driver Support #2456

Merged
merged 22 commits into from
Dec 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 45 additions & 18 deletions js/src/admin/components/BasicsPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,6 @@ export default class BasicsPage extends Page {
'welcome_message',
'display_name_driver',
];
this.values = {};

const settings = app.data.settings;
this.fields.forEach((key) => (this.values[key] = Stream(settings[key])));

this.localeOptions = {};
const locales = app.data.locales;
Expand All @@ -42,8 +38,29 @@ export default class BasicsPage extends Page {
this.displayNameOptions[identifier] = identifier;
}, this);

this.slugDriverOptions = {};
Object.keys(app.data.slugDrivers).forEach((model) => {
this.fields.push(`slug_driver_${model}`);
this.slugDriverOptions[model] = {};

app.data.slugDrivers[model].forEach((option) => {
this.slugDriverOptions[model][option] = option;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the keys getting localized somewhere ? I mean default and idOnly for example (same with display name drivers) They don't seem to be, at least the option's label not the key itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, and mail drivers aren't either. That's a limitation worth revisiting later IMO

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now, something like app.translator.trans('core.admin.basics.slug_drivers.${option}') wouldn't work, since extensions are adding drivers, and that means they couldn't add locales for the drivers anyway.

The solution would need to be more sophisticated. Yeah worth revisiting it later.

});
});

this.values = {};

const settings = app.data.settings;
this.fields.forEach((key) => (this.values[key] = Stream(settings[key])));

if (!this.values.display_name_driver() && displayNameDrivers.includes('username')) this.values.display_name_driver('username');

Object.keys(app.data.slugDrivers).forEach((model) => {
if (!this.values[`slug_driver_${model}`]() && 'default' in this.slugDriverOptions[model]) {
this.values[`slug_driver_${model}`]('default');
}
});

if (typeof this.values.show_language_selector() !== 'number') this.values.show_language_selector(1);
}

Expand Down Expand Up @@ -132,20 +149,30 @@ export default class BasicsPage extends Page {
]
)}

{Object.keys(this.displayNameOptions).length > 1
? FieldSet.component(
{
label: app.translator.trans('core.admin.basics.display_name_heading'),
},
[
<div className="helpText">{app.translator.trans('core.admin.basics.display_name_text')}</div>,
Select.component({
options: this.displayNameOptions,
bidi: this.values.display_name_driver,
}),
]
)
: ''}
{Object.keys(this.displayNameOptions).length > 1 ? (
<FieldSet label={app.translator.trans('core.admin.basics.display_name_heading')}>
<div className="helpText">{app.translator.trans('core.admin.basics.display_name_text')}</div>
<Select
options={this.displayNameOptions}
value={this.values.display_name_driver()}
onchange={this.values.display_name_driver}
></Select>
</FieldSet>
) : (
''
)}

{Object.keys(this.slugDriverOptions).map((model) => {
const options = this.slugDriverOptions[model];
if (Object.keys(options).length > 1) {
return (
<FieldSet label={app.translator.trans('core.admin.basics.slug_driver_heading', { model })}>
<div className="helpText">{app.translator.trans('core.admin.basics.slug_driver_text', { model })}</div>
<Select options={options} value={this.values[`slug_driver_${model}`]()} onchange={this.values[`slug_driver_${model}`]}></Select>
</FieldSet>
);
}
})}

{Button.component(
{
Expand Down
1 change: 1 addition & 0 deletions js/src/common/models/User.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export default class User extends Model {}

Object.assign(User.prototype, {
username: Model.attribute('username'),
slug: Model.attribute('slug'),
displayName: Model.attribute('displayName'),
email: Model.attribute('email'),
isEmailConfirmed: Model.attribute('isEmailConfirmed'),
Expand Down
5 changes: 2 additions & 3 deletions js/src/forum/components/DiscussionListItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import DiscussionControls from '../utils/DiscussionControls';
import slidable from '../utils/slidable';
import extractText from '../../common/utils/extractText';
import classList from '../../common/utils/classList';
import DiscussionPage from './DiscussionPage';

import { escapeRegExp } from 'lodash-es';
/**
Expand Down Expand Up @@ -156,9 +157,7 @@ export default class DiscussionListItem extends Component {
* @return {Boolean}
*/
active() {
const idParam = m.route.param('id');

return idParam && idParam.split('-')[0] === this.attrs.discussion.id();
return app.current.matches(DiscussionPage, { discussion: this.attrs.discussion });
}

/**
Expand Down
3 changes: 2 additions & 1 deletion js/src/forum/components/DiscussionPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ export default class DiscussionPage extends Page {
} else {
const params = this.requestParams();

app.store.find('discussions', m.route.param('id').split('-')[0], params).then(this.show.bind(this));
app.store.find('discussions', m.route.param('id'), params).then(this.show.bind(this));
}

m.redraw();
Expand All @@ -123,6 +123,7 @@ export default class DiscussionPage extends Page {
*/
requestParams() {
return {
bySlug: true,
page: { near: this.near },
};
}
Expand Down
4 changes: 3 additions & 1 deletion js/src/forum/components/Search.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import UsersSearchSource from './UsersSearchSource';
* - state: SearchState instance.
*/
export default class Search extends Component {
static MIN_SEARCH_LEN = 3;

oninit(vnode) {
super.oninit(vnode);
this.state = this.attrs.state;
Expand Down Expand Up @@ -152,7 +154,7 @@ export default class Search extends Component {
search.searchTimeout = setTimeout(() => {
if (state.isCached(query)) return;

if (query.length >= 3) {
if (query.length >= Search.MIN_SEARCH_LEN) {
search.sources.map((source) => {
if (!source.search) return;

Expand Down
2 changes: 1 addition & 1 deletion js/src/forum/components/UserPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ export default class UserPage extends Page {
});

if (!this.user) {
app.store.find('users', username).then(this.show.bind(this));
app.store.find('users', username, { bySlug: true }).then(this.show.bind(this));
}
}

Expand Down
28 changes: 17 additions & 11 deletions js/src/forum/resolvers/DiscussionPageResolver.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,6 @@
import DefaultResolver from '../../common/resolvers/DefaultResolver';
import DiscussionPage from '../components/DiscussionPage';

/**
* This isn't exported as it is a temporary measure.
* A more robust system will be implemented alongside UTF-8 support in beta 15.
*/
function getDiscussionIdFromSlug(slug: string | undefined) {
if (!slug) return;
return slug.split('-')[0];
}

/**
* A custom route resolver for DiscussionPage that generates the same key to all posts
* on the same discussion. It triggers a scroll when going from one post to another
Expand All @@ -18,17 +9,32 @@ function getDiscussionIdFromSlug(slug: string | undefined) {
export default class DiscussionPageResolver extends DefaultResolver {
static scrollToPostNumber: string | null = null;

/**
* Remove optional parts of a discussion's slug to keep the substring
* that bijectively maps to a discussion object. By default this just
* extracts the numerical ID from the slug. If a custom discussion
* slugging driver is used, this may need to be overriden.
* @param slug
*/
canonicalizeDiscussionSlug(slug: string | undefined) {
if (!slug) return;
return slug.split('-')[0];
}

/**
* @inheritdoc
*/
makeKey() {
const params = { ...m.route.param() };
if ('near' in params) {
delete params.near;
}
params.id = getDiscussionIdFromSlug(params.id);
params.id = this.canonicalizeDiscussionSlug(params.id);
return this.routeName.replace('.near', '') + JSON.stringify(params);
}

onmatch(args, requestedPath, route) {
if (app.current.matches(DiscussionPage) && getDiscussionIdFromSlug(args.id) === getDiscussionIdFromSlug(m.route.param('id'))) {
if (app.current.matches(DiscussionPage) && this.canonicalizeDiscussionSlug(args.id) === this.canonicalizeDiscussionSlug(m.route.param('id'))) {
// By default, the first post number of any discussion is 1
DiscussionPageResolver.scrollToPostNumber = args.near || '1';
}
Expand Down
5 changes: 2 additions & 3 deletions js/src/forum/routes.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,8 @@ export default function (app) {
* @return {String}
*/
app.route.discussion = (discussion, near) => {
const slug = discussion.slug();
return app.route(near && near !== 1 ? 'discussion.near' : 'discussion', {
id: discussion.id() + (slug.trim() ? '-' + slug : ''),
id: discussion.slug(),
near: near && near !== 1 ? near : undefined,
});
};
Expand All @@ -59,7 +58,7 @@ export default function (app) {
*/
app.route.user = (user) => {
return app.route('user', {
username: user.username(),
username: user.slug(),
});
};
}
3 changes: 3 additions & 0 deletions src/Admin/Content/AdminPayload.php
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ public function __invoke(Document $document, Request $request)
$document->payload['extensions'] = $this->extensions->getExtensions()->toArray();

$document->payload['displayNameDrivers'] = array_keys($this->container->make('flarum.user.display_name.supported_drivers'));
$document->payload['slugDrivers'] = array_map(function ($resourceDrivers) {
askvortsov1 marked this conversation as resolved.
Show resolved Hide resolved
return array_keys($resourceDrivers);
}, $this->container->make('flarum.http.slugDrivers'));

$document->payload['phpVersion'] = PHP_VERSION;
$document->payload['mysqlVersion'] = $this->db->selectOne('select version() as version')->version;
Expand Down
16 changes: 14 additions & 2 deletions src/Api/Controller/ShowDiscussionController.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use Flarum\Api\Serializer\DiscussionSerializer;
use Flarum\Discussion\Discussion;
use Flarum\Discussion\DiscussionRepository;
use Flarum\Http\SlugManager;
use Flarum\Post\PostRepository;
use Flarum\User\User;
use Illuminate\Support\Arr;
Expand All @@ -31,6 +32,11 @@ class ShowDiscussionController extends AbstractShowController
*/
protected $posts;

/**
* @var SlugManager
*/
protected $slugManager;

/**
* {@inheritdoc}
*/
Expand Down Expand Up @@ -61,11 +67,13 @@ class ShowDiscussionController extends AbstractShowController
/**
* @param \Flarum\Discussion\DiscussionRepository $discussions
* @param \Flarum\Post\PostRepository $posts
* @param \Flarum\Http\SlugManager $slugManager
*/
public function __construct(DiscussionRepository $discussions, PostRepository $posts)
public function __construct(DiscussionRepository $discussions, PostRepository $posts, SlugManager $slugManager)
{
$this->discussions = $discussions;
$this->posts = $posts;
$this->slugManager = $slugManager;
}

/**
Expand All @@ -77,7 +85,11 @@ protected function data(ServerRequestInterface $request, Document $document)
$actor = $request->getAttribute('actor');
$include = $this->extractInclude($request);

$discussion = $this->discussions->findOrFail($discussionId, $actor);
if (Arr::get($request->getQueryParams(), 'bySlug', false)) {
$discussion = $this->slugManager->forResource(Discussion::class)->fromSlug($discussionId, $actor);
} else {
$discussion = $this->discussions->findOrFail($discussionId, $actor);
}
luceos marked this conversation as resolved.
Show resolved Hide resolved

if (in_array('posts', $include)) {
$postRelationships = $this->getPostRelationships($include);
Expand Down
28 changes: 19 additions & 9 deletions src/Api/Controller/ShowUserController.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

use Flarum\Api\Serializer\CurrentUserSerializer;
use Flarum\Api\Serializer\UserSerializer;
use Flarum\Http\SlugManager;
use Flarum\User\User;
use Flarum\User\UserRepository;
use Illuminate\Support\Arr;
use Psr\Http\Message\ServerRequestInterface;
Expand All @@ -29,15 +31,22 @@ class ShowUserController extends AbstractShowController
public $include = ['groups'];

/**
* @var \Flarum\User\UserRepository
* @var SlugManager
*/
protected $slugManager;

/**
* @var UserRepository
*/
protected $users;

/**
* @param \Flarum\User\UserRepository $users
* @param SlugManager $slugManager
* @param UserRepository $users
*/
public function __construct(UserRepository $users)
public function __construct(SlugManager $slugManager, UserRepository $users)
{
$this->slugManager = $slugManager;
$this->users = $users;
}

Expand All @@ -47,17 +56,18 @@ public function __construct(UserRepository $users)
protected function data(ServerRequestInterface $request, Document $document)
{
$id = Arr::get($request->getQueryParams(), 'id');
$actor = $request->getAttribute('actor');

if (! is_numeric($id)) {
$id = $this->users->getIdForUsername($id);
if (Arr::get($request->getQueryParams(), 'bySlug', false)) {
$user = $this->slugManager->forResource(User::class)->fromSlug($id, $actor);
} else {
$user = $this->users->findOrFail($id, $actor);
}

$actor = $request->getAttribute('actor');

if ($actor->id == $id) {
if ($actor->id === $user->id) {
$this->serializer = CurrentUserSerializer::class;
}

return $this->users->findOrFail($id, $actor);
return $user;
}
}
13 changes: 12 additions & 1 deletion src/Api/Serializer/BasicDiscussionSerializer.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
namespace Flarum\Api\Serializer;

use Flarum\Discussion\Discussion;
use Flarum\Http\SlugManager;
use InvalidArgumentException;

class BasicDiscussionSerializer extends AbstractSerializer
Expand All @@ -19,6 +20,16 @@ class BasicDiscussionSerializer extends AbstractSerializer
*/
protected $type = 'discussions';

/**
* @var SlugManager
*/
protected $slugManager;

public function __construct(SlugManager $slugManager)
{
$this->slugManager = $slugManager;
}

/**
* {@inheritdoc}
*
Expand All @@ -35,7 +46,7 @@ protected function getDefaultAttributes($discussion)

return [
'title' => $discussion->title,
'slug' => $discussion->slug,
'slug' => $this->slugManager->forResource(Discussion::class)->toSlug($discussion),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be easier to have a method on the abstract model instead?

$discussion->slugger->slug();

So that we resolve the slug manager late here as well, plus we can reduce the overhead on every, single class that tries to resolve the slug manager..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused how that would work tbh. I'm very wary about adding stuff to AbstractModel unless it absolutely affects ALL models, which isn't the case here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if it's a trait one can apply to the model, eg Discussion has the Sluggable trait. It adds that trait, sets the property protected $slugger = DiscussionSlugger::class; and the trait handles the rest?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this. It adds unnecessary static stuff to the models, and I'd really prefer to move away from that as much as we possibly can because it's unnecessary leakage of global state. It also makes the extension mechanism a lot more hacky (similar to what I had to do with #2460), which I really didn't like and will revisit again after stable.

];
}

Expand Down
Loading